Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Defensive sanity check of assessment data (fixes #178) #179

Merged
merged 3 commits into from
Sep 26, 2022
Merged

Fix: Defensive sanity check of assessment data (fixes #178) #179

merged 3 commits into from
Sep 26, 2022

Conversation

joe-replin
Copy link
Contributor

Fix

  • Checks for a conflict in data to see if assessment questions are in a submitted state while the assessment is still 'In Progress'. Fixes Assessment Data Sanity Check #178

Testing

You wouldn't be able to reproduce in the core assessment. For example, our Adapt course had learners with low broadband connection as well as a distributed assessment & various banks of assessments. SCORM & xAPI was also added to this project so that contributed to slightly longer load times.

  1. Ensure your course is currently SCORM-enabled on an LMS.
  2. Before having another attempt at an assessment, throttle your network speeds with browser devtools
  3. Upon selecting 'Retry Assessment', close the course while the loading screen is present
  4. Even under these circumstances, it may take several times to reproduce.

@joe-replin joe-replin self-assigned this Sep 7, 2022
chris-steele
chris-steele approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@chris-steele chris-steele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'm sure there are other items of data that can benefit from this approach both in this plugin and others if anyone has ideas.

@chris-steele
Copy link
Contributor

I wonder if when these sorts of contradictions are detected we at least log the occurrence in the console so that there is some record of it for debugging purposes.

@joe-replin
Update adapt-assessmentArticleModel.js
65393cb 
Added log for when this discrepancy occurs.
joe-allen-89
joe-allen-89 approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected in latest FW,

@oliverfoster oliverfoster merged commit 052d8c9 into master Sep 26, 2022
@oliverfoster oliverfoster deleted the issue/178 branch September 26, 2022 10:52
github-actions bot pushed a commit that referenced this pull request Sep 26, 2022
@semantic-release-bot
Chore(release): 5.1.5 [skip ci]
ed52587 
## [5.1.5](v5.1.4...v5.1.5) (2022-09-26)

### Fix

* Defensive sanity check of assessment data (fixes #178) (#179) ([052d8c9](052d8c9)), closes [#178](#178) [#179](#179)
@github-actions
Copy link

🎉 This PR is included in version 5.1.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverfoster oliverfoster oliverfoster approved these changes

@chris-steele chris-steele chris-steele approved these changes

@joe-allen-89 joe-allen-89 joe-allen-89 approved these changes

Assignees

@joe-replin joe-replin

Labels
breaking released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assessment Data Sanity Check
4 participants
@joe-replin @chris-steele @oliverfoster @joe-allen-89