Skip to content

feat: support permissions from the API #3351

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

Merged
merged 15 commits into from
Oct 21, 2024
Merged

feat: support permissions from the API #3351

merged 15 commits into from
Oct 21, 2024

Conversation

leafty
Copy link
Member

@leafty leafty commented Oct 11, 2024

Fixes #3350.

See also: SwissDataScienceCenter/renku-data-services#454, SwissDataScienceCenter/renku-data-services#453.

/deploy renku-data-services=main renku=release-0.59.0

@leafty leafty temporarily deployed to renku-ci-ui-3351 October 17, 2024 13:22 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-3351.dev.renku.ch

@leafty leafty temporarily deployed to renku-ci-ui-3351 October 17, 2024 14:00 — with GitHub Actions Inactive
@leafty leafty marked this pull request as ready for review October 17, 2024 14:00
@leafty leafty requested a review from a team as a code owner October 17, 2024 14:00
@leafty leafty temporarily deployed to renku-ci-ui-3351 October 17, 2024 14:14 — with GitHub Actions Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Nice work!
Just a generic comment: do we want to show a disabled button for users without permissions, or should we hide the button completely instead? The second option seems the most popular on other similar platforms.

image

If we keep the current approach, I'm wondering if we should at least remove the button for non-logged users.

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

I also reviewed, and everything looks good and worked as I expected in the things I tried in the deployment. 🎉

I will let Lorenzo approve the PR, since he has some additional comments.

@leafty
Copy link
Member Author

leafty commented Oct 18, 2024

Nice work! Just a generic comment: do we want to show a disabled button for users without permissions, or should we hide the button completely instead? The second option seems the most popular on other similar platforms.

image

If we keep the current approach, I'm wondering if we should at least remove the button for non-logged users.

This PR does not change the UI, any such consideration should be for another PR after consultation with the product team.

@lorenzo-cavazzi
Copy link
Member

I'm ok with keeping the approach on this PR. It changes how it looks for anonymous users (at least for a few buttons, didn't check all of them), but that's not a blocker.

@ciyer feel free to approve the PR if you have reviewed it since I didn't check the code. Otherwise, I'll do it when I'm back next Tuesday

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

🎉

@leafty leafty merged commit 2523dec into main Oct 21, 2024
20 of 24 checks passed
@leafty leafty deleted the leafty/permissions-guard branch October 21, 2024 07:34
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

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.

Update AccessGuard and MembershipGuard to use permissions from the backend
4 participants