ndunand / moodle-qtype_matrix

Source code of https://moodle.org/plugins/qtype_matrix
2 stars 10 forks source link

Wrong parameter return #54

Closed drobayo closed 2 years ago

drobayo commented 2 years ago

Hello developers.

We have detected a typo that we have adjusted internally but we would like to see the possibility of correcting it also in the plugin code base for future updates.

This is the line: https://github.com/ndunand/moodle-qtype_matrix/blob/master/qtype_matrix_grading.class.php#L55 Here the variable returned is return $resut[$type] and there the L is missing fix => return $result[$type]

Also we found an error related to this function: https://github.com/ndunand/moodle-qtype_matrix/blob/master/question.php#L340 $x = 1 / 0; our team found this solution and add logic to calculate final grade for this attempt. $grade_value = 0; foreach($responses as $response) { $x = $this->grade_response($response); $grade_value += $x[0]; } return $grade_value;

Let me know if is better to push these changes and thanks for your help!.

ndunand commented 2 years ago

Hello @drobayo ,

Thanks for reporting this.

907bae4 fixes the first issue you mentioned, and will be included in future releases.

As for the second issue, I'm honestly not sure (as I'm not the original developer ... maybe @laurentopprecht or @CamilleTrd coule chime in?) of the expected behavior... Could you elaborate what is the current behavior, and what your suggested fix achieves?

nadavkav commented 2 years ago

Thank you @drobayo ! We are experiencing the same issue, and I can confirm that both your suggested fixes solved it.

btw, it was very weird to see in the question code the following logic: $x = 1 / 0; (divide by zero, and a function with no return value) I wonder if we are all using the proper production level version of qtype_matrix?

ndunand commented 2 years ago

Hi, Looking at this, it looks like this function is only used in qbehaviour_interactivecountback ("Interactive with multiple tries (credit for earlier tries)" mode). To answer the question @nadavkav yes this is the most up to date production version, but apparently this was never implemented. I can only guess that the division by zero part was there by design, to throw an error when called.

ndunand commented 2 years ago

Thanks @drobayo for sharing a fix, this is now integrated via 79f43a3.

nadavkav commented 2 years ago

Thank you @ndunand