CTP-1685 Add unfreeze button to lifecycle block #13
Conversation
nbozhkov-ucl
changed the title
|
Also, please check the GHA checks results as well: |
|
Added changes. |
|
Addressed all automatic check issues introduced by my code. Deployed to 43-clc for testing and sign-off. |
| @@ -79,6 +79,11 @@ public function get_content() { | |||
| if (manager::should_show_auto_freezing_preferences($courseid)) { | |||
| $html .= $renderer->fetch_block_content($courseid); | |||
| } | |||
| if (manager::is_course_frozen($courseid)) { | |||
| if (has_capability("moodle/site:managecontextlocks", $context)) { | |||
There was a problem hiding this comment.
this doesn't look like the right permission to be using, should be a course level permission not a site level one
There was a problem hiding this comment.
Shall I create a new course level capability or maybe reuse existing one - https://github.com/ucl-isd/moodle-block_lifecycle/blob/main/db/access.php#L43-L51?
There was a problem hiding this comment.
Seems that unlocking course context always rely on moodle/site:managecontextlocks to unlock course context. Even if I create new capability for the button, user will always need moodle/site:managecontextlock to unfreeze the course context.
There was a problem hiding this comment.
Currently course admin and tutor roles in Moodle have moodle/site:managecontextlocks set to "Not set". If we want them to be able to unfreeze course context (and use the newly created button in the lifecycle block), we will need to update the permissions for these roles and set moodle/site:managecontextlocks set to "Allow". Hope both last comments gives enough context on the reason for using site level permission. Let me know if you need any further research - I will do it in separate Jira story.
There was a problem hiding this comment.
we can't be giving everyone a really critical site level permission with site wide effect. This is fundamentally wrong.
If this permission isn't suitable, then we need to create a new one, and if need be this will have to be processed through an ad-hoc task as a way around the the limitations of that permission check.
@aydevworks may have a second opinion on this.
There was a problem hiding this comment.
P.S. at present this MR doesn't meet the definition of done so no, keep CTP-1685 story open, will have to rollover into next sprint, so be it.
There was a problem hiding this comment.
I agree that site wide permission is not right. However, if it is only given to roles in the course context level would that not limit it just at course level?
I might be wrong but changing current way of working of context freezing should require changes to core moodle code.
I was also thinking about ad-hoc task as an option - it will need more development time/effort.
There was a problem hiding this comment.
By looking at the /admin/lock.php, it looks like it's not very difficult to do the context locking / unlocking.
I think we could try create a new course level permission and do the context unfreezing in our plugin instead of using the /admin/lock.php.
|
also - rebase, squash your commits and ignore these failures which aren't related to your change- #14 (comment) |
nbozhkov-ucl
force-pushed
the
CTP-1685
branch
2 times, most recently
from
0fb109e to
df498ea
Compare
nbozhkov-ucl
force-pushed
the
CTP-1685
branch
from
df498ea to
ac6abb7
Compare
|
Rebased, squashed my commits and deployed to 43-clc for review. |
|
Fixed by #17 |
No description provided.