Fix two scenarios where null values were causing errors

[garemoko]

Description

• Fixes an issue where an error would occur if an assignment grade comment did not exist. Now in this scenario we just don't include the comment in the statement.
• Fixes an issue where for essay type questions, a null response text would be included in the statement. Now this is replaced by an empty string to indicate that the response was an empty box.

Related Issues
None

PR Type
Fix

[garemoko]

This hasn't been tested (other than the automatic tests)

[garemoko]

I'm told that the quiz fix works, but the assignment grade does not and there's still a "assignfeedback_comments not found" error. I find this surprising given that it's inside a try-catch block. I'll need to take another look.

[garemoko]

@ryansmith94 @davidpesce Please take a look at: https://github.com/xAPI-vle/moodle-logstore_xapi/blob/master/src/transformer/repos/MoodleRepository.php#L53

and

moodle-logstore_xapi/src/transformer/events/mod_assign/assignment_graded.php

Lines 34 to 37 in 8fa1eea

	$gradecomment = $repo->read_record('assignfeedback_comments', [
	'assignment' => $grade->assignment,
	'grade' => $grade->id
	])->commenttext;

In the MoodleRespository, is there any reason we need to throw an exception, rather than returning false, if a record is not found? For graded assignments, I don't believe we have any possible way of knowing before making the DB request whether or not there will be a comment entry in the DB.

[ryasmi]

Yeah we throw an error because it's a clearer error than a can't read X of false sort of error when you try to access properties on the returned record when it's not found. In cases where we might expect something to be missing we just need wrap the calls in a try catch.

[ryasmi]

Returning false is somewhat equivalent to returning null... "the billion dollar mistake"

[garemoko]

Wrapping it in a try catch is what I tried but apparently the error is still getting through somehow.

I need to make some time to replicate this on my own Moodle so I can test properly. Or better, perhaps I can set up a test case for this.

[garemoko]

Turns out that this was a namespace issue where the exception was being thrown as \Exception and I was catching \src\transformer\events\mod_assign\Exception. Changing Exception to \Exception fixed the issue.

[garemoko]

@ryansmith94 this is now ready for review

[ryasmi]

Looks good to me @garemoko, think we should add some tests for this to avoid this becoming a problem again?

[garemoko]

@ryansmith94 tests added.

[ryasmi]

Nice, tests look good thanks Andrew 👍

[HT2Bot]

🎉 This PR is included in version 4.2.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