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

Add xAPI Events to gain deeper insights into the learners' behaviors #311

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

benjaminLedel
Copy link

No description provided.

@otacke
Copy link
Contributor

otacke commented Nov 22, 2023

Hi!

Before the H5P core team reviews this, you should ensure to follow the H5P coding style guide.

Also, you should not use jQuery. It's going to be removed from H5P core and would need to be supported with adding it to the content type, which is not intended. Also, jQuery doesn't really feel necessary anymore, in particular for things like inArray which can easily be handled by native indexOf or includes.

Moreover, overriding The XAPIEvent H5P.XAPIEvent.allowedXAPIVerbs is not something that you should to do. A) because it feels like bad practice, B) because the resulting xAPI event will not contain a valid verb id.

The xAPI verb id, however, is the smaller issue compared to adding a viewedTime as a property to the xAPI object part og the statement which is against the xAPI specification, so learning record stores should reject it.
XAPI commonly uses ISO 8601 format for time values, by the way.
And are you really using this yourself? You're sending a viewed statement 25 times per second.

Finally, these statements could be useful, but they should probably be triggered by H5P.Video in general and not only by H5P.InteractiveVideo.

But ultimately, the H5P core team (that I am not a member of) will need to decide on your pull request.

@benjaminLedel
Copy link
Author

Hi @otacke, thanks for your review.

Since the H5P.XAPIEvent.allowedXAPIVerbs is located in the player package - so it is hard to add new xAPI verbs. Maybe we need another merge request for H5P Player?

I can adjust the time format, maybe we move the information to the object?

The viewed statement is triggered only once a seconds. And yes, this information would be quite useful.

@otacke
Copy link
Contributor

otacke commented Nov 22, 2023

You can request that, but the H5P core team will need to decide. As I mentioned, you'd generate an xAPI verb id that would not make sense, however. The verb will end up in https://github.com/h5p/h5p-php-library/blob/master/js/h5p-x-api-event.js#L74 where the id is composed, but the ADL profile that is referenced does not contain played or paused - not even all the verbs that the allow list defines, but that's some other story. You would need to compose the xAPI statement yourself and either reference some other xAPI profile by id (e.g. https://profiles.adlnet.gov/profile/90b2c849-d744-4d0c-8bd0-403e7859a35b/concepts) or create your own.

You cannot simply add arbitrary properties to xAPI statements. If you need to, then you will have to use an extension. And those come with their own set of disadvantages, as the LRS will need to know how to handle these or not be able to process that piece of information.

Oh, now I see the guard that you put into timeUpdate (had a look on my smartphone only this morning). Even triggering this once a second is much. What xAPI statements are meant for is sending them to a LRS or write them to some local database at least. Should this really trigger a call to the server every second for every user that watches the video?

As I said, I am not a member of the H5P core team and cannot decide on your pull request. Just pointing out things that I noticed and would point to if I had a say.

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