-
Bug
-
Resolution: Fixed
-
Major
-
3.9
-
MOODLE_39_STABLE
-
MOODLE_39_STABLE
-
MDL-68800-lti-gbs-patch-fixes -
This is a followup of MDL-65306 where some points where detected in the final implementation, but was agreed to allow it to land (to save 1 iteration), as far as the 3 following points were not breaking anything. From the original issue:
1) One little detail that I had here annotated is that, here and there the code does checks like this:
if (isset($something) and empty($something) { ...
|
In those cases, the isset() is redundant, empty() works perfectly with "undefined". From the manual:
No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false.
(tiny example).
2) There are various alignments / code wrapping along the whole patch (all the code in fact) that, while they aren't controlled by our code checker, clearly are in violation of the coding style. Code like:
must follow always the wrapping guidelines, that, very basically are: "Always use 4-space indenting, with 4-extra allowed when the very next line in code body may conflict / look confusing". In the docs you'll find some good examples.
3) In restore... the after_restore_lti() method... I just realized that we are executing it for all the records in a given course, not in a given activity. Problem is that the method is executed once for every mod_lti (each one is a task) restored, so we are really running and running over the same stuff N times (the number of lti activities restored). Both the existing loop (that adjusts gradeitemid and resourceid), and the new loop inserting missing records from old backups.
We really should be able to restrict the code to be applied over current activity stuff instead of whole course.
I've been reviewing the problem and I think that it's "safe" (meaning, nothing will become broken because of the repeated executions), just it's not "correct", the repetition itself.
4) Finally, in MDL-65306, some problems when testing backup and restore were detected. So we need to ensure here that:
4a) backup and restore is working ok, both from old backups and current ones.
4b) tangentially related with the above, also upgrade (that applies a similar logic to backup & restore) needs to be checked.
5) Testing instructions of this issue will "replay" all the testing in the original issue, PLUS the points needed to tests 4a and 4b.
- has a non-specific relationship to
-
-
- Closed
-
