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

UI: add ACL-checking to turn off exec button #7919

Merged
merged 9 commits into from May 11, 2020
Merged

Conversation

backspace
Copy link
Contributor

This closes #7453. It’s set to merge into #7836 as it builds on its test infrastructure.

Though this doesn’t fully meet the three-repetitions abstraction guideline because client permissions are handled so differently from allocation and job permissions, extracting the pseudo-abstract ability base class seemed worth it to me because of how much shared functionality there was between the allocation and job abilities, and the client ability makes use of some of it too.

I stopped short of also extracting a computed property that searches for the requisite fine-grained string (alloc-exec in this case), despite the sole difference being the string being searched for. I think this would be easier in a true Javascript class context because the subclasses would be able to call a function to generate computed property to search for the particular string. If generalising this seems desirable, I could do it instead with a computed property that’s extracted into its own file.

Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I completely approve of refactoring behavior into an abstract class. I was close to doing it when I wrote the client ability 😅.

Some manner of further reducing the boilerplate to just a function call and a string like you suggest does sound appealing. I wouldn't be opposed to seeing that happen now, but I'm also okay with this subsystem of the UI maturing over time.

@backspace
Copy link
Contributor Author

It’s my hope that it’ll become easier to use inheritance to accomplish that look-for-this-capability calculation once we have “native” classes 🤞😀

@backspace backspace changed the base branch from b-ui/exec-button-presence-assertions to master May 11, 2020 19:26
@backspace backspace merged commit 8c3a210 into master May 11, 2020
@backspace backspace deleted the f-ui/exec-acl branch May 11, 2020 19:43
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: add ACL-checking for exec button
2 participants