fix: resolve dupe button on one-expense report #50133

daledah · 2024-10-03T08:16:09Z

Details

Fixed Issues

$ #49793
PROPOSAL: #49793 (comment)

Tests

Precondition:

Control WS, delayed submissions set to Weekly, had admin as approver
User submit an expense report, admin approved
User submit another report with the same amount and merchant as 2.

Steps:

Open the expense report submitted in step 3
Verify that: "Review duplicates" button appears.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Precondition:

Control WS, delayed submissions set to Weekly, enable Approval and have admin as approver
User submit an expense report, admin approved
User submit another report with the same amount and merchant as 2.

Steps:

Open the expense report submitted in step 3
Verify that: "Review duplicates" button appears.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-10-03.at.15.45.13.mov

Android: mWeb Chrome

Screen.Recording.2024-10-03.at.15.46.21.mov

iOS: Native

Screen.Recording.2024-10-03.at.15.49.31.mov

iOS: mWeb Safari

Screen.Recording.2024-10-03.at.15.50.31.mov

MacOS: Chrome / Safari

Screen.Recording.2024-10-03.at.15.12.26.mp4

MacOS: Desktop

Screen.Recording.2024-10-03.at.15.54.12.mov

melvin-bot · 2024-10-03T14:05:16Z

@sobitneupane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

sobitneupane · 2024-10-04T06:11:47Z

@daledah The "Review Duplicate" button remains visible even after one of the duplicate expenses is deleted.

Steps to Reproduce:

Delete one of the duplicate expenses.
Open the remaining expense.
The "Review Duplicate" option is still visible, even though the duplicate error has been removed.

Screen.Recording.2024-10-04.at.11.49.44.mov

sobitneupane

Screenshots/Videos

MacOS: Chrome / Safari

Existing Review Duplicate button in batched expenses:

New Review Duplicate button added by this PR:

iOS: Native

Existing Review Duplicate button in batched expenses:

New Review Duplicate button added by this PR:

iOS: mWeb Safari

Existing Review Duplicate button in batched expenses:

New Review Duplicate button added by this PR:

sobitneupane · 2024-10-04T06:51:41Z

@dannymcclain could you please review the screenshots attached above?

After the change, the Approve button will appear alongside the Review duplicates button, while other buttons, such as the Submit button, will each appear on a separate row.
Green "Review duplicates" button is currently being used for batched expenses. Should we use the same for one-expense report as well?

daledah · 2024-10-04T08:31:49Z

@sobitneupane Can you detail your steps in #50133 (comment)? I can't seem to replicate the exact condition as in your video.

sobitneupane · 2024-10-04T12:19:10Z

@daledah I can no longer reproduce the issue. I’ll follow up if I manage to replicate it again.

dannymcclain · 2024-10-04T14:50:21Z

The screenshots look a bit off to me. I think the Resolve duplicates button should be green if it's the only action available. If there is another, higher-priority action (like approve or submit) then it would go to our default/standard button color.

On mobile, the buttons should be next to each other instead of stacked.

cc'ing @dubielzyk-expensify & @JmillsExpensify for a gut check on all this.

dubielzyk-expensify · 2024-10-07T08:58:20Z

That looks right to me Danny!

pecanoro · 2024-10-08T22:32:37Z

@daledah Are all the buttons consistent with what @dannymcclain posted?

daledah · 2024-10-09T03:13:14Z

@pecanoro I'll update soon.

daledah · 2024-10-09T08:57:49Z

@dannymcclain @pecanoro I updated

dannymcclain · 2024-10-09T13:20:58Z

Updated screenshots/videos are looking good.

On mobile, it looks like there's a second or two when the report is loading that the big green button (Approve for example) is full-width, and then the Resolve duplicates button flashes in and the layout changes—is there any way to get rid of that jump or is it just a consequence of how we're loading the state of the report?

Otherwise layout is looking good to me.

pecanoro · 2024-10-09T17:19:40Z

@sobitneupane Ready for you to do another review!

