search

h5p / h5p-interactive-video

MIT License
236 stars 207 forks source link

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

Open benjaminLedel opened 1 year ago

otacke commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.