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
fix(appwrite): make meta.permissions
overrideable
#5886
Conversation
🦋 Changeset detectedLatest commit: 0391c8c The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Hey @abdelrahman-essawy thank you for the contribution! I think the defaultReadPermissions
and defaultWritePermissions
should respect the previous values:
[
Permission.read(Role.any()),
Permission.write(Role.any()),
]
to avoid any breaking changes due to this PR.
Am I missing something here?
Hey @aliemir , please check out my discussion #5767 (comment) with @BatuhanW on this regard, and tell me what you think. |
Just discussed with @BatuhanW about this again. Here on Default Values - Permissions docs of Appwrite says that if there's no explicit permissions, client SDK will provide permissions to the author. Tested out with our I confirm that this will be a breaking change if default permissions will be empty. Can you make the necessary changes to avoid this breaking change? 🙏 If you can, please let us know. We'll mark this PR to be included in our May release 🚀 We'll review again after you make the changes 🙌 |
7892e9c
to
2f1f62a
Compare
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.
Thank you for the update @abdelrahman-essawy, just left two small comments. Hope you can have time to check those 🙏
defaultReadPermissions?: Permission[]; | ||
defaultWritePermissions?: Permission[]; |
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.
Since we've added the ability to configure permissions through dataProvider
instance. Can we add a section like "Handling Permissions" to our Appwrite docs? https://refine.dev/docs/data/packages/appwrite/#example Adding a small section above "Example" section will be enough I guess.
Can you add this?
you are very welcome, sure thing! |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 0391c8c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
Users couldn't override the default
readPermissions
/writePermissions
values.What is the new behavior?
Users are able to override the default permissions by passing
options.defaultReadPermissions
/options.defaultWritePermissions
OR by passingmeta?.readPermissions
/meta?.writePermissions
.If user passed
options.defaultReadPermissions
ANDmeta?.readPermissions
, then the priority will be as following:meta?.readPermissions
if not? thenoptions.defaultReadPermissions
if not? thenRole.any()
.fixes #5767
Notes for reviewers
This should be compatible with old users who are not passing
meta?.readPermissions
and expecting to getRole.any()
by default.