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

[HOLD for payment 2023-10-16] [$500] Add payment method popover has wrong position #26070

Closed
1 of 6 tasks
kavimuru opened this issue Aug 28, 2023 · 62 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Aug 28, 2023

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


Action Performed:

  1. On A, request money from B
  2. On B, go to IOU report
  3. On report header, click pay button and then click arrow button
  4. Observe popover positions

Expected Result:

Popover should show below button

Actual Result:

Popover overlaps with button

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.57-2
Reproducible in staging?: y
Reproducible in production?: y
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-08-18.at.1.51.28.PM.mov
Recording.2998.mp4

Expensify/Expensify Issue URL:
Issue reported by: @aimane-chnaif
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692363288571469

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a94242021f694c9e
  • Upwork Job ID: 1697388288553955328
  • Last Price Increase: 2023-09-21
  • Automatic offers:
    • aimane-chnaif | Reviewer | 26993215
    • Nikhil-Vats | Contributor | 26993216
    • aimane-chnaif | Reporter | 26993217
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@StevenKKC
Copy link
Contributor

StevenKKC commented Aug 28, 2023

Proposal

Please state again the problem we are trying to solve in this issue.

Add payment method popover has wrong position.

What is the cause of this issue?

We does not pass anchorAlignment to PopoverMenu in AddPaymentMethodMenu.


<AddPaymentMethodMenu


And after redesign money request report, calculating anchor position in getAnchorPosition does not updated.
getAnchorPosition(domRect) {
if (this.props.popoverPlacement === 'bottom') {
return {
anchorPositionVertical: domRect.top + (domRect.height - 2),
anchorPositionHorizontal: domRect.left + 20,
};
}
return {
anchorPositionVertical: domRect.top - CONST.MODAL.POPOVER_MENU_PADDING,
anchorPositionHorizontal: domRect.left,
};
}

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

First, we should pass anchorAlignment to KYCWall, AddPaymentMethodMenu and PopoverMenu.

  • In SettlementButton.js
    <KYCWall
        ...
+       anchorAlignment={this.props.anchorAlignment}
    >
  • In BaseKYCWall.js
    <AddPaymentMethodMenu
        ...
+       anchorAlignment={this.props.anchorAlignment}
    >
  • In AddPaymentMethodMenu.js
    <PopoverMenu
        ...
+       anchorAlignment={this.props.anchorAlignment}
    >

Second, we should update anchor position calculating in getAnchorPosition function.
We can modify calculating like drop down menu.

    getAnchorPosition(domRect) {
        return {
            anchorPositionVertical: this.props.anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP
                ? domRect.top + domRect.height + CONST.MODAL.POPOVER_MENU_PADDING
                : domRect.top - CONST.MODAL.POPOVER_MENU_PADDING,
            anchorPositionHorizontal: domRect.left + domRect.width,
        };
    }

What alternative solutions have you investigated? (Optional)

None.

@aimane-chnaif
Copy link
Contributor

@alexpensify as a bug reporter, I'd like to review this as C+ as I already have context while reviewing another PR

@alexpensify
Copy link
Contributor

I'm catching up from being OOO, I will test soon.

@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 31, 2023
@melvin-bot melvin-bot bot changed the title Add payment method popover has wrong position [$500] Add payment method popover has wrong position Aug 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01a94242021f694c9e

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Aug 31, 2023

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

@alexpensify
Copy link
Contributor

@aimane-chnaif - let's solve this bug! I've assigned the issue to you.

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 4, 2023

@alexpensify, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Nikhil-Vats
Copy link
Contributor

Nikhil-Vats commented Sep 4, 2023

Proposal

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

Add payment method popover has wrong position in money report header.

What is the root cause of that problem?

MoneyReportHeader uses SettlementButton to show the payment method menu and pay button. SettlementButton uses KYCWall which uses AddPaymentMethodMenu. In AddPaymentMethodMenu, we don't pass anchorAlignment prop to PopoverMenu.

Due to which, the default prop is always used which is to anchor at bottom i.e. show the menu at top of the button -

anchorAlignment: {
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
},

This combined with the wrong calculation below shows wrong result, I added the correct calculation in this PR -

getAnchorPosition(domRect) {
if (this.props.popoverPlacement === 'bottom') {
return {
anchorPositionVertical: domRect.top + (domRect.height - 2),
anchorPositionHorizontal: domRect.left + 20,
};
}
return {
anchorPositionVertical: domRect.top - CONST.MODAL.POPOVER_MENU_PADDING,
anchorPositionHorizontal: domRect.left,
};
}

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

