xAPI-vle
Clearer errors in event DB record no longer found
Description Possibly a dupe of https://github.com/xAPI-vle/moodle-logstore_xapi/issues/339#issuecomment-444581720
Currently when a DB record is not found, we throw quite a long exception message that may not be clear to the end user what's going on.
Issues of missing DB records are quite common in processing historical data and I guess can also happen when using cron tasks especially if there is more time between tasks. It would be nice to have a clear error in this case.
@garemoko @ryansmith94 @gordonmacqueen-lp @Patches- @lzabo I have tested this and it affects the following event when a course is viewed as a guest: events/core/course_viewed.php $user = $repo->read_record_by_id('user', $event->userid); The guest userid may indeed be 1 but Moodle records it in logstore_standard_log as 0. When we call this the exception is thrown.
A simple solution is:
// Get a valid user for guest.
$guest = false;
if ($event->userid == 0) {
$guest = true;
if (isset($CFG->siteguest)) {
$event->userid = $CFG->siteguest;
}
}
$user = $repo->read_record_by_id('user', $event->userid);
// Reset userid so it is the same as in logstore_standard_log.
if ($guest == true) {
$event->userid = 0;
}
This can also occur in user_created events where the relateduserid is used, possibly incorrectly: events/core/user_created.php $user = $repo->read_record_by_id('user', $event->relateduserid);
It seems that when setting the actor variable we need a valid user: 'actor' => utils\get_user($config, $user), Any thoughts on this?
@mlynn-lp yeah a util sounds good 👍 do you think the following code is functionally equivalent to the code above?
$userid = $event->userid == 0 && isset($CFG->siteguest) ? $CFG->siteguest : $event->userid;
$user = $repo->read_record_by_id('user', $userid);
I would recommend setting the $CFG->siteguest on the $config where that is initially set so that you can test this more easily.
@mlynn-lp yeah a util sounds good 👍 do you think the following code is functionally equivalent to the code above?
$userid = $event->userid == 0 && isset($CFG->siteguest) ? $CFG->siteguest : $event->userid; $user = $repo->read_record_by_id('user', $userid);
@ryansmith94 Yes that would work but I'd have to use the ternary further down to reset the userid if the event is from a guest. At that point $userid would not be zero, so I couldn't detect it using that method therefore I would prefer to use a simple boolean variable to indicate if the event is from a guest or not.
@mlynn-lp that code I sent doesn't change the $event->userid so I don't think you would need to reset the userid right? Perhaps I'm missing some context. If you have other code that needs to know if the user is a guest, I suppose you could change the code above the the following.
$guest = $event->userid == 0;
$userid = $guest && isset($CFG->siteguest) ? $CFG->siteguest : $event->userid;
$user = $repo->read_record_by_id('user', $userid);
// Other code that uses $guest.
@ryansmith94 The exception is being thrown because Moodle stores userid 0 in the logstore when in fact the event was from the guest user.
I reset the userid to 0 so that the original userid is stored in xapi_logstore_log (same as it will be stored in logstore_standard_log) but for user retrieval purposes we know in some cases that the action was initiated by the guest user, so I retrieve the guest user details.
I think this may be a duplicate of issue #386.
@mlynn-lp thanks for explaining the exception, I had understood that part.
I understand why you needed to reset the $event->userid to 0 in the code you posted. In the code I posted the $event->userid is not changed, so there's no need to reset it. The code I posted also ensures that when the userid is 0 it fetches the guest user details. I believe our code is functionally equivalent, I've just avoided mutation for the benefits that immutability provides.
You're right, it does seem to be a duplicate of #386.