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

feat: initial 'Remove account' implementation #16260

Merged
merged 5 commits into from
Apr 25, 2024

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Apr 23, 2024

Purpose / Description

Google require that we more prominently display 'Delete Account'

Fixes


  • the path for requesting deletion is not prominently displayed (for example a little process diagram of "1) access this page 2) log in to the account 3) click delete account link" 4) confirm ?
    • note that the destination URL of "remove-account" is not remembered for redirect after login if not authenticated which would be nice to have. So currently steps are "click remove-account link", "log in", "click account", "click remove account", "confirm", account is deleted...
  • ⚠️ the name doesn't match - we are "AnkiDroid Flashcards" on Play Store listing but that is not listed on the page at all. This is difficult. We need to have that name on the Play Store, but the account is an "AnkiWeb" account based on the webpage. Is there a way to include some information on the deletion page that this account is used by all compatible Anki ecosystem apps including Anki Mobile, Anki Desktop, and AnkiDroid Flashcards?
  • Privacy Policy link

Approach

  • A 'remove account' button opens AnkiWeb
  • A user must log in to AnkiWeb if they did not already do so in 'Shared Decks'
    • We perform an internal redirect back to the 'remove account' functionality
  • A user must enter their password
  • A user must type 'remove'

How Has This Been Tested?

API 33 emulator

  • with non-validated account
  • with validated account
Screenshot 2024-04-23 at 19 36 40 Screenshot 2024-04-23 at 19 37 08 Screenshot 2024-04-23 at 19 57 21 Screenshot 2024-04-23 at 20 33 05 Screenshot 2024-04-23 at 20 33 13

Learning (optional, can help others)

Google 🤷‍♂️ ... happily let copycats into the store, still make us jump through hoops

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

display a sample email account
@david-allison david-allison added the Review High Priority Request for high priority review label Apr 23, 2024
Copy link
Contributor

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

@mikehardy
Copy link
Member

As long as the copycat also respects user privacy and we are not guarding intellectual property in the "Anki" name copycats are fine 🤷 - orthogonal issues

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Apr 23, 2024
* onKeyDown is now incorrect
  * Syncing used to be done on this screen
* use `?.also`
* loggedIntoMyAccountView
Required by Google Play

> Such web links must be functional (e.g., load without errors)
> relevant in scope e.g.,
>   * have a pathway for requesting account deletion prominently displayed on the page
>   * and easily accessible
> The name of the app or developer must be known (that is, it must match the name shown in the Google Play store listing)

This change begins this:

* A 'remove account' button opens AnkiWeb
* A user must log in to AnkiWeb if they did not already do so in 'Shared Decks'
* ⚠️ A user must close and reopen the screen
* ⚠️ A user must press the 'remove account' button again
* A user must enter their password
* A user must type 'remove'

 ⚠️ The name of the app is not provided on this screen

Issue 16256
@david-allison david-allison force-pushed the remove-account branch 2 times, most recently from 0668fe5 to 8f0efa7 Compare April 23, 2024 19:16
Use a WebView as we need to control the 'after login' URL

AnkiWeb currently redirects from 'https://ankiweb.net/account/remove-account ->
* https://ankiweb.net/account/login

then to either:

* https://ankiweb.net/account/verify-email
* https://ankiweb.net/decks

Pressing 'back' on this screen closes it

----

Improvement to flow:

```diff
* A 'remove account' button opens AnkiWeb
* A user must log in to AnkiWeb if they did not already do so in 'Shared Decks'
- * ⚠️ A user must close and reopen the screen
- * ⚠️ A user must press the 'remove account' button again
* A user must enter their password
* A user must type 'remove'
```

Issue 16256
@david-allison
Copy link
Member Author

Nits picked, Privacy policy added

@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Apr 23, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Everything looks good, pulled it locally: tested all UI elements etc, working ✅
Fantastic, thank you

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Apr 23, 2024
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Ok, for the code changes.

Is there a way to include some information on the deletion page that this account is used by all compatible Anki ecosystem apps including Anki Mobile, Anki Desktop, and AnkiDroid Flashcards?

Ask Damien very nicely to put a mention about the clients currently using the account data(Anki, Anki Mobile and AnkiDroid Flashcards)?

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Apr 24, 2024
@david-allison
Copy link
Member Author

We have full control over the page as it's in a WebView

But we also need this if the user clicks the delete account link from the Play Store, which isn't under our control

@mikehardy
Copy link
Member

Just trying to push strings through past flakiness... #16267

@mikehardy mikehardy added Blocked by dependency Currently blocked by some other dependent / related change and removed Blocked by dependency Currently blocked by some other dependent / related change labels Apr 25, 2024
@mikehardy mikehardy merged commit a81a828 into ankidroid:main Apr 25, 2024
5 checks passed
@github-actions github-actions bot added this to the 2.18 release milestone Apr 25, 2024
Copy link
Contributor

Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR.

Read more about updating strings on the wiki,

@github-actions github-actions bot removed Review High Priority Request for high priority review Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Apr 25, 2024
@david-allison david-allison deleted the remove-account branch April 28, 2024 01:02
@dae
Copy link
Contributor

dae commented Apr 29, 2024

Ask Damien very nicely to put a mention about the clients currently using the account data(Anki, Anki Mobile and AnkiDroid Flashcards)?

I can do this as a last resort, but I think it's unreasonable for a third-party service to have to do this - have you tried pointing out to them that the service is not under your control? I'm sure Twitter/X's account removal page doesn't list all the third-party clients that access it for example.

@snowtimeglass
Copy link
Contributor

snowtimeglass commented Apr 30, 2024

As a desirable approach on the client (AnkiDroid) side, I suppose that it would be better to use AnkiWeb logo for the "AnkiWeb account" screen rather than AnkiDroid logo (only), to be less misleading.
image

In case that an app has a screen for connecting to Twitter/X, it is natural to use Twitter/X logo in the screen, rather than the logo of the app itself (only).

I would think the same is true for the screen for connecting to AnkiWeb.

@snowtimeglass
Copy link
Contributor

snowtimeglass commented Apr 30, 2024

it would be better to use AnkiWeb logo for the "AnkiWeb account" screen rather than AnkiDroid logo (only), to be less misleading.

That is, for example,

  • Simply replacing the logo

  • Displaying the two logos to visualize the independence of the app (AnkiDroid) and the external service (AnkiWeb) from each other.

    [Reference]

    [image]

    (Anyway, the current logo on the screen may be a bit too big.)

@david-allison
Copy link
Member Author

As a desirable approach on the client (AnkiDroid) side, I suppose that it would be better to use AnkiWeb logo for the "AnkiWeb account" screen rather than AnkiDroid logo (only), to be less misleading.
image

In case that an app has a screen for connecting to Twitter/X, it is natural to use Twitter/X logo in the screen, rather than the logo of the app itself (only).

I think the same would be true for the screen for connecting to AnkiWeb.

Could you move these comments to a separate issue? I agree and they should be discussed, but they'll be lost if added to a closed PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants