-
Notifications
You must be signed in to change notification settings - Fork 60
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
Improvement: Enhance smart menu restrictions for authenticated and guest roles, resolves #571 #640
Improvement: Enhance smart menu restrictions for authenticated and guest roles, resolves #571 #640
Conversation
e68c840
to
4335db3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @prasanna-lmsace ,
many thanks for working on this improvement!
Review
Documentation
- [Y] Commit Message understandable and issue referenced (via resolves keyword)
- [Y] Patch author correct
- [N] CHANGES.md appended
- [-] README.md appended
- [-] Credits appended
- [Y] Appropriate Comment quantity
- [Y] Successful Moodle PHPDoc check
version.php
- [Y] Checked if $plugin->version increment necessary (and increment done if necessary)
- [Y] $plugin->release untouched
lib.php
- [-] Only necessary functions
Languages
- [-] Only english strings
- [-] Necessary magic strings added (e.g. capabilities)
Automated tests
- [Y] New functionality covered
- [Y] No failing steps or scenarios
Mustache templates
- [-] Examle context exists
CSS and styles.css
- [-] Bootstrap styles used if possible
Duplicated code
- [-] Duplicated code is marked properly
Github action
- [Y] All green
Commit history and scope
- [Y] Already single commit
- [Y] Focus on single purpose
- [Y] No surplus files
Additional aspects for Boost Union
- [-] No usage of $theme->settings
- [-] Usage of isset() checks when processing plugin settings in renderer / output code
- [-] Usage of admin_setting_configselect instead of admin_setting_configcheckbox
- [-] Modified mustache templates properly marked and .upstream template in place
Review result
I rebased your branch onto latest master and made some small refinements. I especially restricted the support for the guest role to the "real" guest role (instead of all roles with the guest archetype) and added support for the visitor role (which was not covered yet). I will merge the branch as soon as the tests are green again.
4335db3
to
44111b2
Compare
No description provided.