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

Allows to update permissions without payload #3346

Merged
merged 21 commits into from Jun 24, 2022
Merged

Allows to update permissions without payload #3346

merged 21 commits into from Jun 24, 2022

Conversation

gepd
Copy link
Contributor

@gepd gepd commented Jun 6, 2022

What does this PR do?

Currently, to update the permission of a document, you also have to update some kind of data, or at least pass some kind of data.

This does not work:

await database.updateDocument(listCollection, list, {}, [`user:${user.$id}`]);

So to update permissions we have to pass some data, even if the data is the same as the current one:

await database.updateDocument(listCollection, list, {name: "test"}, [`user:${user.$id}`]);

This change will allow to send an empty payload, but empty payload and read/write permission will keep throwing an error

await database.updateDocument(listCollection, list, {}, [], []);

Test Plan

No new test

Related PRs and Issues

#2850

Have you read the Contributing Guidelines on issues?

yes

@gepd
Copy link
Contributor Author

gepd commented Jun 6, 2022

The failing test doesn't seems to be related with my changes

@TorstenDittmann
Copy link
Contributor

Restarted the tests 👍🏻

I think it would make sense to make the data param optional. Otherwise it would be required but optional 🙂

@gepd
Copy link
Contributor Author

gepd commented Jun 8, 2022

@TorstenDittmann good catch I totally forgot about that, I'm not good at types in PHP, but reading the manual, that should be the way to make it optional

let me know if that is enough 😀

@TorstenDittmann
Copy link
Contributor

Utopia has all those ->param before the callback. If the last argument is true, the parameter of the rest API is marked as optional 😉

@gepd
Copy link
Contributor Author

gepd commented Jun 9, 2022

@TorstenDittmann I think I got it, Is everyting good now?

Co-authored-by: Everly Precia Suresh <77877486+everly-gif@users.noreply.github.com>
app/controllers/api/database.php Outdated Show resolved Hide resolved
app/controllers/api/database.php Show resolved Hide resolved
@gepd
Copy link
Contributor Author

gepd commented Jun 21, 2022

I need help
After checking the test I realized there is something else looking for the document structure

I'm getting error 400 which I believe comes from here: https://github.com/gepd/appwrite/blob/feat-empty-payload/app/controllers/api/database.php#L1943

I'm not sure but, is this related to utopia php?

Seems like here is looking for a specific structure: https://github.com/utopia-php/database/blob/main/src/Database/Database.php#L1169-L1173

@Meldiron @TorstenDittmann

@gepd
Copy link
Contributor Author

gepd commented Jun 23, 2022

I have finally fixed, I have updated the test, I think not deleting the collection was causing problems

@Meldiron @TorstenDittmann

Copy link
Contributor

@Meldiron Meldiron left a comment

Choose a reason for hiding this comment

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

Looks good to me

@TorstenDittmann TorstenDittmann merged commit 0f0654d into appwrite:master Jun 24, 2022
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.

None yet

4 participants