Skip to content
This repository has been archived by the owner on Feb 4, 2022. It is now read-only.

✨ Confirm dialog when opening URLs #373

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

Komposten
Copy link
Member

@Komposten Komposten commented Aug 24, 2019

This PR adds a confirmation dialog (in the form of a bottom sheet) that pops up when the user taps a URL, see screenshot below:

Confirmation dialog Settings page
image image

The user is given several options:

  1. Cancel -- The URL will not be opened.
  2. Confirm -- The URL is opened.
  3. Trust this domain -- The app will not ask again for this particular domain.
  4. Don't ask again -- The app will never ask again.

Remaining stuff to do:

  • 💬 Show the dialog in all places a user-defined URL can be tapped.
  • ⚙ Add a page in Application Settings to view and delete trusted domains.
  • ♻ Better way of storing the trusted domains (currently using a single string, but should probably use a list).
  • 🐛 Don't save the trust/don't ask status if the dialog is dismissed.
  • 🕵️‍♂️ Improve the domain detection to only include the root domain (e.g. www.images.google.com -> google.com).
  • ❓ Maybe mark some domains as always trusted (e.g. okuna.io). Wait until the Okuna trusted proxies list is available.

@Komposten Komposten marked this pull request as ready for review August 28, 2019 08:05
@Komposten
Copy link
Member Author

@lifenautjoe I've marked this as ready for review. I haven't pushed the license update yet, but all the code should be here.

One question: We talked about clearing the trusted domains list when logging out. This is currently not in since the user preferences in general aren't cleared on log out. Losing your list of trusted domains, and thus having to go through the "trust this domain" thing again for each, when logging out and then back in might be annoying. At the same time I can see why we don't want another person logging in to "inherit" the domain list.

Clearing it on log out isn't difficult so I can add that if we want. Another, slightly more difficult, option would be to prefix the preference namespace with the user's ID (unless that is a security issue for some reason), so that the app remembers each user's settings separately.

Of course, it does not feel very likely that multiple people keep logging in and out on the same phone, so maybe this is a non-issue in the first place?

@Komposten
Copy link
Member Author

Generated localisation files and LICENSE-3RD-PARTY. For the license, the full license text is too long to include in the file itself (IMO). I left a note with a link to the license, but we can of course add the full license to the repo instead if we wish.

Also, my previous comment on this PR still holds so don't forget to read that one.

@duichwer
Copy link
Contributor

duichwer commented Sep 4, 2019

I like the idea of preventing clicking on a suspicious domain, but if i understand i right the preferred domains are stored local on the device?
It's okay for the moment and until now. But I know that the lifetime of my current device will end and I will use okuna on multiple devices. Then I would be perfect if these settings will be synced in the future.

@Komposten
Copy link
Member Author

I like the idea of preventing clicking on a suspicious domain, but if i understand i right the preferred domains are stored local on the device?
It's okay for the moment and until now. But I know that the lifetime of my current device will end and I will use okuna on multiple devices. Then I would be perfect if these settings will be synced in the future.

Yeah, it's all stored locally at the moment. There are two reasons for this:

  1. Massive server changes and re-factorings are coming up, so it's better to wait until after that.
  2. Most settings will be synced eventually, so it will be easier to write the architecture for everything at once.

Copy link
Member

@uiboy uiboy left a comment

Choose a reason for hiding this comment

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

I also noticed that when the popup to ask permission opens up,

If I select, "Never ask again", I cannot tap "Trust this domain" or "Cancel" but if I have selected trust this domain, I can tap never ask again but not cancel.
This should allow tapping anywhere at all times, so people should be able to switch between the two choices and even cancel if they change their minds, this added friction might be annoying.

I also think we should clear the trusted domains list on logout, since its an edge case that people will use multiple accounts on a single device anyway, so if the edge case happens we should err on the safe side.

assets/i18n/en/user.arb Show resolved Hide resolved
@Komposten
Copy link
Member Author

If I select, "Never ask again", I cannot tap "Trust this domain" or "Cancel" but if I have selected trust this domain, I can tap never ask again but not cancel.
This should allow tapping anywhere at all times, so people should be able to switch between the two choices and even cancel if they change their minds, this added friction might be annoying.

I did the "disable Trust this domain when Never ask again is selected" thing to make it consistent with how the filter pages work (select the Connections circle -> all other circle checkboxes are selected and disabled). Can see how it can cause friction here, though. Will change.

I also think we should clear the trusted domains list on logout, since its an edge case that people will use multiple accounts on a single device anyway, so if the edge case happens we should err on the safe side.

Makes sense 👌

Also, the Cancel button will now always be enabled.
@Komposten
Copy link
Member Author

Komposten commented Sep 7, 2019

@uiboy About clearing prefs on exit, I can either clear all user prefs (trusted domains, "never ask again" and comment order) or only the prefs added by this PR (trusted domains and "never ask again").

The former is a simple preferenceService.clear() call in UserService.logout(), the latter would require specialised code (either as preferenceService.clear(onlyUrlPrefs: true) or preferenceService.clearUrlPrefs()) to clear only what we change here.

What would be the preferred option?

Nevermind, just saw that 371-posts-media adds more stuff to prefs. Don't necessarily want to clear all that so I'll just clear the url stuff for now.

@Komposten Komposten requested a review from uiboy September 8, 2019 12:03
@Komposten
Copy link
Member Author

Komposten commented Oct 4, 2019

I just updated this against develop. Doing so I noticed a couple of things that are missing and should be added before this is done:

  • The Trusted Domains list doesn't show a "nothing here" prompt when the list empty (no domains, or nothing matching the search phrase).
  • The Trusted Domains list should probably use "slide right to delete" list items to be consistent with other lists (@lifenautjoe @uiboy, opinion on this? See the screenshot in the PR description for current design of the list).

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

Successfully merging this pull request may close these issues.

3 participants