[HOLD for payment 2023-06-28] [$1000] Web - Web- Send money - Green payment button is partially visible

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Login with any account
3. Click on FAB button -> Send money -> Change currency to USD -> Enter amount -> Choose the user
4. Click on the settlement button dropdown

Expected Result:

Green payment button is NOT partially visible

Actual Result:

Green payment button is partially visible

Workaround:

Unknown

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.24.4.

Reproducible in staging?: yes

Reproducible in production?: yes

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

Bug6081541_Recording__4834__1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c7865a7582eac48a
• Upwork Job ID: 1666923005452320768
• Last Price Increase: 2023-06-08

[melvin-bot]

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

[melvin-bot]

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

[dostongulmatov]

Proposal

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

Web - Web- Send money - Green payment button is partially visible

What is the root cause of that problem?

There are two causes for this problem

1. In PopoverMenu/index.js, wrong props given to the anchorAlignment for PopoverWithMeasuredContent component
2. X Coordinate of popover menu is being calculated in correctly because it is being calculated when animation started

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

1. We should fix value of anchorAlignment props given to PopoverWithMeasuredContent component in PopoverMenu/index.js component, It might be breaking use of PopoverMenu component in some other places as well
   

   App/src/components/PopoverMenu/index.js

   Lines 66 to 68 in c3b3a53

   	anchorPosition={props.anchorPosition}
   	anchorAlignment={props.anchorOrigin}
   	onClose={props.onClose}

   
   into anchorAlignment={props.anchorAlignment}
2. Here in the
   

   App/src/components/ButtonWithDropdownMenu.js

   Lines 57 to 67 in c3b3a53

   	useEffect(() => {
   	if (!caretButton.current) {
   	return;
   	}
   	caretButton.current.measureInWindow((x, y, w, h) => {
   	setPopoverAnchorPosition({
   	horizontal: x + w,
   	vertical: y + h,
   	});
   	});
   	}, [windowWidth, windowHeight]);

   we should change dependencies of the useEffect because windowWidth and windowHeight is coming from useWindowDimensions hook and it gets rendered when the component is rendered and x coordinate of caretButton is being calculated incorrectly because it is being calculated onAnimation start when the right drawer is not being shown fully yet, so if we add isMenuVisible to the dependency array of useEffect and setPopoverAnchorPosition if isMenuVisible it will work. Here it is the changes

What alternative solutions did you explore? (Optional)

N/A

Result

In works well

Screen.Recording.2023-06-07.at.09.06.45.mov

[michaelhaxhiu]

So to be clear, this is the bug? That little part of the button shouldn't be visible? I see it but damn it's tiny 😢

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[dostongulmatov]

So to be clear, this is the bug? That little part of the button shouldn't be visible? I see it but damn it's tiny 😢

@michaelhaxhiu yes I think so, actually the width of popover and button are equal but popover is positioned incorrectly so when we fix popover should be placed exactly on the top of the button and it will hide the whole button behind it.

[michaelhaxhiu]

I would bet it's a regression from some change we made 😢 but it would be challenging to figure that out. either way, it's labelled external because I agree 👍

[alitoshmatov]

@michaelhaxhiu
As @dostongulmatov pointed out PopoverMenu component is broken and should be fixed asap. The prop name is anchorAlignment in propTypes but component itself is expecting anchorOrigin. Thus creating confusion around this component.

App/src/components/PopoverMenu/index.js

