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

Scan - Clicking outside of the allow location modal submits the scan expense #47857

Closed
3 of 6 tasks
lanitochka17 opened this issue Aug 22, 2024 · 29 comments
Closed
3 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 22, 2024

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: 9.0.23-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): nathanmulugetatesting+1091@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Create a workspace
  3. Make sure the location permission is denied on the browser settings
  4. Go to workspace chat and click on plus > Submit expense
  5. Go to scan tab
  6. Upload an attachment to scan and click on submit
  7. After the allow location modal opens up click somewhere else outside the modal

Expected Result:

The Allow location modal closes and the app doesn't submit the scan expense

Actual Result:

When clicking outside the allow location modal the scan expense gets submitted

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6579103_1724318133318.bandicam_2024-08-22_11-48-23-544.mp4

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @ikevin127
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 22, 2024

Edited by proposal-police: This proposal was edited at 2024-08-22 13:08:20 UTC.

Proposal

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

What is the root cause of that problem?

We call onDeny when backdrop is pressed and the expense gets submitted.

const skipLocationPermission = () => {
onDeny(RESULTS.DENIED);
setShowModal(false);
setHasError(false);

onCancel={skipLocationPermission}

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

  • In ConfirmModal, we can accept a new callback onBackdropPress and pass it to Modal.
  • From LocationPermissionModal, we can pass the code below to ConfirmModal.
            onBackdropPress={() => {
                setShowModal(false);
                // setHasError(false);
                resetPermissionFlow();
            }}
  • Same should be done in android file.

NOTE: We shouldn't call setHasError(false); is pressed because it will change the text of the confirm button.

What alternative solutions did you explore? (Optional)


Result

Monosnap.screencast.2024-08-22.18-33-52.mp4

@NJ-2020
Copy link
Contributor

NJ-2020 commented Aug 22, 2024

Edited by proposal-police: This proposal was edited at 2024-08-22 12:54:16 UTC.

Proposal

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

Scan - Clicking outside of the allow location modal submits the scan expense

What is the root cause of that problem?

When we deny or click outside we still create the transaction

onDeny={() => createTransaction(selectedParticipantList, false)}

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

We can add new parameter to LocationPermissionModal like onCancel meaning clicking outside we can set the setStartLocationPermissionFlow to false on cancel and on deny we can still create the transaction

What alternative solutions did you explore? (Optional)

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Added code snippet

@nkdengineer
Copy link
Contributor

Proposal

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

When clicking outside the allow location modal the scan expense gets submitted

What is the root cause of that problem?

When we click outside, onClose is triggered and by default behavior of confirm modal we call onCancel

onClose={onCancel}

And then onDeny is called creates the transaction

const skipLocationPermission = () => {
onDeny(RESULTS.DENIED);
setShowModal(false);
setHasError(false);
};

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

We should separate onClose and onCancel by create a new prop onClose in confirm modal then we will fallback onCancel to onClose event if onClose prop is undefined

onClose={onClose ?? onCancel}

onClose={onCancel}

Then we will pass onClose that will only close the location permission modal

onClose={() => {
    setShowModal(false);
    setHasError(false);
    resetPermissionFlow();
}}

onConfirm={grantLocationPermission}

What alternative solutions did you explore? (Optional)

@Krishna2323
Copy link
Contributor

Proposal Updated

  • Minor updated in code snippet and added a note.

@melvin-bot melvin-bot bot added the Overdue label Aug 26, 2024
Copy link

melvin-bot bot commented Aug 27, 2024

@muttmuure Eep! 4 days overdue now. Issues have feelings too...

@muttmuure muttmuure added the External Added to denote the issue can be worked on by a contributor label Aug 28, 2024
Copy link

melvin-bot bot commented Aug 28, 2024

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

@muttmuure
Copy link
Contributor

Not overdue

@melvin-bot melvin-bot bot added Help Wanted Apply this label when an issue is open to proposals by contributors and removed Overdue labels Aug 28, 2024
Copy link

melvin-bot bot commented Aug 28, 2024

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

@muttmuure muttmuure moved this to Release 2.5: SuiteWorld (Sept 9th) in [#whatsnext] #wave-collect Aug 28, 2024
@muttmuure muttmuure moved this from Release 2.5: SuiteWorld (Sept 9th) to Polish in [#whatsnext] #wave-collect Aug 28, 2024
@ikevin127

This comment was marked as outdated.

@ikevin127
Copy link
Contributor

Thanks everyone for the proposals! 🚀

@Krishna2323's proposal is the most complete and detailed both in terms of RCA and most importantly solution, despite the many edits (which I'm not a fan of). The solution tests well according to the Expected result and takes into account details which the other proposals don't (setHasError(false) and the addition of onBackdropPress prop which is already accepted by the Modal component).

Note: I checked the timeline of edits and nothing is outstanding or "borrowed" from the other proposals (please let me know if you think otherwise). Given this, I think we should go with them for this issue.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 29, 2024

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

@ikevin127
Copy link
Contributor

@neil-marcellini 👋 Friendly bump for assignment based on proposal review if it makes sense to you.

@raza1708
Copy link

raza1708 commented Sep 1, 2024

Awesome

Copy link

melvin-bot bot commented Sep 1, 2024

📣 @raza1708! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@trjExpensify
Copy link
Contributor

Maybe @Julesssss can review that approved proposal to keep this moving, as he has context on this feature?

@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2024
@melvin-bot melvin-bot bot removed the Overdue label Sep 3, 2024
@Julesssss
Copy link
Contributor

Yeah no worries, Neil I can take this one.

The onBackdropPress proposal sounds good to me 👍

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 3, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 5, 2024

This comment was marked as off-topic.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 10, 2024
@ikevin127
Copy link
Contributor

⚠️ Automation failed here -> this should've been paid out yesterday 2024-09-17 according to production deploy checklist Deploy Checklist: New Expensify 2024-09-09 which was shipped to production on 2024-09-10.

cc @muttmuure @Julesssss

@Julesssss
Copy link
Contributor

Yeah, looks ike this can be paid.

@Krishna2323
Copy link
Contributor

@muttmuure, friendly bump

@ikevin127
Copy link
Contributor

Bumped for payments on Slack as well: https://expensify.slack.com/archives/C01GTK53T8Q/p1727218685162429?thread_ts=1724914803.531589&cid=C01GTK53T8Q

@muttmuure muttmuure added Daily KSv2 and removed Weekly KSv2 labels Sep 25, 2024
@muttmuure
Copy link
Contributor

muttmuure commented Sep 25, 2024

Please apply to https://www.upwork.com/jobs/~021838979607891473449

@ikevin127 @Krishna2323

@ikevin127
Copy link
Contributor

@muttmuure Applied as C+ reviewer.

@Krishna2323
Copy link
Contributor

@muttmuure, applied to the offer.

@muttmuure
Copy link
Contributor

Offer sent

@muttmuure
Copy link
Contributor

Paid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
No open projects
Status: Done
Development

No branches or pull requests

10 participants