search

studentquiz / moodle-mod_studentquiz

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

StudentQuiz: separate out changestate management #476

Closed danghieu1407 closed 8 months ago

danghieu1407 commented 9 months ago

from general delete/edit management #727398

Hi @timhunt, I have eliminated the "edit" capability in changestate.php. Additionally, I have adjusted the condition in renderer.php so that the button for changing the state is rendered only if the user has the capability to edit or possesses the capability "mod/studentquiz:changestate." In the Behat testing, I have excluded the "mod/studentquiz:manage" capability to ensure that users with the "mod/studentquiz:changestate" capability are specifically able to change the state. Could you please review this change? Many thanks.

timhunt commented 8 months ago

Just to note, this patch was extremely difficult to understand, and the explanation above did not help.

To explain properly:

  1. The question_require_capability_on((int)$key, 'edit'); check in changestate.php should never have existed, which is why it was removed.
  2. The change in renderer.php is still wrong. We shoud show the button if, and only if, the user has permission to use the changestate.php script. Therefore, the only permission check the should be done is has_capability('mod/studentquiz:changestate', $this->page->context.
  3. The change in the Behat test does acutally turn it into a valid test for this but fix, but that would be much more obvious if the scenario was renamed to something like "Scenario: A non-editing teacher only needs the changestate capability to approve questions"
  4. It Sucks to live with the CI build failing. That is becuase Moodle PHPDoc Checker does not understand the coding style rules about this. Until that is fixed, please add continue-on-error: true # This step will show errors but will not fail to the Moodle PHPDoc Checker step in .github/workflows/ci.yml. (You can find examples of that in other plugins.)

Once you have solved points 2, 3 and 4 this should be ready to merge.

danghieu1407 commented 8 months ago

Hello @timhunt,

I've updated the code addressing points 2 and 3. Regarding point 4, upon rebasing, I noticed it had already been added by you. I apologize for squashing with the old commit.

The changes are now ready for your review. Could you please take a look? Many thanks