Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StudentQuiz: separate out changestate management #476

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

danghieu1407
Copy link
Contributor

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
Copy link
Member

timhunt commented Nov 29, 2023

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.

from general delete/edit management
@danghieu1407
Copy link
Contributor Author

danghieu1407 commented Nov 30, 2023

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

@timhunt timhunt merged commit b55da1c into studentquiz:main Nov 30, 2023
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants