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

[WEB-143] Adding basic permissions #81

Merged
merged 33 commits into from
Jan 12, 2024

Conversation

COMTOP1
Copy link
Member

@COMTOP1 COMTOP1 commented Jan 11, 2024

This will be the basis of adding permissions, not complete as I will need both WEB-141 and WEB-142 for a complete integration, hence merging to this branch rather than main

Copy link

linear bot commented Jan 11, 2024

WEB-143 No permissions

There is no list of permissions for each role or descriptions

Can't add, edit, delete or list permissions

@COMTOP1 COMTOP1 requested a review from archessmn January 11, 2024 22:00
@COMTOP1 COMTOP1 requested review from markspolakovs and archessmn and removed request for archessmn and markspolakovs January 11, 2024 22:00
@COMTOP1
Copy link
Member Author

COMTOP1 commented Jan 11, 2024

Either @markspolakovs or @archessmn would like a review, evidently only one person can be requested in private repos
Anyone else is welcome to review as well

Copy link
Member

@markspolakovs markspolakovs left a comment

Choose a reason for hiding this comment

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

I've got the same concern about this as I did in web-auth - permissions are something that's only used by code, so there shouldn't be a reason to add a custom one through UI (or even worse, delete one that is used by a role), so I'm not sure how useful it is to have a UI for managing them. In fact I'd consider replacing Permission with a Prisma enum type to guard this (I recall me and @probablybenallen having a discussion about this)

@COMTOP1
Copy link
Member Author

COMTOP1 commented Jan 11, 2024

Ah I see, should I consider all the current permissions in the calendar2023_prod database or different ones?

@markspolakovs
Copy link
Member

They're defined here:

https://github.com/ystv/experimental-hypothetical-new-internal-site-idea/blob/7c9e7d94206b1d6a2727094eb210d656069043fe/lib/auth/permissions.ts#L10-L26

(I can't remember why we used a string rather than an enum type mind you)

@COMTOP1
Copy link
Member Author

COMTOP1 commented Jan 11, 2024

I think I know why, you can't have a period in the enum name

@markspolakovs
Copy link
Member

That wasn't the reason (it was a long discussion), but it might be a reason.

@COMTOP1
Copy link
Member Author

COMTOP1 commented Jan 11, 2024

Ah okay

@COMTOP1
Copy link
Member Author

COMTOP1 commented Jan 11, 2024

Reconverted to using the enums again

@COMTOP1
Copy link
Member Author

COMTOP1 commented Jan 11, 2024

Once I get the roles and users sorted then I can finalise everything, @markspolakovs anything else you think should be changed?

@COMTOP1 COMTOP1 merged commit d32a53b into web-140-there-is-no-current-admin-ui Jan 12, 2024
4 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