Adding idnumber to course and activity (course module) extensions

[BrendanHalley]

Hi all!

In Moodle all courses and activities have idnumber fields. This is useful information to have in xAPI statements as it normally ties back to an external system or record. This reference should persist into the xAPI statement as it directly relates to the learning experience.

I'm currently working on this as it is needed for an internal project however would like to push it back to core. I have a couple of questions about what standards to use and if a merge request would even be accepted.

• Is the key https://w3id.org/learning-analytics/learning-management-system/ being adopted for all extensions?
  short-id has the extension name https://w3id.org/learning-analytics/learning-management-system/short-id

• Should this option be behind a toggle flag as short-id is? If this is the standard being set out it a new field group will likely need to be created for optional extra data.

[ryasmi]

Hi @trewq, this sounds good to me, would be happy to accept this into core via a pull request.

Regarding your first question about the extension key to use, I'm not totally sure why @garemoko chose that key in #65, but I think it's probably ok to use https://w3id.org/learning-analytics/learning-management-system/external-id or something similar.

Regarding your second question, @garemoko and @davidpesce may disagree with me, but I think it's probably ok to include this extension if the idnumber is not null. It's possible we should have done the same with the shortname, but the shortname isn't always used as an id.

As a final note, since I believe you're referring to the idnumber on mdl_course_modules and not a mdl_course, when you create your pull request can you please ensure that this idnumber is added to the activity in the statement that refers to the course_module and not the course. I'm sure you would have done that anyway, but I know that the shortname is actually a property on a mdl_course so you may have been tempted to just try adding it in next to that code for a statement 👍

[BrendanHalley]

Hey @ryansmith94, sorry it has taken so long to get back to this.

I have done something very similar to this for some internal requirements. I have added it to both courses and course modules as it's a common field. I have also added it to the xAPI statement even when null as I consider the fact that it's null to also be important information.

Just want to confirm if this should behind a is_enabled_config? Overall I don't agree with limiting what's in an xAPI statement (it should include as much data relating to the current state of the environment and event as possible) however if that's the standard being set I'll go along with it.

I'll should have a pull request for you soon (hopefully this week).

[ryasmi]

Hey @trewq, no worries.

I have added it to both courses and course modules as it's a common field.

Ok no worries, that sounds fine 👍

I have also added it to the xAPI statement even when null as I consider the fact that it's null to also be important information. ... Overall I don't agree with limiting what's in an xAPI statement.

Whilst I agree with that in theory, I know a few users of this plugin that would disagree with that in practice. Most xAPI users are charged for the amount of data they store in their LRS and they'll see a "null" field in every statement that'll be useless to them because they don't use the idnumber, but it'll be racking up cost for them. I'd suggest that we add an option for sending null, but at that point you might as well have an option for sending it all together (whether it's null or not). Therefore in answer to your question of whether this should be behind a is_enabled_config, I think it should unfortunately.

[BrendanHalley]

Hey @ryansmith94,

Pull request submitted.

You're right, I didn't even consider users that are on those sorts of pricing schemes.

[ryasmi]

No worries, yeah it's pretty common, tends to be one of the more expensive costs of running an LRS. Thanks for the pull request, I'll review that shortly.

[HT2Bot]

🎉 This issue has been resolved in version 3.18.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