-
Notifications
You must be signed in to change notification settings - Fork 84
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 JISC context extensions to statements. #422
Comments
Would be good to get thoughts from @garemoko and @davidpesce on this if that's okay guys? |
It seems a bit specific to one org. Does it provide value to others? |
@ryansmith94 can you provide any context on what these things are? For ip address, I would definitely want to make recording that optional as there may be GDPR and/or security implications, or at least perceived implications (if it's an event property I guess that means it's already being stored in Moodle). In general, I'm in favor of getting jisc 's fork back and if adding this properties along with a "JISC mode" setting or something is a quick way to do that, I think that's a good idea. |
Hey both, thanks for sharing thoughts on this. I agree David it is specific to one org in the sense that it's JISC, but JISC's fork of the plugin is actually used by many UK institutions so it does provide value to multiple organisations in that sense. Andrew agree with the IP Address, I think the JISC extensions/mode flag is enough to cover that if we note in the setting description that it will record IP addresses and session IDs. Glad you're in favour of getting JISC back into this repository. Can you provide a bit more detail in terms of what you're looking for with the context on what these things are? I don't really know why the vle_mod_id is wrapped within the courseArea extension, but unfortunately it will need to stay like that for backwards compatibility for their institutions. |
Actually I think the only one I can't work out myself is the sess key - I assume that's a key for the session, but is it some Moodle thing that already exists or some custom generated thing? is it a UUID? |
You can pull session info from the global $SESSION variable. |
@garemoko yeah I think it's just a key for the session, I assume JISC is doing some session based analysis. According to their example, it's just a string (i.e. |
Guess I could have just Googled it: https://github.com/moodle/moodle/blob/master/lib/sessionlib.php#L28-L49 Ok, so yes it all looks like potentially useful information. On that basis, I might prefer to just split the ip address off as a separate setting and include the short name and session key all the time. |
Worth noting that there are only 10 billion (10,000,000,000) different Moodle session keys. I'm not sure what the probability of collision is once you get into large data sets, but I don't think we can do much about that in this plugin anyway. |
Hmm okay yeah would be interested in having the session key and short name all the time if you're both happy with that. Yeah not sure on the session keys, I guess that's a Moodle problem rather than a problem for us to get too involved with. |
Just out of curiosity, apparently there's a 50% chance of collision after only 37 thousand session ids. See https://stats.stackexchange.com/questions/391140/in-a-set-of-10-billion-possible-items-if-i-have-x-items-what-is-the-probability?noredirect=1#comment735115_391140 |
The sessions aren't stored long term, they are periodically (I'm not sure on time) deleted. |
I'm not sure the deletion in Moodle makes any difference to the problem because they will remain in the LRS and potentially cause issues in analysis. |
Ahh, good point. I thought for some reason they were looking for a way to go back and cross-reference them. I'd raise it with JISC and see what they say. You can always add something like the date to the session ID to avoid the potential collisions. |
To be fair they might just be trying to cross reference them, I'm making an assumption here too. Yeah adding the date might be ok to avoid collisions. Either way, I'll discuss with JISC. |
Using date might be problematic for sessions with span midnight. The same problem exists if only using month or year in the case of a session spanning midnight on new year's eve |
Yeah that's very true. |
I guess the IP and session ID could be combined, unlikely that the session ID would collide for the same person. The session ID might exist across IPs though if the IP changes over time for a machine. |
Note to self: |
🎉 This issue has been resolved in version 4.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
JISC are using these context extensions to all statements, I'm not sure whether to add them behind a flag or whether they're useful anyway. This is part of an effort to get all users of JISC's fork back to using the core plugin (#50 and #66 will also be needed).
The text was updated successfully, but these errors were encountered: