-
Notifications
You must be signed in to change notification settings - Fork 28
Update: Added a '_shouldRetryConnection' flag to decide if LRS connections should be reattempted (issue 140) #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…tions should be reattempted (issue 140)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new flag, '_shouldRetryConnection', to control whether LRS connection attempts should be retried in various parts of the codebase. Key changes include:
- Simplifying conditional statements in js/XAPIIndex.js and js/CMI5.js.
- Adding retry logic in js/XAPI.js for connection initialization, state sending, and statement sending.
- Implementing a flag-based mechanism to prevent multiple consecutive retry attempts.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| js/XAPIIndex.js | Simplified conditional return statement when the config is disabled. |
| js/XAPI.js | Introduced retry logic for LRS connection, state sending, and statement sending with a new flag. |
| js/CMI5.js | Streamlined the exitCourse method with a one-line return if no returnURL is provided. |
Comments suppressed due to low confidence (2)
js/XAPI.js:1452
- The variable 'statements' is not defined in this scope. Please verify the correct variable or value to pass to 'sendStatementsSync'.
this.sendStatementsSync(statements);
js/XAPI.js:1489
- The variable 'statements' is not defined in this callback scope. Ensure that the correct variable is used or that 'statements' is defined before this call.
this.sendStatementsSync(statements);
|
@oliverfoster Thanks for the valuable feedback. I've made amendments now in line with what you had pointed out. can you please take a look and see if you're happy with those? |
|
Holding off on merge pending a full test of the solution |
|
@oliverfoster We have tested this in our own content now and can see the retries running and being limited to the correct number. If you're happy I will merge |
|
Go for it |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Addresses #140
Update