sobitneupane · 2024-10-10T12:01:34Z

fix: resolve dupe button on one-expense report #50133 (comment)
Padding off for Review Duplicates button when it is the only button (gap between button and the search icon)

When shouldShowIntegrationButton is true, Review Duplicates button takes more space.

sobitneupane · 2024-10-10T12:04:50Z

src/components/MoneyReportHeader.tsx

@@ -247,6 +248,11 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea
        [chatReport?.isOwnPolicyExpenseChat, policy?.harvesting?.enabled],
    );

+    const shouldDuplicateButtonBeSuccess = useMemo(
+        () => isDuplicate && !shouldShowSettlementButton && !shouldShowExportIntegrationButton && !shouldShowSubmitButton && !hasAllPendingRTERViolations,


Things to be considered shouldDuplicateButtonBeSuccess:

Looks like the Submit button is not success in employer's end

- I think ExportIntegrationButton will always have default color.

@sobitneupane I think submit button not being success-themed is expected:

App/src/components/MoneyReportHeader.tsx

Line 244 in 392d56a

// The submit button should be success green colour only if the user is submitter and the policy does not have Scheduled Submit turned on

I believe so. In that case, should we have Review duplicates button success? cc: @dannymcclain

🤔 Hmm, maybe? In this case I'm not sure. I don't think it's wrong to have two regular buttons here. But maybe @JmillsExpensify will have a better informed opinion on if the review duplicates button should be success colored in this situation.

It's not wrong to have two regular buttons. This is the case of delayed submission being enabled, with a submission frequency set.

daledah · 2024-10-13T14:54:59Z

@sobitneupane I updated, please review again.
Regarding #50133 (comment), I believe there are no delays from the UI side, it's all about loading states of the component.

pecanoro · 2024-10-14T14:25:06Z

@sobitneupane All yours! Please try to review it as soon as possible; thank you!! 🙇‍♀️

sobitneupane · 2024-10-15T12:55:11Z

Sorry for the delay. I will try to get it done by EOD.

sobitneupane

Screenshots/Videos

Android: Native

Screen.Recording.2024-10-16.at.17.25.59.mov

iOS: Native

Screen.Recording.2024-10-16.at.17.25.03.mov

iOS: mWeb Safari

Screen.Recording.2024-10-16.at.17.08.11.mov

MacOS: Chrome / Safari

Screen.Recording.2024-10-16.at.17.00.09.mov

MacOS: Desktop

Screen.Recording.2024-10-16.at.17.30.39.mov

sobitneupane · 2024-10-16T12:12:14Z

Thanks for the update @daledah. The changes look good.

However, it seems that the gap between the header and the MoneyReportHeaderStatusBar(for nextStep) has increased due to the changes introduced in this PR.

daledah · 2024-10-16T14:27:06Z

@sobitneupane I updated.

pecanoro · 2024-10-16T15:47:38Z

@sobitneupane If you can check the latest changes and complete the checklist today, it would be great!

sobitneupane · 2024-10-16T15:49:29Z

@daledah There are still some issues with the spacing on small screens. Let's make sure we avoid any unintended UI changes.

daledah · 2024-10-16T16:36:32Z

@sobitneupane I updated.

sobitneupane · 2024-10-17T10:22:42Z

@daledah There is merge conflict. Can you please resolve it.

daledah · 2024-10-17T10:54:38Z

@sobitneupane I updated.

sobitneupane · 2024-10-17T11:04:29Z

@daledah This issue is still reproducible.

daledah · 2024-10-17T11:16:25Z

@sobitneupane I updated.

sobitneupane

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

OSBotify · 2024-10-17T14:37:36Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-10-18T19:04:33Z

🚀 Deployed to staging by https://github.com/pecanoro in version: 9.0.51-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

github-actions · 2024-10-22T04:37:49Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.51-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

github-actions · 2024-10-22T17:20:13Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.51-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

daledah added 3 commits October 3, 2024 15:07

fix: resolve dupe button on one-expense report

1993ffd

fix: ui on narrow layout

140b0fe

fix: styles

d644b39

daledah marked this pull request as ready for review October 3, 2024 14:05

daledah requested a review from a team as a code owner October 3, 2024 14:05

melvin-bot bot requested review from sobitneupane and removed request for a team October 3, 2024 14:05

daledah mentioned this pull request Oct 3, 2024

[HOLD for payment 2024-10-29] [$250] [Dupe Detection] Unable to resolve duplicates on one-expense reporrts #49793

Closed

6 tasks

pecanoro self-requested a review October 3, 2024 14:19

sobitneupane reviewed Oct 4, 2024

View reviewed changes

daledah added 2 commits October 9, 2024 10:38

fix: update ui

30122b7

fix: lint

c230fa5

sobitneupane reviewed Oct 10, 2024

View reviewed changes

daledah added 2 commits October 13, 2024 21:34

Merge branch 'main' into fix/49793

1e08c6b

fix: ui

b4ff475

sobitneupane reviewed Oct 16, 2024

View reviewed changes

fix: remove gap on large screens

5119fd6

fix: ui

d5a7b0f

daledah added 3 commits October 16, 2024 23:49

fix: ui

83c838b

fix: lint

907ab0a

refactor: change logics to more precise

cb4be38

Merge branch 'main' into fix/49793

51ba3f8

fix: remove redundant condition

729c6df

sobitneupane approved these changes Oct 17, 2024

View reviewed changes

pecanoro approved these changes Oct 17, 2024

View reviewed changes

pecanoro merged commit 7317bd6 into Expensify:main Oct 17, 2024
17 checks passed

github-actions bot mentioned this pull request Oct 18, 2024

Deploy Checklist: New Expensify 2024-10-18 #51106

Closed

59 tasks

Shahidullah-Muffakir mentioned this pull request Oct 20, 2024

[HOLD for payment 2024-12-05] Review duplicates - Review duplicates button does not disappear after the expense is paid #51141

Closed

8 tasks

fix: resolve dupe button on one-expense report #50133

fix: resolve dupe button on one-expense report #50133

Conversation

daledah commented Oct 3, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 3, 2024

sobitneupane commented Oct 4, 2024

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots/Videos

sobitneupane commented Oct 4, 2024

daledah commented Oct 4, 2024

sobitneupane commented Oct 4, 2024

dannymcclain commented Oct 4, 2024

dubielzyk-expensify commented Oct 7, 2024

pecanoro commented Oct 8, 2024

daledah commented Oct 9, 2024

daledah commented Oct 9, 2024

dannymcclain commented Oct 9, 2024

pecanoro commented Oct 9, 2024

sobitneupane commented Oct 10, 2024

sobitneupane Oct 10, 2024

Choose a reason for hiding this comment

daledah Oct 10, 2024

Choose a reason for hiding this comment

sobitneupane Oct 11, 2024

Choose a reason for hiding this comment

dannymcclain Oct 11, 2024

Choose a reason for hiding this comment

JmillsExpensify Oct 11, 2024

Choose a reason for hiding this comment

daledah commented Oct 13, 2024

pecanoro commented Oct 14, 2024

sobitneupane commented Oct 15, 2024

sobitneupane left a comment • edited Loading

Choose a reason for hiding this comment

Screenshots/Videos

sobitneupane commented Oct 16, 2024

daledah commented Oct 16, 2024

pecanoro commented Oct 16, 2024

sobitneupane commented Oct 16, 2024

daledah commented Oct 16, 2024

sobitneupane commented Oct 17, 2024

daledah commented Oct 17, 2024

sobitneupane commented Oct 17, 2024

daledah commented Oct 17, 2024

sobitneupane left a comment

Choose a reason for hiding this comment

Reviewer Checklist

OSBotify commented Oct 17, 2024

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

daledah commented Oct 3, 2024 •

edited

Loading

sobitneupane left a comment •

edited

Loading