search

studentquiz / moodle-mod_studentquiz

Moodle-Plugin
GNU General Public License v3.0
38 stars 37 forks source link

StudentQuiz: make the SQ working with Moodle 4.3 #467

Closed vuvanhieu143 closed 7 months ago

vuvanhieu143 commented 10 months ago

Make StudentQuiz working with Moodle 4.3

vuvanhieu143 commented 9 months ago

Hi @timhunt ,

I have finished implementing the changes for Moodle 4.3. Here's a summary of the modifications made in this commit:

  1. The removal of M.util.set_user_preference in Moodle 4.3 necessitated the use of UserRepository.
  2. In Moodle 4.3, there were several class changes, including renaming and code modifications. To accommodate this, I have relocated all the classes designed for Moodle 4.2 and below to the 'questionbank/legacy' folder. We can consider removing them in the future if we no longer support Moodle 4.2.
  3. I added conditional statements in unit tests and renderer to ensure the correct view is used based on the version.
  4. I rewrote the Behat scenario, as it was employing too many UI steps.
  5. Due to CSS changes in the question bank for Moodle 4.3, I had to add some extra CSS styling to maintain UI consistency with older versions.

Additionally, there are three failed CI issues:

  1. An unexpected 'array' (T_ARRAY) error occurred in /home/runner/work/moodle-mod_studentquiz/moodle-mod_studentquiz/moodle/mod/studentquiz/classes/condition/studentquiz_condition.php on line 104. This issue is specific to PHP 7.3, and I don't think we need to fix it.
  2. We encountered 8 clones with 642 duplicated lines in 10 files. To address this, we can add the following exclusions:
    • IGNORE_PATHS: 'classes/question/bank/legacy'
    • IGNORE_NAMES: 'studentquiz_condition_pre_43.php' to skip the duplicated code. to the CI workflows if you want.

  3. The CI flagged missing PHP documentation for the following functions:

    • Line 40: custom_completion::get_state
    • Line 66: custom_completion::get_defined_custom_rules
    • Line 74: custom_completion::get_custom_rule_descriptions
    • Line 88: custom_completion::get_sort_order

    However, it appears that these functions are intentionally undocumented, so I think this warning is incorrect.

vuvanhieu143 commented 9 months ago

I forgot to mention, that we did have a new 403 branch, so I have added this to the workflow if you think it is not needed I can remove it.

timhunt commented 8 months ago

I will review this once:

  1. The CI build is green. (See Point 4 of this comment https://github.com/studentquiz/moodle-mod_studentquiz/pull/476#issuecomment-1832193241)
  2. Pull #476 is merged, and this is rebased on it.

It was good to add the extre CI build. However, please set that build to run with PHP 8.2 and ensure that is passing. Thanks.

vuvanhieu143 commented 8 months ago

Hi @timhunt , I have rebased and updated the codes.

So this should be ready for review

timhunt commented 7 months ago

Thanks @vuvanhieu143, there is a lot of good stuff here.

Some thoughts:

  1. Do you realy need to make the duplicate classes in the lagacy namespace in all cases? Are you aware of this trick: https://github.com/moodleou/moodle-quizaccess_honestycheck/blob/main/rule.php. This can be used if you only need a different parent class. (And possibly if you only need a few other small changes.) Can this be used to reduce any of the duplication?
  2. It seems that core_user/repository was added in Moodle 3.9 https://tracker.moodle.org/browse/MDL-68442. Do we really need to keep the M.util.set_user_preference code? (and hte user_preference_allow_ajax_update code)
  3. Why does this code exist? https://github.com/studentquiz/moodle-mod_studentquiz/pull/467/files#diff-ff0a7381d5c58efb0ae104fd7f83a85e6fed2fb1bd9b75ee031dc4cffa439659L42 - if a class is in the classes folder, then we should be loading it using autoloading, and we should not call require_once - of course, this is an existing issue, so if it is not easy to fix here, then we can fix it in a future issue, but if it is easy to do, then please tidy up now.
  4. The layout of code like this https://github.com/studentquiz/moodle-mod_studentquiz/pull/467/files#diff-4c361637e60119277162597debea7ecd4b9b24de02cb726a5e47016c2404dec0R132 is wrong. The comma should be at the end of the previous line. I can see you are keeping consistency with existing code, but it might just be better to fix this now?
  5. For tests/behat/backup.feature - Can you use 'And I upload "mod/quiz/tests/fixtures/moodle_28_quiz.mbz" file to "Files" filemanager' to avoid the XPath? See, for example mod/quiz/tests/behat/backup.feature (Lots of other really nice Behat changes. Thanks.)

So, overall, really good. Do you want to address some or all of the above points before I merge it? Thanks.

vuvanhieu143 commented 7 months ago

Hi @timhunt,

Thank you for your feedback.

1) I was not aware of that technique, so I have applied those changes, and we now have only one duplicated class (which I can't remove since their structures are different. I don't think we should write a common function just for the sake to remove duplication).

2) I can't remove it because setUserPreference is only implemented in version 4.3 (in MDL-62859), and it is not backported to the older version.

3) I have fixed almost everything. It turns out our current namespace is wrong, which is why we can't use autoloading. There is one class left "question_bank_filter.php", which I plan to refactor in the next ticket so we can utilize autoloading.

4) Oh, I missed that. Thank you. I have fixed it.

5) I did read mod/quiz/tests/behat/backup.feature, and they still need to press the button in this step: "And I press 'Manage course backups.' We can't remove the XPath because the button doesn't have any specific class to identify."

Hope all changes make sense.

timhunt commented 7 months ago

Thanks. That all makes sense. Merged.