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

HFP-3825 Add guard against crazy ;-) content types #52

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Conversation

otacke
Copy link
Owner

@otacke otacke commented Nov 24, 2023

When merged in, will add a guard against content types that may fire xAPI relevant for scoring without being attached to a Game Map exercise popup.

@otacke otacke merged commit 62335e8 into master Nov 24, 2023
1 check passed
@otacke otacke deleted the HFP-3825 branch November 24, 2023 12:04
@fnoks
Copy link
Collaborator

fnoks commented Nov 24, 2023

I was taking a quick look at this one, and wondered why you're not using the this.isAttached instead? And also, I see the instance is deleted when isAttached is set to false, so then I don't understand why CP keeps sending events?

@otacke
Copy link
Owner Author

otacke commented Nov 24, 2023

This was merely a hot fix without much thinking. I merely checked what caused the problem and noticed that Course Presentation was sending a "completed" statement when being re-instantiated from previous state.

@otacke
Copy link
Owner Author

otacke commented Nov 24, 2023

@fnoks The problem occurred when the user completed the Course Presentation subcontent and left it on the summary slide. That will be stored in CP's state. When CP is re-stored from the previous state, it will jump to the summary slide and send out an xAPI completed statement which it should not when re-creating (prepare for a pull request).

Since GameMap by then will already have the xAPI tracker installed and react on the completed statement while assuming the exercise is attached, the bug appeared. In fact, using '!this.isAttached' is more clear and serves the same purpose. I changed it just now.

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.

2 participants