We need to add anchorAlignment prop in AddPaymentMethodMenu, KYCWall and remove the string type prop popoverPlacement which basically tries to implement the same behaviour but does it incorrectly.

In SettlementButton, the prop already exists but is only passed down to ButtonWithDropdownMenu, we can also pass it to KYCWall.

Then, in MoneyReportHeader, we can pass the prop like below only for the code block which shows the button for large screens -

anchorAlignment={{
    horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
    vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP,
}}

This prop will be passed down like MoneyReportHeader -> SettlementButton -> KYCWall -> AddPaymentMethodMenu -> PopoverMenu.

Other than this, we just need to fix the method to calculate the anchorPosition of Popover menu to below to make it same as this correct logic which I fixed in this PR -

getAnchorPosition(domRect) {
    if (this.props.anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP) {
        return {
            anchorPositionVertical: domRect.top + (domRect.height + CONST.MODAL.POPOVER_MENU_PADDING),
            anchorPositionHorizontal: domRect.left + 20,
        };
    }

    return {
        anchorPositionVertical: domRect.top - CONST.MODAL.POPOVER_MENU_PADDING,
        anchorPositionHorizontal: domRect.left,
    };
}

Since we removed popoverPlacement and started passing down anchorAlignment to KYCWall in SettlementButton, we also need to do the following changes to prevent regression -

  1. In BaseWalletPage here instead of passing popoverPlacement="bottom", pass below props to show popover above button in wallet page in settings -
anchorAlignment={{
    horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
    vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
}}
  1. In ReportPreview, we use SettlementButton to show popover above the button but the default props of SettlementButton will show the popover below since we now pass it to KYCWall too, so pass props to show popover above button (anchor point is bottom) in ReportPreview -
anchorAlignment={{
    horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
    vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
}}

What alternative solutions did you explore? (Optional)

If we want to keep using popoverPlacement for controlling the anchorPosition, we can skip adding anchorAlignment in KYCWall component and instead pass the correct anchorAlignment (TOP or BOTTOM for vertical but LEFT for horizontal) based on popoverPlacement to AddPaymentMethodMenu.

Result with all menus behaviour working properly -

Screen.Recording.2023-09-05.at.1.50.33.AM.mov

@alexpensify
Copy link
Contributor

@aimane-chnaif - When you get a chance, can you look at the proposals to identify if one will fix this issue? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2023
@alexpensify
Copy link
Contributor

@aimane-chnaif - any update here?

@melvin-bot
Copy link

melvin-bot bot commented Sep 7, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@aimane-chnaif
Copy link
Contributor

@Nikhil-Vats does your proposal have any meaningful difference from @StevenKKC's proposal?

@aimane-chnaif
Copy link
Contributor

And let's try to find out all popovers throughout the app where similar issue happens.
Can you confirm if #26343 is dupe or different issue?

@Nikhil-Vats
Copy link
Contributor

Yes @aimane-chnaif, I know they appear similar at first but I only added a proposal here as I know the working of PopoverMenu in depth having worked on a similar issue before and their proposal has a couple issues -

Firstly, they haven't even specified what value needs to be passed as prop for anchorAlignment. Secondly, if we simply pass the value there will be regressions as the same component is used in multiple places and popover menu is expected to be open above the button in few cases and below the button in 1 case (header). I have specified it as regression in my proposal here and also shown the behaviour of all the popovers in result.

I wouldn't have added a proposal if the existing proposal solved everything without regression.

@Nikhil-Vats
Copy link
Contributor

Yes @aimane-chnaif, it would solve the related issue as well, that is a dupe -

Screen.Recording.2023-09-07.at.10.01.15.PM.mov

@Nikhil-Vats
Copy link
Contributor

Nikhil-Vats commented Sep 7, 2023

@aimane-chnaif I forgot to specify one of the key difference in my proposal, there is an existing prop popoverPlacement which tries to calculate the position but does it incorrectly and it is part of the root cause, we need to either remove it or we need to change it to enum (mentioned it in my alternate solution section) to fix this issue correctly.

@Pujan92
Copy link
Contributor

Pujan92 commented Sep 7, 2023

Proposal

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

KYCWall ButtonWithDropdownMenu Popovers open in the incorrect places

What is the root cause of that problem?

