- 🇺🇸United States rszrama
Thanks, this is great. Definitely needs to be in for the exact use case mentioned.
A couple minor tweaks I'd request before we commit:
- Rename the condition
current_user_role
. - Rename the class
CurrentUserRole
to match (along with the related comments). - Relabel the condition User role.
- Let's introduce a new more general category called Current request.
- I don't think we need the weight; I'd just remove it.
My rationale for the machine name / label mismatch is that I think "Current request" makes sense as a group (think it's better than "Global" or "Context"), and we don't need the redundant "current" in the label ... but it still makes sense to identify in the code.
My only other consideration is that since we're adding this new, we might like the opportunity to build operator selection into it. Right now the configuration element is titled Allowed roles, but we could make this two elements:
- Matching strategy radios element with options User must have a selected role, User must have all selected roles, and User must have none of the selected roles
- Roles checkboxes element with the various roles as options.
- Rename the condition
- First commit to issue fork.
- last update
6 months ago 793 pass - Status changed to Needs review
6 months ago 7:48pm 20 May 2024 - 🇺🇸United States dww
Addressed most of #6, but not the fancy roles operations stuff at the end. Converted to MR, hiding patches.
- last update
6 months ago 793 pass - 🇺🇸United States dww
Implemented the separate 'Matching strategy' radios and corresponding logic changes in
evaluate()
. Also replaced the deprecateduser_role_names()
call with the recommended alternative. Light local testing is working well. Probably wants automated tests, but I don't have time for that now (about to board a flight). - last update
6 months ago 793 pass - last update
6 months ago 793 pass - 🇮🇱Israel jsacksick
Let's just shorten the keys to "any", and "all", no need for the redundant "role" in there.
We also need tests. - last update
6 months ago 793 pass - 🇺🇸United States dww
- Shortened the keys
- Added the missing config schemaY'all can write tests if you need them. I'm way out of time on the project where I need this functionality, and it's working fine for me.
Cheers,
-Derek - First commit to issue fork.
- last update
6 months ago 787 pass, 2 fail - last update
6 months ago 794 pass -
jsacksick →
committed 69d4e8be on 8.x-2.x authored by
dww →
Issue #2965067 by dww, tBKoT, rszrama, jsacksick: Add a condition to...
-
jsacksick →
committed 69d4e8be on 8.x-2.x authored by
dww →
- Status changed to Fixed
6 months ago 3:31pm 24 May 2024 -
jsacksick →
committed 5b466295 on 3.0.x authored by
dww →
Issue #2965067 by dww, tBKoT, rszrama, jsacksick: Add a condition to...
-
jsacksick →
committed 5b466295 on 3.0.x authored by
dww →
Automatically closed - issue fixed for 2 weeks with no activity.