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

Fix: Added cmi.objectives _recordObjectives and id existence check (fixes #316) #317

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Apr 14, 2024

fixes #316

Fix

  • Added _recordObjectives config
  • Check for objective existence before resetting id

@oliverfoster oliverfoster self-assigned this Apr 14, 2024
@oliverfoster oliverfoster changed the title Fix: Added cmi.objectives _recordObjectives and id existence check (fixes #261) Fix: Added cmi.objectives _recordObjectives and id existence check (fixes #361) Apr 14, 2024
@oliverfoster oliverfoster changed the title Fix: Added cmi.objectives _recordObjectives and id existence check (fixes #361) Fix: Added cmi.objectives _recordObjectives and id existence check (fixes #316) Apr 14, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@danielghost danielghost left a comment

Choose a reason for hiding this comment

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

cmi.objectives.n.id shouldn't really need to be checked if it already exists, as it is acceptable to set this to the same value each time. The cost of the additional calls required to loop through the records will be negligable, so worth including if this overcomes LMS implementation issues.

Likewise there are some LMSs that haven't implemented objectives correctly where supported, so good idea to be able to explicitly switch this off for such cases.

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.

👍

@joe-allen-89 joe-allen-89 merged commit 71e88df into master Apr 29, 2024
@joe-allen-89 joe-allen-89 deleted the issue/261 branch April 29, 2024 10:45
github-actions bot pushed a commit that referenced this pull request Apr 29, 2024
## [5.9.8](v5.9.7...v5.9.8) (2024-04-29)

### Chore

* Readme updates for broken links, clarity, and linting errors (fixes #312) (#313) ([df90b8d](df90b8d)), closes [#312](#312) [#313](#313)

### Fix

* Added cmi.objectives _recordObjectives and id existence check (fixes #316) (#317) ([71e88df](71e88df)), closes [#316](#316) [#317](#317)

### Upgrade

* Bump ip from 1.1.8 to 1.1.9 (#311) ([1728659](1728659)), closes [#311](#311)
Copy link

🎉 This PR is included in version 5.9.8 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmi.objectives issues
4 participants