gjbarnard / moodle-format_topcoll

Collapsed Topics course format for Moodle.
http://docs.moodle.org/en/Collapsed_Topics_course_format
GNU General Public License v3.0
35 stars 60 forks source link

Fix error handling for `getElement` return values #131

Closed olivabigyo closed 1 year ago

olivabigyo commented 1 year ago

Moodle's Component.getElement, which is just a wrapper around Element.querySelector (https://developer.mozilla.org/en-US/docs/Web/API/Element/querySelector) never returns undefined (it returns null if nothing's found.) Thus the error handling clauses in content.js didn't trigger properly.

This PR replaces strict comparisons to undefined with basic truthyness checks.

Note: I only tested this with Moodle 4.0 (this is why it is a pull request against MOODLE_400), but is should probably also be applied to master.

Fixes #130

gjb2048 commented 1 year ago

@olivabigyo I'm intrigued by this, however I need to have more evidence / proof of the 'wrapper' assertion, what problem it solves and critically why Moodle has the same issue as you're reporting -> https://github.com/moodle/moodle/blob/master/course/format/amd/src/local/content.js#L613-L672 - thus, have you raised a Moodle Tracker issue for this?

olivabigyo commented 1 year ago

Here you can see the definition of getElement in the BaseComponent class: https://github.com/moodle/moodle/blob/master/lib/amd/src/local/reactive/basecomponent.js#L193-L200 And you are right, the documentation in that class is incorrect, the @returns should say that it returns a DOM element or null.

The core Moodle code might have the same issue that your code, but maybe hard to trigger. Critically, your code has an additional part, that updates the section number: https://github.com/gjb2048/moodle-format_topcoll/blob/852ceffc168da813c9378a4e8c9c67be0a1c9036/amd/src/content.js#L108-L111 There, the itemno variable is initialized from this.getElement, which is null if the layout doesn't have section numbers.

So, to fix #130 it would be enough to fix just that part. Others I just changed for consistency.

All in all, i've replaced strict comparisons to undefined in 4 places:

  1. Line 87. Entry into the function: https://github.com/gjb2048/moodle-format_topcoll/blob/852ceffc168da813c9378a4e8c9c67be0a1c9036/amd/src/content.js#L87-L89 This whole if is not needed, as there is a check at the caller (which you can see is a simple truthyness check): https://github.com/gjb2048/moodle-format_topcoll/blob/852ceffc168da813c9378a4e8c9c67be0a1c9036/amd/src/content.js#L72-L74
  2. Line 104. I don't know if this is needed or not, depends on whether createMethod can return null or not. I haven't checked. https://github.com/gjb2048/moodle-format_topcoll/blob/852ceffc168da813c9378a4e8c9c67be0a1c9036/amd/src/content.js#L103-L107
  3. Line 109. This is the main thing I've described above. This is what fixes #130 https://github.com/gjb2048/moodle-format_topcoll/blob/852ceffc168da813c9378a4e8c9c67be0a1c9036/amd/src/content.js#L108-L111
    1. Line 114. Here undefined is appropriate, as we are talking about indexing into an array with an index that's potentially not present. Again, I just changed it for consistency. https://github.com/gjb2048/moodle-format_topcoll/blob/852ceffc168da813c9378a4e8c9c67be0a1c9036/amd/src/content.js#L113-L117
gjb2048 commented 1 year ago

@olivabigyo Thank you so much for the explanation. I can't comment on the logic for the most part, as I transposed and adapted the code from the Moodle original and 'assumed' that it was correct. My mistake in that respect. Will you be raising a Moodle Tracker issue for this? Though I'll admit I've had a difficult time in the past just having documentation comments updated, even if they are wrong.

gjb2048 commented 1 year ago

@olivabigyo Also, would you like a credit in the Changes.md file?

gjb2048 commented 1 year ago

@olivabigyo Allowing merge despite CI result - looks like Code Checker is getting stricter again!