cengage / moodle-ltiservice_gradebookservices

An implementation for Moodle of IMS LTI Gradebook Services
https://docs.moodle.org/dev/LTI_Gradebook_Services
GNU General Public License v3.0
5 stars 2 forks source link

Misleading error message when user is not enrolled #21

Open miguelhughes opened 3 years ago

miguelhughes commented 3 years ago

Hi! My suggestion is on moodle-ltiservice_gradebookservices/classes/local/resources/scores.php this function:


private function get_json_for_post_request($response, $body, $item, $contextid, $typeid) {
    $score = json_decode($body);
    if (empty($score) ||
        !isset($score->userId) ||
        !isset($score->timestamp) ||
        !isset($score->gradingProgress) ||
        !isset($score->activityProgress) ||
        !isset($score->timestamp) ||
        isset($score->timestamp) && !gradebookservices::validate_iso8601_date($score->timestamp) ||
        (isset($score->scoreGiven) && !is_numeric($score->scoreGiven)) ||
        (isset($score->scoreMaximum) && !is_numeric($score->scoreMaximum)) ||
        (!gradebookservices::is_user_gradable_in_course($contextid, $score->userId))
    ) {
        throw new \Exception('Incorrect score received' . $score, 400);
    }
}

I would get the response 'Incorrect score received' which lead me to think that the error was with the value in the "scoreGiven" field. I think it should be a more generic error, such as "invalid value in the score payload", even better if it specified which was the problematic field. My problem was that I hadn't enrolled in the course (is_user_gradable_in_course returned false I guess) which is a completely different problem than "Incorrect score received". I believe the error "user is not enrolled in course" would have been a lot more helpful in my scenario.

I ran into a problem with this and spent a few hours troubleshooting the problem, I thought the issue was on my my code, but it was with a problem on how I'd set up things in moodle itself.

Thanks for writing this plugin!

claudevervoort commented 3 years ago

Thanks Miguel for the feedback. I agree it would gain on clarity. This code is now part of the core moodle code, so the fix would need to go there. I'll see if I can break that down a bit out there.