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

Issue 564 - change message ( fixed english only ) #574

Closed
wants to merge 23 commits into from

Conversation

aitkeni
Copy link

@aitkeni aitkeni commented Apr 19, 2020

Description

resolves issue 564. the message was confusing so was changed (in the english resourse only ) to the suggested stopped when location services are disabled or stopped at user request

How to test

1 go to settings,
click on stop logging text or icon,
see message appears. it should now read
Location Tracking was disabled
Stopped at user request.
2 go to settings
turn off location services from OS.
click on stop logging text or icon
see message appears. it should now read
Location Tracking was disabled
Stopped.
(any ideas for unit tests)

Still todo other languages (please advise!)
(In another locale the english text will appear until the translation is present I think)

Fixes #564

Copy link
Collaborator

@kenpugsley kenpugsley left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. Summary of thoughts from out of band discussion with @tstirrat on this. This is definitely an improvement, but would like the messages to be a bit more clear and user centric. Right now, I can think of 3 cases:

  1. the user stopped the location service via the phone's settings. We should tell them the consequence of this change (that we cannot determine your exposure history). They know what they just did, so really we want them to know the consequence - and the current message really doesn't do that.
  2. the user stopped the location service in the app. Again, we should tell them the consequence of this change. I would think this would be the same message as case 1
  3. the location service stopped for a reason that wasn't a user action. Honestly I'm not sure if this can be detected or not (or even happen), but if so, then I think the message is a slightly different message.

Also ... as I was thinking about it, these messages can show up later when the app is restarted or comes back to the foreground. The current messages do not really help the user to know how to fix the issue. It would be helpful if you can get this case covered as well.

@tremblerz
Copy link
Contributor

Can you add more verbose message keys and preferable title also?
Something like - "Device location has been disabled. Please enable it in the settings" and "Location access to Safe Paths has been removed. Please enable it in the settings for effective logging" (Or instead we can do the same on-boarding kind of process also from UX perspective)
Ex. - Uber says: "For a better location experience, turn on the device location"

Given that the PR is dedicated towards only changing the message which is same for the both case right now, I would suggest only change text and get it in, and make a ticket for much in depth UI/UX for the events of location being disabled/enabled etc.

@aitkeni
Copy link
Author

aitkeni commented Apr 21, 2020

Makes sense. Leave it with me!

@aitkeni
Copy link
Author

aitkeni commented Apr 21, 2020

Actually One clarification @tremblerz , the proposed change will distinguish between if the location services have been switched off externally or if its the user clicking the stop tracking icon. So what you are suggesting is to change the message when location services have been disabled externally
to
"location_disabled_title": "Device location has been disabled. Please enable it in the settings",
"location_disabled_message": "Location access to Safe Paths has been removed. Please enable it in the settings for effective logging.",

I cant help thinking that the title and message duplicate information, as they are both displayed together at all times. the message can show more info as the font is smaller.
would it not make more sense to have the title convey the current state, and the message to convey the reason for the state and the suggested resolution.
"location_disabled_title": "Device location has been disabled.",
"location_disabled_message": "Location access to Safe Paths has been removed. Please enable it in the settings for effective logging.",

How does that sound?

@alpita-masurkar
Copy link
Collaborator

Actually One clarification @tremblerz , the proposed change will distinguish between if the location services have been switched off externally or if its the user clicking the stop tracking icon. So what you are suggesting is to change the message when location services have been disabled externally
to
"location_disabled_title": "Device location has been disabled. Please enable it in the settings",
"location_disabled_message": "Location access to Safe Paths has been removed. Please enable it in the settings for effective logging.",

I cant help thinking that the title and message duplicate information, as they are both displayed together at all times. the message can show more info as the font is smaller.
would it not make more sense to have the title convey the current state, and the message to convey the reason for the state and the suggested resolution.
"location_disabled_title": "Device location has been disabled.",
"location_disabled_message": "Location access to Safe Paths has been removed. Please enable it in the settings for effective logging.",

How does that sound?

On that note, you also don't need 'has been' in these two strings. The meaning is still conveyed and is shorter and might look better on screen.

"location_disabled_title": "Device location disabled.",
"location_disabled_message": "Location access to Safe Paths removed. Please enable it in the settings for effective logging.",

@aitkeni
Copy link
Author

aitkeni commented Apr 21, 2020

agreed

@alpita-masurkar
Copy link
Collaborator

#564

@summetj
Copy link
Collaborator

summetj commented Apr 23, 2020

Sorry, missed this pull request because it was not linked to the issue #564. Should be merged with #647

@aitkeni aitkeni requested a review from kenpugsley April 24, 2020 21:37
tstirrat
tstirrat previously approved these changes Apr 24, 2020
@tstirrat
Copy link
Contributor

I did a test on this (Pixel 2 emulator), and it gets stuck showing "stopped at user request" even though I actually have location permissions disabled in the OS.

I would expect it to say "Location access to Safe Paths removed. Please enable it in the settings for effective logging." right?

I suspect we need more thorough checks..

Can you show a video of how it should be working?

@tstirrat tstirrat dismissed their stale review April 24, 2020 22:34

Functional test did not match what I expected

@tstirrat tstirrat added Do Not Merge (yet) Hold off on merging! This can be used for various reasons, Work in Progress, outside legal review... question Further information is requested labels Apr 24, 2020
@aitkeni
Copy link
Author

aitkeni commented Apr 25, 2020

Thanks for helping me on this @tstirrat .
heres a vid of the way the new messages are showing.
2020-04-25 00-53-20.mkv.zip
Unfortunately I just noticed that the new suggested text for when location services are disabled was too wordy and spills over uncomfortably onto two lines and the message is truncated.
image
I think I should be shortened again.
Also I could reproduce the issue you are seeing when the loc services are disabled then the ui shows active when bringing the app back into the foreground
2020-04-25 01-10-59.mkv.zip
i dont think this is an issue i have introduced with my change (but i could be wrong)
If I confirm its an existing defect then I will raise a new issue and I can take a look this weekend, but will shorten that message first.
many thx.

@aitkeni
Copy link
Author

aitkeni commented Apr 25, 2020

just confirmed that this defect exists without my changes. in fact when location services disabled it gets a bit messed up. the app can think location tracking is enabled when its not as well as getting stuck at Location tracking disabled ( which is the original message before my changes ) Ill raise an issue for this.

@aitkeni
Copy link
Author

aitkeni commented Apr 26, 2020

issue #685 raised

efalkner and others added 11 commits April 26, 2020 17:07
* enable haitian creole tests

* get the tests running with onboarding 5 changes

* revert accidental commit for config

* remove check for notification as it's not supported

* upload screenshots on failure

* let's save some failed screenshots

* try disabling builds if the only code pushed is for e2e

* disable the check for content on the home screen as it's not always returning the same thing

* comment out the paths filter because of our requirements for PR merge

* remove the paths filter because of our requirements for PR merge
 ### Testing Notes ###
as before

 ### Fixed Issues ###
Resolves Path-Check#564
…ge. Also changed the enabled message to follow the same pattern.
@aitkeni
Copy link
Author

aitkeni commented Apr 27, 2020

apologies for the delay in updating. I have shortened the messages so they fit into the android notification message area. I have also significantly changed the word 'securely' to 'anonymously' in the Enabled message. I think it is more accurate and better for PR (it seems all the press is talking about how contact tracing is bad for your privacy rights) so I thought this would be a better message and would make our app rise above... regarding the issues you saw @tstirrat i have a separate PR coming to fix that issue (issue-685)

@tstirrat
Copy link
Contributor

Superseded by #725

@tstirrat tstirrat closed this Apr 30, 2020
Code reviews automation moved this from In review to Done Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge (yet) Hold off on merging! This can be used for various reasons, Work in Progress, outside legal review... question Further information is requested
Projects
Development

Successfully merging this pull request may close these issues.

Confusing notification text on "stop logging" action