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: account settings not restoring on "Cancel" #4033

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Jul 17, 2024

...if the user attempted to set invalid
credentials by pressing "OK" prior to pressing "Cancel"

TODO:

But there are still cases where invalid credentials are still stored,
see comments.

WofWca added 4 commits July 17, 2024 16:00
So that you have to explicitly ignore its errors
...when invalid credentials are provided,
making it seem like you could actually change password

The bug was introduced in
#3849
136992d

Tested shallowly. Works alright.

Also added error dialog for "instant onboarding" just in case
(did not test it this though).
...if the user attempted to set invalid
credentials by pressing "OK" prior to pressing "Cancel"

But there are still cases where invalid credentials are still stored,
see comments.

This means that there are users for whom the "Password and Account"
dialog shows credentials that are actually invalid.
This commit does not address this,
neither does #4032

This issue is also present on DC Android.
@WofWca WofWca force-pushed the wofwca/cancel-failed-credential-changes branch from 8d7f7e0 to ab195ca Compare July 17, 2024 14:02
@Simon-Laux
Copy link
Member

Isn't that sth that core should do? the variables it actually uses to connect are prefixed with configured_. I think we should rather restore from those.

@WofWca
Copy link
Collaborator Author

WofWca commented Jul 17, 2024

To be honest, I don't know much about the core and its API.
But yeah, it sounds weird that you can commit invalid credentials and then it keeps working on the old credentials while still showing the invalid ones.

I made this merge request with an assumption that the core API doesn't need to be changed.

@Simon-Laux
Copy link
Member

Simon-Laux commented Jul 18, 2024

maybe we could ask core for an api that we can call when the user closes the windows to reset it, or have it behind a reset button in that change credentials dialog.
like the core api should reset the vars to the ones that are actually used?
CC @r10s @link2xt @iequidoo

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