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

fix: not able to update user prefs #370

Closed
wants to merge 2 commits into from
Closed

fix: not able to update user prefs #370

wants to merge 2 commits into from

Conversation

safwanyp
Copy link
Contributor

@safwanyp safwanyp commented Mar 29, 2023

What does this PR do?

This PR fixes an issue where User Preferences could not be updated in the Appwrite console.

Fixes: #1468

Test Plan

  • Ran npm test
  • Did manual testing for multiple scenarios of adding/deleting user preferences in the console.

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

Yes. I ran the linter, formatter, and checks to ensure my code follows the same standard as the original code.

@vercel
Copy link

vercel bot commented Mar 29, 2023

@safwanyp is attempting to deploy a commit to the appwrite Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Mar 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
console ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 29, 2023 at 3:47PM (UTC)

Comment on lines 16 to 17
} else if (prefs[prefs.length - 1][0] === '' || prefs[prefs.length - 1][1] === '') {
arePrefsDisabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this allows me enter empty keys, not sure this is what should be allowed 🤔

Bildschirm­foto 2023-03-29 um 15 10 12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Didn't acciunt for this. Will update soon.

@safwanyp
Copy link
Contributor Author

@TorstenDittmann seems like it was a simple mistake I made while using OR instead of AND lol. Please have a look and let me know if you spot any more issues!

Copy link
Contributor

@TorstenDittmann TorstenDittmann left a comment

Choose a reason for hiding this comment

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

Bildschirmaufnahme.2023-03-29.um.17.56.22.mov

looks like we do have some weird conditions here.

Now that I think about it, we might wanna allow empty key or empty values. Since this is possible from the API - we should respect that here.

But than we need to be careful, because 2 empty keys will delete one of them

@safwanyp
Copy link
Contributor Author

That's interesting. So we allow as many empty values as needed, but restrict the number of empty keys to 1?

@safwanyp
Copy link
Contributor Author

safwanyp commented Apr 1, 2023

Hey @TorstenDittmann

So we allow as many empty values as needed, but restrict the number of empty keys to 1?

Pinging you for this clarification ^

@stnguyen90
Copy link
Contributor

Hey @TorstenDittmann

So we allow as many empty values as needed, but restrict the number of empty keys to 1?

Pinging you for this clarification ^

Definitely no more than 1 of the same key because you can't have 2 key-value pairs with the same key.

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.

🐛 Bug Report: Appwrite interface, user preferences
3 participants