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

Add delete account functionality (fix #1896) #2083

Merged
merged 18 commits into from
Jun 6, 2022
Merged

Conversation

alexbruy
Copy link
Contributor

@alexbruy alexbruy commented May 11, 2022

Add button to the Account page allowing user to remove Mergin Maps account.
зображення
When account removal requested user is informed that all local projects will be removed from the device and that he should manually cancel Apple subscription if there is one. Also user should type his username in the text field before proceeding.
зображення
In case if account removal failed an error message is shown.

Fixes #1896.

@alexbruy alexbruy requested a review from tomasMizera May 11, 2022 11:38
@alexbruy alexbruy force-pushed the delete-account branch 2 times, most recently from 4bdf8a4 to 7b76b1e Compare May 11, 2022 12:17
Copy link
Collaborator

@tomasMizera tomasMizera left a comment

Choose a reason for hiding this comment

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

Thanks Alex 🚀 , here are my notes:

  1. From my point of view, we should first try to delete an account on Mergin and only after this step succeeds continue with the removal of local projects and local auth. The thing is that Mergin server that a user is using might not even support DELETE which would result in simple removal of all downloaded projects.
  2. When user hits Yes in account deletion dialog, there should be a busy indicator running showing that we are doing something on background. It should keep running while wait for server response, delete all local projects and remove auth.
  3. I know that styling of QML dialog might be depressing 😬, especially with buttons. However, could we make it look more like the one below? I suggest to keep the standard buttons and not worry about styling them, so only style the background and text of the popup (e.g. give it some margins from screen edges, center popup on a screen, blur/dim everything around the popup - this should be possible out of the box with Dialog coming from qtquick controls2)

  1. Lastly, I know we had a discussion about which library to use for dialogs, it seems that QT recommends to use QtQuick controls2 variant. Mind the comment here: https://doc.qt.io/qt-5/qml-qtquick-dialogs-dialog.html#details
    image

app/qml/AccountPage.qml Outdated Show resolved Hide resolved
core/merginapi.h Outdated Show resolved Hide resolved
app/qml/AccountPage.qml Outdated Show resolved Hide resolved
app/qml/AccountPage.qml Outdated Show resolved Hide resolved
app/qml/AccountPage.qml Outdated Show resolved Hide resolved
app/qml/ProjectPanel.qml Outdated Show resolved Hide resolved
app/test/testmerginapi.cpp Outdated Show resolved Hide resolved
core/merginapi.cpp Outdated Show resolved Hide resolved
app/qml/AccountPage.qml Outdated Show resolved Hide resolved
app/qml/AccountPage.qml Outdated Show resolved Hide resolved
@alexbruy
Copy link
Contributor Author

@tomasMizera hope you are happy now

@alexbruy alexbruy requested a review from tomasMizera May 12, 2022 06:42
@tomasMizera tomasMizera added the FROZEN do not merge before upcoming release label May 13, 2022
@tomasMizera
Copy link
Collaborator

@alexbruy regarding failing tests, it might be that you need to wait LONG_REPLY instead:

FAIL!  : TestMerginApi::testAccountDelete() 'spy.wait( TestUtils::SHORT_REPLY )' returned FALSE. ()
   Loc: [../input/app/test/testmerginapi.cpp(2491)]

@PeterPetrik PeterPetrik removed the FROZEN do not merge before upcoming release label May 26, 2022
@alexbruy alexbruy closed this Jun 2, 2022
@alexbruy alexbruy reopened this Jun 2, 2022
@alexbruy alexbruy force-pushed the delete-account branch 2 times, most recently from 6457187 to c24f745 Compare June 2, 2022 06:22
@jozef-budac
Copy link

I am not sure it we fulfill the requirement with this change. After deletion of the user, the account is not deleted, it is just deactivated. According apple quidelines:
"It’s insufficient to only provide the ability to temporarily disable or deactivate an account. People should be able to delete the account along with their personal data."

Right now, when I delete account in input, I am not able to register again, as the username exists and when I try to log in with deleted account in input od web app, it says, Account is not activated

@PeterPetrik
Copy link
Contributor

Lets see if they accept it or not,. we can change it after Apple review if needed. I think it is safer to wait 5 days as it is now, so in case of the unwanted removal they can still reactivate the account via support

@PeterPetrik PeterPetrik merged commit df28f52 into master Jun 6, 2022
@PeterPetrik PeterPetrik deleted the delete-account branch June 6, 2022 10:36
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.

Add option to remove account from within the app - APPLE REQUIREMENT - JUNE 30
4 participants