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.

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

Scorm hashi bugs #7001

Merged
merged 2 commits into from
Jun 9, 2020
Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jun 6, 2020

Summary

Fixes two small issues that I noticed while using hashi after the SCORM updates:

  • When SCORM data is undefined, the SCORM shim will error because it is trying to access an undefined key path
  • An option had an extra space inside it, meaning it would not pass validation

Reviewer guidance

  • Check that non-SCORM html5 apps do not give an error trying to access a property on undefined as currently happens

Contributor Checklist

PR process:

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Testing:

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added this to the pf-mvp milestone Jun 6, 2020
@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

Merging #7001 into pf-mvp will not change coverage.
The diff coverage is 0.00%.

Impacted Files Coverage Δ
kolibri/plugins/html5_viewer/options.py 71.42% <ø> (ø)
packages/hashi/src/SCORM.js 19.13% <0.00%> (ø)

Copy link
Contributor

@kollivier kollivier left a comment

Choose a reason for hiding this comment

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

Good catch, tested and the problem appears to be fixed!

const lessonStatus = this.data.cmi.core.lesson_status;
if (statusProgressMap.hasOwnProperty(lessonStatus)) {
const lessonStatus = getByKeyPath(this.data, 'cmi.core.lesson_status', SCHEMA, self.userData);
if (Object.prototype.hasOwnProperty.call(statusProgressMap, lessonStatus)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious about why this change was made. e.g. how Object.prototype.hasOwnProperty.call differs in behavior from statusProgressMap.hasOwnProperty. Is this to handle the case where lessonStatus is undefined? If so, any reason not to check it for undefined first?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was flagged by our linter which has this rule: https://eslint.org/docs/rules/no-prototype-builtins

I made the same change the last time I merged pf-mvp -> develop

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - I originally made this change on develop, so @indirectlylit's linting fix was caught up in my stash. It seemed simpler to retain this change than to undo it when I reapplied it to pf-mvp.

@kollivier
Copy link
Contributor

Also, since pf-mvp changes have been merged to develop, we need to make sure we apply this fix there as well.

@kollivier kollivier modified the milestones: pf-mvp, 0.14.0 Jun 8, 2020
@indirectlylit indirectlylit merged commit 6a96f92 into learningequality:pf-mvp Jun 9, 2020
@rtibbles rtibbles deleted the SCORM_hashi_bugs branch June 9, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants