Skip to content
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

Clearer errors in event DB record no longer found #386

Open
garemoko opened this issue Jan 7, 2019 · 9 comments
Open

Clearer errors in event DB record no longer found #386

garemoko opened this issue Jan 7, 2019 · 9 comments
Labels

Comments

@garemoko
Copy link
Contributor

garemoko commented Jan 7, 2019

Description
Possibly a dupe of #339 (comment)

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.

@ryasmi
Copy link
Member

ryasmi commented Jan 8, 2019

Yeah agree we should have a clearer error for this.

@mlynn-lp
Copy link
Contributor

mlynn-lp commented Jun 1, 2020

@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?

@ryasmi
Copy link
Member

ryasmi commented Jun 2, 2020

@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);

@ryasmi
Copy link
Member

ryasmi commented Jun 2, 2020

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
Copy link
Contributor

mlynn-lp commented Jun 4, 2020

@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.

@ryasmi
Copy link
Member

ryasmi commented Jun 4, 2020

@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.

@mlynn-lp
Copy link
Contributor

mlynn-lp commented Jun 4, 2020

@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.

@ryasmi
Copy link
Member

ryasmi commented Jun 4, 2020

@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.

@mlynn-lp
Copy link
Contributor

mlynn-lp commented Jun 4, 2020

My error - it is a similar issue to #606.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants