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

Improvement: Add restriction for site admins to smartmenu(item), resolves #421 . #656

Merged

Conversation

NJahreis
Copy link
Contributor

@NJahreis NJahreis commented May 13, 2024

Hi everyone,

like @VlastimilValaVspj (#421) suggested I would like to have a smartmenu only visible to site admins where I can directly link to some often used settings.
The current pull request code works but changes to the behat tests are not yet implementeted, as I would like to check if any major changes are necessary first before looking into the tests.

@NJahreis NJahreis force-pushed the issue421-smartmenu-restrictadmin branch from 2ff3604 to 3939234 Compare May 14, 2024 08:48
@NJahreis NJahreis changed the title Improvement: Add restriction for site admins to smartmenu(item), resolves #421. Improvement: Add restriction for site admins to smartmenu(item), resolves #421 . May 14, 2024
@NJahreis NJahreis force-pushed the issue421-smartmenu-restrictadmin branch 4 times, most recently from 5e804b7 to deb3551 Compare July 2, 2024 11:20
@NJahreis NJahreis marked this pull request as ready for review July 4, 2024 11:28
@NJahreis
Copy link
Contributor Author

NJahreis commented Jul 4, 2024

I'll fix the typo in boost_union/tests/behat/theme_boost_union_smartmenusettings_menus_rules.feature in the next commit during review to save on running the complete CI run again.

@abias abias force-pushed the main branch 3 times, most recently from 5696e9a to b9fa4ca Compare October 21, 2024 15:33
@abias abias force-pushed the issue421-smartmenu-restrictadmin branch from deb3551 to dc4db86 Compare November 18, 2024 17:20
@abias
Copy link
Member

abias commented Nov 18, 2024

Hi @NJahreis ,

many thanks for working on this nice improvement and welcome to the club of Boost Union contributors.

Your code looked generally spot-on and, during my initial review, I had just rebased the branch onto latest main and had made some minor adjustments to the comments.

But then I thought about the case that a menu (item) should be shown to all user except site admins which is not covered by your improvement yet and which also can only be realized with the other existing restrictions with additional effort.
That's why I went ahead and changed your checkbox to a 3-value select box. I hope you find this as useful as I do.

I will merge the PR as soon as the tests are green again.

Cheers,
Alex

@abias abias force-pushed the issue421-smartmenu-restrictadmin branch 2 times, most recently from 33f9728 to 410d103 Compare November 19, 2024 06:09
@abias abias force-pushed the issue421-smartmenu-restrictadmin branch from 410d103 to d78b2e1 Compare November 19, 2024 06:11
@abias abias merged commit a0e2c3d into moodle-an-hochschulen:main Nov 19, 2024
5 of 6 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