-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor and provide explanations for cluster access badges on home page, Project detail page, ProjectUser detail page #641
base: develop
Are you sure you want to change the base?
Conversation
…ject detail and ProjectUser detail views
…ined on home page
…adges and info icons
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.
Looks good! I have few future suggestions and couple of clarifying questions.
- If possible, do not exceed the 72 chars limit on commits titles, it makes it easier in understanding changes. If more info in necessary, utilize a new line on the commit message.
- I noticed on the previous PRs that they were merged using a merge commit, which is okay, but also adds some unnecessary messiness when checking the history tree and commits log. So I'd suggest using the rebasing flow instead of the merge flow, as it makes the history super clean and easy to track, reference, etc. (Happy to pair on it if you haven't done this before)
- Ideally, after a review is done, squash the commits into a single commit that references the exact issue being fixed, or multiple commits if seen fit.
- Finally, I have couple of questions to make sure I bring your attention to:
On the PR description, it says:
Explicitly distinguished between two states (where previously only "No Access" existed):
"Denied", for users whose cluster access has been denied by admins.
"No Access", for users who have not requested access.
Example: PIs selected under a new project request are not always automatically granted cluster access, since they often do not need it.
1- This looks different the PR title, as my initial understanding that this is a benign refactor. Is there a newly introduced functionality vs refactor?
2- Given that there were some things that changed how they were computed, is there anything that verifies that there is no regression in the refactor?
Description
**** Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ****
(i)
next to the badges denoting a user's access to a particular project on the cluster.Type of change
How Has This Been Tested?
**** Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration. ****
PR Self Evaluation
Strikethrough things that don’t make sense for your PR.
[ ] I have made corresponding changes to the documentation (if needed)