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

🐛 Account for String and Valkyrie::ID Permissions #6712

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Feb 20, 2024

tl;dr Given that we sometimes might compare object.id for permissions, we should treat Valkyrie::ID the same as the string.

Woe is us, for checking permissions on a String, what once was an identifier, but which hath been usurped by the more rigid Valkyrie::ID.

But loe, for the old ways still work. Granting permission to objects based on a string identifier. But Valkyrie::ID, one no longer a string, fails.

@samvera/hyrax-code-reviewers

**tl;dr** Given that we sometimes might compare `object.id` for
permissions, we should treat Valkyrie::ID the same as the string.

Woe is us, for checking permissions on a String, what once was an
identifier, but which hath been usurped by the more rigid Valkyrie::ID.

But loe, for the old ways still work.  Granting permission to objects
based on a string identifier.  But Valkyrie::ID, one no longer a string,
fails.
@jeremyf jeremyf force-pushed the treat-valkyrie-id-and-string-same-for-permissions branch from ddb84b9 to 2cde6c1 Compare February 20, 2024 22:00
@dlpierce
Copy link
Contributor

Yes, the String/Valkyrie::ID duality has been a source of endless fun. Thanks!

@dlpierce dlpierce added the notes-bugfix Release Notes: Fixed a bug label Feb 21, 2024
@dlpierce dlpierce merged commit a48c602 into main Feb 21, 2024
6 checks passed
@dlpierce dlpierce deleted the treat-valkyrie-id-and-string-same-for-permissions branch February 21, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-bugfix Release Notes: Fixed a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants