Tangy form event#16
Conversation
Summary of ChangesHello @faridy-mahvish, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates xAPI (Experience API) functionality into the Tangy Forms player. It enables the collection and submission of xAPI statements based on user interactions within a form. Key changes include a new service for sending xAPI data, modifications to the form player component to capture and dispatch these statements, and new TypeScript interfaces to define the structure of xAPI statements and form responses. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Thank you for your contribution. I've reviewed the changes and have some suggestions to improve correctness, maintainability, and adherence to best practices. My main points are around handling circular dependencies, improving robustness in data parsing, modernizing asynchronous code, and ensuring type safety. Please see my detailed comments below.
| tangyForm.addEventListener('TANGY_FORM_UPDATE', async (event) => { | ||
| let response = event.target.store.getState() | ||
| let response = event.target.store.getState(); | ||
| this.xapiResponse = []; |
There was a problem hiding this comment.
xapiResponse is a misleading variable name: this would imply it is the response for sending a statement: it isn't. You are taking in the statements (without actor), and putting the actor into the statement. Could be more accurately called complete statements or statementsWithActor
| @@ -0,0 +1,132 @@ | |||
| export interface TangyFormResponse { | |||
There was a problem hiding this comment.
TangyFormResponse, Form, TangyFormItem, and TangyInput interfaces are not related to xAPI, they are not used by xAPI related code, they must be removed and should not be part of this pull request.
| export interface XapiChoice { | ||
| id: string; | ||
| description: { | ||
| "en-US": string; |
There was a problem hiding this comment.
The Xapi interfaces also need to be removed: these are not compliant with the xAPI spec (e.g. statements can be of four different types, the result is optional, there are extensions, etc. This could get in the way of a valid statement.
This code is not responsible to validate a statement created by a form component.
| xapiEndpoint?: string; | ||
| xapiAuth?: string; | ||
| xapiRegistration?: string; | ||
| xapiStatementWithActor: any[] = []; |
There was a problem hiding this comment.
Should be plural: xapiStatementsWithActor
| tangyForm.addEventListener('TANGY_FORM_UPDATE', async (event) => { | ||
| let response = event.target.store.getState() | ||
| let response = event.target.store.getState(); | ||
| this.xapiStatementWithActor = []; |
There was a problem hiding this comment.
This logic is flawed: the TANGY_FORM_UPDATE event will happen every time the user clicks next. Hence there will be multiple statements for a single question for a single submission which is not desired.
There was a problem hiding this comment.
This logic needs to move to the after-submit event to avoid the problem as above.
| if(this.xapiStatementWithActor && this.xapiStatementWithActor.length > 0 && this.xapiEndpoint && this.xapiAuth) { | ||
| for (let statement of this.xapiStatementWithActor) { | ||
| try { | ||
| await this.xapiService.sendStatement(statement, this.xapiEndpoint, this.xapiAuth); |
There was a problem hiding this comment.
This should make one request to submit statements together (e.g. 1 http request instead of n requests where n is the number of questions).
This is possible as per the xAPI spec
mikedawson
left a comment
mikedawson
left a comment
There was a problem hiding this comment.
As per comments on lines
2 participants
Description
This pull request adds support for the launcher xAPI parameter. The client now reads the xapiStatement provided in the Tangy form input and sends it to the LRS endpoint specified via the xapi parameter.
This enables proper xAPI tracking for form launches and allows learning activity data to be recorded in the configured LRS.
Files Change and Includes