Lines 64 to 68 in c3b3a53

	return (
	<PopoverWithMeasuredContent
	anchorPosition={props.anchorPosition}
	anchorAlignment={props.anchorOrigin}
	onClose={props.onClose}

In every instance of PopoverMenu usage, anchorAlignment is used to pass the values but anchorAlignment is not used.

App/src/components/AvatarWithImagePicker.js

Line 327 in 82ce35f

anchorAlignment={this.props.anchorAlignment}

App/src/pages/home/report/ReportActionCompose.js

Line 1028 in 82ce35f

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

[s77rt]

@dostongulmatov Thanks for the proposal. Your RCA is correct. The proposed solution makes sense but it is not complete. We should indeed pass anchorAlignment correctly but we would also need to update styles accordingly otherwise the popover won't look correct in many other cases e.g. in the edit photo popover ¹.

The setPopoverAnchorPosition fix in ButtonWithDropdownMenu looks good to me but the downside is that we are still rendering the menu initially at the wrong position then correct it (using the useEffect). Do you think we can render it in the correct position at the very first render? I can think of two options but not sure which is the optimal here:

1. Call measureInWindow before setIsMenuVisible.
2. Call measureInWindow only if isMenuVisible is true. This will keep popoverAnchorPosition empty which prevents PopoverMenu from getting rendered.

Also we should avoid measuring the caret button when it's measured already and window height/width didn't change.

Please update your proposal answering the above concerns and feel free to tag me.

---

Fun fact: the anchorAlignment bug was caught previously during a PR review here #18692 (comment) but was not fully fixed by 6fa4cd5#diff-bc86c5432d255b2c866c9d76583c339dd840c3772574ef43cf0ad81665abcf3b. It may be helpful to check the style changes in that PR.

Footnotes

1. https://github.com/Expensify/App/assets/16493223/fabb65b0-6a5b-442f-b5b1-f4e79128c7d7 ↩

[hoangzinh]

^ other option is we can call setIsMenuVisible(true) within measureInWindow to prevent flicker when measureInWindow is not done yet but Popover is displaying 🤔

[dostongulmatov]

@s77rt yes, in order to be able to initially render the popover in a correct position we will need to make mesurePopoverPosition function which is going to look like below

    const measurePopoverPosition = () => {
        caretButton.current.measureInWindow((x, y, w, h) => {
            setPopoverAnchorPosition({
                horizontal: x + w,
                vertical: y + h,
            });
        }); 
    }

and we'll call it inside useEffect only when popoverAnchorPosition !== null so if it is null that means popover is not rendered yet and we do not need to measurePopoverPosition yet

useEffect(() => {
        if (!caretButton.current) {
            return;
        }
        if(popoverAnchorPosition !== null){
            measurePopoverPosition();
        }        

    // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [windowWidth, windowHeight]);

we can call measurePopoverPosition in the onPress of caretButton as well and measurePopoverPosition function should be called only when popoverAnchorPosition === null which is going to prevent from measuring position every time when the caret button is clicked and it is going to avoid measuring the caret button when it's measured already and window height/width didn't change. Here it is how the caret button onPress going to look like:

    onPress={() => {
         if(popoverAnchorPosition === null){
             measurePopoverPosition();
         }
         setIsMenuVisible(true);
    }}

[dostongulmatov]

thank you @hoangzinh for suggestion

[dostongulmatov]

@s77rt regarding anchorAlignment no solution other then checking each PopoverMenu component came to my mind, I see it is being used in 6 places and if we are going to give all of them correct anchorAlignment it should fix the issue, what do you think?

[s77rt]

@dostongulmatov Is your last comment regarding the edit photo misplaced popover? (those may require styles changes) Can you please add more details on the style changes?

[dostongulmatov]

@s77rt okay I will check and will update you about how I am going to handle it

[dostongulmatov]

@s77rt I do not think the popover placement problem require styling changes, I see that its happening because of the value of anchorAlignment is given incorrectly. for example in order to fix the alignment of popover in the profile photo popover we will need to change anchorAlignment value

App/src/pages/settings/Profile/ProfilePage.js

Lines 95 to 97 in 035783a

	anchorPosition={styles.createMenuPositionProfile(props.windowWidth)}
	anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
	size={CONST.AVATAR_SIZE.LARGE}

into anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM}} and it is going to work or we can just remove anchorAlignment line from here and it is going to take default value of anchorAlignment which is also {horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM}.

I see the same problem in Workespace Settings as well. we can fix it with the same approach.

App/src/pages/workspace/WorkspaceSettingsPage.js

Lines 125 to 127 in 035783a

	anchorPosition={styles.createMenuPositionProfile(props.windowWidth)}
	anchorAlignment={{horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT, vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP}}
	isUsingDefaultAvatar={!lodashGet(props.policy, 'avatar', null)}

I will check it in another places as well and let you know.

[dostongulmatov]

@s77rt I have checked most of other places where PopoverMenu is being used as well and I do not see any problem regarding the alignment of the popover menu other then Workspace Settings Page and Profile Page, if you see any problem let me know.

[dostongulmatov]

The PR is ready for review.

cc @arosiclair @s77rt

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @dostongulmatov got assigned: 2023-06-12 14:42:16 Z
• when the PR got merged: 2023-06-15 16:18:15 UTC
• days elapsed: 4

On to the next one 🚀

[melvin-bot]

@arosiclair, @michaelhaxhiu, @s77rt, @dostongulmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[s77rt]

Not overdue. PR is merged and deployed to staging

[melvin-bot]

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

[melvin-bot]

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

• fix send money - green payment button is visible bug #20655

If no regressions arise, payment will be issued on 2023-06-28. 🎊

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

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]

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:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] 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.
• [@michaelhaxhiu] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: Add pay button to money request header #18692
• The offending PR has been commented on: Add pay button to money request header #18692 (comment)
• A discussion in #expensify-bugs has been started: I don't think this applies here, looks like an edge case that could have been caught during testing
• Determine if we should create a regression test for this bug: No

[dostongulmatov]

looks like we can proceed with the payment

[michaelhaxhiu]

Ready to pay this one as soon as you accept the final offer on Upwork. Comment here when you do please!

[s77rt]

@michaelhaxhiu Accepted! Thanks!

[dostongulmatov]

@michaelhaxhiu Accepted, Thank you!

[melvin-bot]

@arosiclair, @michaelhaxhiu, @s77rt, @dostongulmatov Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@arosiclair, @michaelhaxhiu, @s77rt, @dostongulmatov Still overdue 6 days?! Let's take care of this!

[michaelhaxhiu]

all paid