Default vertical anchorAlignment is bottom which means it won't open below the passed anchor. And we are not passing anchorAlignment values from BaseKYCWall component.

<AddPaymentMethodMenu
isVisible={this.state.shouldShowAddPaymentMenu}
onClose={() => this.setState({shouldShowAddPaymentMenu: false})}
anchorRef={this.anchorRef}
anchorPosition={{
vertical: this.state.anchorPositionVertical,
horizontal: this.state.anchorPositionHorizontal,
}}
shouldShowPaypal={false}

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

  1. We need to make a dynamic prop called popoverPlacement inside SettlementButton and expect it from the parent as the same component we are using to show the popover either top(ReportPreview) or bottom(transfer balance and MoneyReportHeader).

Based on this popoverPlacement value we need to generate anchorAlignment prop and pass it to the AddPaymentMethodMenu

 anchorAlignment={
      this.props.popoverPlacement === 'bottom' ? {
          horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
          vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP, // we assume that popover menu opens below the anchor, anchor is at TOP
      } : {
          horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
          vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
      }
  }

<AddPaymentMethodMenu
isVisible={this.state.shouldShowAddPaymentMenu}
onClose={() => this.setState({shouldShowAddPaymentMenu: false})}
anchorRef={this.anchorRef}
anchorPosition={{
vertical: this.state.anchorPositionVertical,
horizontal: this.state.anchorPositionHorizontal,
}}
shouldShowPaypal={false}

  1. Now we need to correct the anchorPosition for the anchorPositionVertical and that needs to be the bottom of the anchor ref + 8px for some space between anchor and popver.
    anchorPositionVertical: domRect.top + (domRect.height - 2),
anchorPositionVertical: domRect.bottom + CONST.MODAL.POPOVER_MENU_PADDING,
  1. Inside the MenuItem we have different elements and on click of those it won't open the popover below the MenuItem instead it opens below the clicked target(for eg. menuItem text, icons). To fix we need to pass the MenuItem/anchor ref for clickedElementLocation instead of clicked target ref.
const clickedElementLocation = getClickedTargetLocation(this.anchorRef.current);
Screen.Recording.2023-09-08.at.11.41.23.mp4

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

📣 @aimane-chnaif 🎉 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
Copy link

melvin-bot bot commented Oct 3, 2023

📣 @Nikhil-Vats 🎉 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 📖

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@Nikhil-Vats
Copy link
Contributor

I have started working on the PR and it will be ready for review tonight.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 3, 2023
@alexpensify
Copy link
Contributor

Awesome, looks like the PR is ready for review.

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

🎯 ⚡️ Woah @aimane-chnaif / @Nikhil-Vats, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

  • when @Nikhil-Vats got assigned: 2023-10-03 06:11:26 Z
  • when the PR got merged: 2023-10-05 19:01:07 UTC

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 9, 2023
@melvin-bot melvin-bot bot changed the title [$500] Add payment method popover has wrong position [HOLD for payment 2023-10-16] [$500] Add payment method popover has wrong position Oct 9, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

If no regressions arise, payment will be issued on 2023-10-16. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

  • [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR:
  • [@aimane-chnaif] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@aimane-chnaif] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@aimane-chnaif] Determine if we should create a regression test for this bug.
  • [@aimane-chnaif] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 Daily KSv2 labels Oct 16, 2023
@alexpensify
Copy link
Contributor

I will work on the payment process today

@alexpensify
Copy link
Contributor

alexpensify commented Oct 18, 2023

Here is the payment summary:

Upwork Job: https://www.upwork.com/jobs/~01a94242021f694c9e

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment:

  1. This issue was created before August 30, so it will follow the old payment structure. I need to update @Nikhil-Vats' payment and will do so now.

  2. [HOLD for payment 2023-10-16] [$500] Add payment method popover has wrong position #26070 (comment) there is an urgency bonus and no regression. I'm going to factor these into the final payments.

@alexpensify
Copy link
Contributor

@aimane-chnaif - I need you to accept the remaining payment for C+ in Upwork. Also, please complete the checklist so we can close this GH. Thanks!

@alexpensify
Copy link
Contributor

Alright, @aimane-chnaif you've been paid via Upwork for reporting and C+. Please complete the checklist, thanks! I want to close this GH asap.

@aimane-chnaif
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: N/A
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug. This is minor visual bug which doesn't affect user payment flow at all so no need regression test
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. N/A

@alexpensify
Copy link
Contributor

Thanks! Closing this GH since all actions have been completed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants