search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.87k forks source link

[HOLD for payment 2024-10-17] [$250] Use local Onyx key to throttle location permission prompts #47800

Closed garrettmknight closed 1 week ago

garrettmknight commented 2 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: N/A Reproducible in staging?: N/A Reproducible in production?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/378927 Issue reported by: @Julesssss Slack conversation:

Prerequisite - make sure location services is disabled for New Expensify

Action Performed:

  1. Sign up with a new account
  2. Big + > Submit expense
  3. Scan receipt
  4. Select a recipient
  5. Submit expense
  6. See the location services request > tap Not Now
  7. When the system request comes up, tap 'Don't Allow'
  8. Big + > Submit expense
  9. Scan receipt
  10. Select a recipient

Current Result:

Every time the user creates a money request, we prompt them to enable the location permission.

Requested Result:

Instead of waiting for a backend solution, we can use a local Onyx key/timestamp to prevent the modal from showing. Let's implement logic that adds a one-week delay between each prompt of the location permission.

Workaround:

N/A

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016f65004bf221326c
  • Upwork Job ID: 1826277541188173965
  • Last Price Increase: 2024-08-21
  • Automatic offers:
    • shubham1206agra | Reviewer | 103692549
    • Krishna2323 | Contributor | 103692550
Issue OwnerCurrent Issue Owner: @slafortune
melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @slafortune (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 2 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~016f65004bf221326c

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External)

github-actions[bot] commented 2 months ago

@Krishna2323 Your proposal will be dismissed because you did not follow the proposal template.

Krishna2323 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-21 15:49:46 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Use local Onyx key to throttle location permission prompts

What is the root cause of that problem?

New feat.

What changes do you think we should make in order to solve the problem?

NOTE: Every time we call setStartLocationPermissionFlow, we need to update the ONYX value and timestamp accordingly.

EDIT: Actually, we don't even need the value property, the timestamp will be enough to check the day passed and to determine when was the last time we asked the user about that.

What alternative solutions did you explore? (Optional)



Result

nkdengineer commented 2 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Use local Onyx key to throttle location permission prompts

What is the root cause of that problem?

This is a new feature request

What changes do you think we should make in order to solve the problem?

  1. Create an ONYXKEYS to store the last time we show the permission prompt
  2. Get the value of this key and before we show the location permission prompts in these places here, we will check if the last time the prompt opened is not 7 days old we will not process to show the current location prompt

https://github.com/Expensify/App/blob/f7d8e4850e729493eb8bfb52efa1a49d0d344ded/src/pages/iou/request/step/IOURequestStepScan/index.native.tsx#L269

https://github.com/Expensify/App/blob/f7d8e4850e729493eb8bfb52efa1a49d0d344ded/src/pages/iou/request/step/IOURequestStepConfirmation.tsx#L568-L570

OPTIONAL: Since we can create Scan request in IOURequestStepScan via QAB, we also should add the location prompts in IOURequestStepConfirmation to IOURequestStepScan

  1. Then whenever we open the prompt here or here, update the ONYX value to update the time stamp.

    What alternative solutions did you explore? (Optional)

    NA

slafortune commented 2 months ago

Updated the steps to include submitting the receipt which needs to be done before the location services request populates.

trjExpensify commented 2 months ago

@shubham1206agra what do you think about these proposals?

shubham1206agra commented 2 months ago

@Krishna2323's proposal looks good to me.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 2 months ago

📣 @shubham1206agra 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 months ago

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

Julesssss commented 2 months ago

I was already assigned...

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Julesssss commented 2 months ago

I agree the timestamp is enough here. Lets figure out the rest of the minor details in the PR review.

Julesssss commented 2 months ago

Reassigning Tim as I didn't see his 'ssigned Krishna2323 13 hours ago' action

Krishna2323 commented 2 months ago

Will raise the PR today.

shubham1206agra commented 1 month ago

PR still in draft.

Julesssss commented 1 month ago

Hey all. We have an edge case that is a bit annoying. iOS users who approve our modal, but select 'Allow Once' on the native iOS prompt will continue to be prompted when using the quick action flow. This obviously sucks because it slows down the quick action flow and is annoying.

Currently our choices are to:

We are discussing some alternatives, but it would be good to get some early thoughts on preferred solutions.

trjExpensify commented 1 month ago

Disable the location check for iOS users on the quick flow action (this would include users who did grant permission)

This sounds non-ideal, because the quick action is likely to be used repeatedly as the preferred method to submit expenses to your company over and over again. If I'm understanding you, disabling it would mean the added accuracy upside of location checking would not apply to this core flow.

iOS users who approve our modal, but select 'Allow Once' on the native iOS prompt will continue to be prompted when using the quick action flow.

Won't they also be prompted on every Submit expense attempt as well? That's kinda' the nature of choosing "Allow once" isn't it, or am I missing something on why this only applies to the quick action flow?

Julesssss commented 1 month ago

disabling it would mean the added accuracy upside of location checking would not apply to this core flow.

Yeah, exactly. My thinking is that this is slightly preferrable to the frustrating native location prompt on every single request. Also, when users use the regular flow they will be re-prompted with context -- which may lead to them selecting 'always'.

Won't they also be prompted on every Submit expense attempt as well?

In the submit expense flow our 7-day throttling will prevent them from seeing the prompt every time. Additionally we are able to show our custom prompt ahead of the native-iOS prompt -- which is slightly less annoying as they at least are given context for the prompt.

Both options are not good, and in the meantime we are discussing other potential solutions.

trjExpensify commented 1 month ago

Why aren't we applying the same logic to the QAB as we are Submit expense?

trjExpensify commented 1 month ago

My thinking is that this is slightly preferrable to the frustrating native location prompt on every single request.

That's your own doing though, isn't it. You selected Only once, not Always. If it's annoying you, change it to Always.

Julesssss commented 1 month ago

Fair point 😄

It makes the whole 'provide context around this contextless location permission' feature redundant. If users didn't select 'always' after going through our context flow the first time, I would suggest they are more likely to select 'never' when given apple prompt again. Though as it just occurs for iOS quick action IF users selected not now I suppose this is the better option.

Julesssss commented 1 month ago

I suppose another point is that in order to use the quick action, users will have already seen the prompt. If Tom's happy then I am also. Lets move forward with the known iOS issue. I'll comment on the PR

slafortune commented 1 month ago

not deployed yet

melvin-bot[bot] commented 3 weeks ago

This issue has not been updated in over 15 days. @Julesssss, @slafortune, @shubham1206agra, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Julesssss commented 3 weeks ago

PR has had comments addressed and oday I requested an additional test for the prompt to reshow.

Julesssss commented 3 weeks ago

Merged!

melvin-bot[bot] commented 3 weeks ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 3 weeks ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.47-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-17. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 3 weeks ago

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

slafortune commented 2 weeks ago

So that I can process these payment - @shubham1206agra Please propose regression test steps to ensure the new feature will work correctly on production in further releases.

Julesssss commented 1 week ago

Waiting on regression test

Julesssss commented 1 week ago

We can use the test case I added to the PR QA steps.

slafortune commented 1 week ago

@shubham1206agra looks like the original offer is expired - please accept the new offer I sent here

slafortune commented 1 week ago

@Krishna2323Paid ✅

shubham1206agra commented 1 week ago

@slafortune Accepted

slafortune commented 1 week ago

@shubham1206agra Paid ✅