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

set the value of cmi.mode #64

Merged
merged 5 commits into from
Feb 23, 2024
Merged

Conversation

rimsha-zaib
Copy link
Contributor

No description provided.

@rimsha-zaib
Copy link
Contributor Author

@ziafazal Could you please review this PR?

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

In general, try to explain in your commit title why you are making this change. Are you fixing a bug? Implementing a new feature? Why is it needed?

In your commit message, you can then explain what you are doing. In this particular case, "what" you are doing is to set the value of cmi.mode. "Why" we need to do this, that I have no idea just by reading the PR...

@rimsha-zaib
Copy link
Contributor Author

In this commit, I addressed the issue related to SCORM attempts not being set correctly. https://github.com/orgs/overhangio/projects/4/views/1?pane=issue&itemId=37732590.
Upon review, it was found that the cmi.mode value was not being set appropriately. This caused discrepancies in SCORM attempts, leading to unexpected behavior.

@regisb
Copy link
Contributor

regisb commented Jan 30, 2024

Great explanation! You should add this to your git commit description.

Copy link
Collaborator

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

@rimsha-zaib overall look. I have a question and a suggestion. Just wondering is there any way we can get hostname on server side e.g self.runtime.hostname instead of sending it from javascript?

}),
async: false,
success: function (response) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we have to remove this success handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to retrieve user mode through self.runtime. Although runtime has a handleurl function , isn't returning correct URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synchronous AJAX call within GetValue does not properly return the response value in success handler, resulting in an empty string. This causes GetValue to be called twice, so the mode is not being set in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not getting it why synchronous AJAX call within GetValue does not properly return the response value?


@XBlock.json_handler
def scorm_get_value(self, data, _suffix):
"""
Here we get only the get_value events that were not filtered by the LMSGetValue js function.
"""
name = data.get("name")
if name in ["cmi.core.lesson_mode", "cmi.mode"]:
mode = self.get_mode(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can merge these two lines to simplify these

Suggested change
mode = self.get_mode(data)
return {"value": self.get_mode(data)}

Copy link
Collaborator

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

👍

@ziafazal ziafazal merged commit 27be75f into overhangio:master Feb 23, 2024
ziafazal pushed a commit that referenced this pull request May 27, 2024
* fix: set the value of cmi.mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants