h5p / moodle-mod_hvp

H5P Plugin for Moodle
GNU General Public License v3.0
132 stars 171 forks source link

Add a Moodle core event on each recived xAPI statement (coming from the browser) #176

Open nadavkav opened 7 years ago

nadavkav commented 7 years ago

Since Moodle's logstore_xapi plugin is monitoring a set of specific core events, similar to the ones that are sent into the mdl_logstore_standard_log, and send them to an external LRS, H5P module should also be able to send each xAPI statements that is "coming" form the client (browser) via ajax into h5p custom xapi events table, also as a core Moodle event.

nadavkav commented 7 years ago

Suggested fix https://github.com/iucc/h5p-moodle-plugin/commit/4d4fed11509c8be3fb678ffd8f9a15ae98cfb9b9 Which includes a module system setting for enabling this feature

nadavkav commented 7 years ago

Important update https://github.com/iucc/h5p-moodle-plugin/commit/10714e8a983f46f49d151f1867cadc54a6fb75b3

nadavkav commented 7 years ago

If this concept is approved, I will also add a link to an external logstore_xapi github repository with a special PR that include support for this new hvp/classes/events/xapi_statement (which is already working for us, and sending xapi events into an external LRS)

thomasmars commented 7 years ago

This seems reasonable. Could you tell me a bit more about some things:

nadavkav commented 7 years ago

Great! good to read that it make sense :+1:

Hope it's more clear.

thomasmars commented 7 years ago

Thank you, that's some good info. One more thing:

Either way, I think this would be a nice step towards a better integration with Moodle. We would love a pull request for this.

nadavkav commented 7 years ago

Well, I am not entirely sure what should be the right way to go forward implementing this, maybe we should ask it on the Moodle forums (https://moodle.org/mod/forum/view.php?f=33) or the Moodle telegram dev chat (https://telegram.me/moodledev)

Anyways, what I did is change the the Moodle core event's "action" property after the event was fired and already populated with the relevant xAPI statement data, to hold the xAPI verb: https://github.com/iucc/h5p-moodle-plugin/commit/10714e8a983f46f49d151f1867cadc54a6fb75b3#diff-a82923ea8a8f20ccb1d23bc05be41637R46 Which is very useful at the moment, to workaround the dilemma you are face with.

I recommend adding the following xAPI experts: @ryansmith94 and @garemoko and @davidpesce to the discussion (if they can spare some time reviewing this, that would be highly appreciated)

ryasmi commented 7 years ago

I don't have a lot of expertise with h5p or this plugin, but as @thomasmars suggests, I would think it best to have separate events in the Logstore for each of the h5p events and avoid tying the events to the xAPI in the Logstore.

In the xAPI Logstore Plugin it can pick up the separate events and create statements specific to those events there.

This encapsulates the xAPI logic in the xAPI Logstore Plugin and ensures that this plugin's events can be used by other plugins that may or may not be related to the xAPI.

nadavkav commented 7 years ago

Thank you @ryansmith94 for your input! Did you have a look at my implementation of a single H5P xAPI "multi verb" event? https://github.com/iucc/h5p-moodle-plugin/blob/mod_hvp_rtl_support/classes/event/hvp_xapi_statement.php I am already sending all those events into: https://github.com/iucc/moodle-logstore_xapi/commits/h5p_xapi_event_support (Which I haven't PR yet, as it is still under prototype/testing)

Considering the current 35 sub H5P modules and their potential verbs, there are a lot of verb combinations, and it does not seem generic enough solution to add events for each one of them.

garemoko commented 7 years ago

I can see the argument for separating out the events.

A counter argument is that since H5P is already generating xapi statements it's extra complexity to split them out into separate events and then have the logstore plugin catch all the different events.

I wonder if there's a way for the events to go directly to the logstore's xapi emitter. Or maybe it shouldn't be connected to the logstore plugin at all.

I've not really looked into this properly; just throwing out ideas.

ryasmi commented 7 years ago

Did you have a look at my implementation of a single H5P xAPI "multi verb" event?

No I hadn't looked at it when I replied @nadavkav, I've had a quick look through it now, it doesn't seem to be doing much since like @garemoko says this plugin is already generating xAPI statements (which I didn't realise until I read your code and Andrew's comment).

Considering the current 35 sub H5P modules and their potential verbs, there are a lot of verb combinations, and it does not seem generic enough solution to add events for each one of them.

I can see how that might be a pain, however, in that case, you could extend a single class to reduce the work which would maintain flexibility in the xAPI Logstore Plugin and allow the events to be represented independently in the Logstore table.

Or maybe it shouldn't be connected to the logstore plugin at all.

Yeah I'm wondering that too Andrew, whilst there are some benefits to integrating this plugin with the Logstore, that is probably a separate concern. I guess the real benefit is that xAPI Logstore Plugin can use a cron job to emit the statements, but I'm not sure if that would be a benefit here.

garemoko commented 7 years ago

xAPI Logstore Plugin can use a cron job to emit the statements, but I'm not sure if that would be a benefit here

I see that as a real benefit. The question is whether it's better to have the complexity of integrating with the logstore plugin, or duplicate the statement queuing and sending functionality in both plugins. Without having dug into it, I'm not advocating either way.

nadavkav commented 7 years ago

Thank you @garemoko and @ryansmith94 for your feedback! still, not sure how to go forward with this. safely.

We are already using the logstore_xapi (for almost two years now) to send Moodle core event + custom H5P xAPI to an external LRS, and it is also important for us to have those h5p xapi statements send first into Moodle, either mdl_logsotre_standard_log or h5p's internal custom xapi-statements table. so we can not send them directly from the client's browser to an external LRS (if that's what you are thinking about), as we need them for server-side grade and progress calculation (to prevent client side malicious grade manipulation) and also triggering other course learning paths related to xapi statement specific verbs and such.

Indeed, the challenge is that we are already getting xapi statements off the h5p sub plugins, in the first place, and I do not want to lose that data structure and its detailed content when "translating" it into a Moodle standard event. so I just keep everything and later the logstore_xapi expander and translator do not have much to do, but passing almost everything as is to the emitter.

ryasmi commented 7 years ago

You're welcome @nadavkav 👍 I'm not sure either.

Assuming that this plugin generates the statements client-side, I think the client can still manipulate their grade in your current solution.

Okay, so I don't think the mdl_logstore_standard_log is the right place for the events to go since the events are already xAPI statements and it wouldn't really be right to store all those events with under the same eventname in that table. I think integrating this plugin with the mdl_logstore_standard_log is a separate problem (despite being somewhat related).

To improve my understanding, why does this plugin generate xAPI statements? What does it do with them if it doesn't send them to an LRS already?

nadavkav commented 7 years ago

H5P use xAPI as it is a great standard, and since it is a framework not just for Moodle but also for other applications like Wordpress, Drupal, Their own h5p.org cloud service, Canvas, other systems... it sends those xAPI event into the hosting environment. In Moodle's case, into a custom xapi statement ready table, and later display it to the users using a specific reporting UI that is based on that xapi data that was received from the JS client. More info: https://h5p.org/blog/december-2016-xapi-improvements

Since logstore_xapi listen to core Moodle events and since we wish to send those event out into a remote LRS from a single funnel, we extended the H5P plugin to be able to convert the JS xAPI events coming into the Moodle system into a core events, and the logstore_xapi to be able to understand those events. and send them all out to an external LRS. Follow the de-facto route. You might think, that H5P module should send the incoming xAPI events directly into a remote LRS, but as it might seem quicker and shorter route, it could open a potential for data integrity anomalies, and also misconfigurations.

ryasmi commented 7 years ago

Hmmmm okay. It sounds to me like there should be a plugin to load statements from the "xAPI statement ready table" into an LRS. However, that plugin may want to share some code with the xAPI Logstore Plugin. That solution would reduce code duplication (still shares code with the xAPI Logstore Plugin) and reduce data duplication (between the ready table and Logstore table).