Cleanup completing a split bill by youssef-lr · Pull Request #29568 · Expensify/App

youssef-lr · 2023-10-13T13:58:53Z

Details

Fixed Issues

Polish of
$ https://github.com/Expensify/Expensify/issues/322303

Tests

P2P

Start a split bill by using Global Create => Scan => Tap 'split' on a participant
Confirm the split
Tap on the split action to edit it while the fields are still empty
Resize the page and make it vertically smaller, make sure the 'Split' button remains bottom docked, and the page is scrollable.
Click 'Split'
Make sure the last message of the report says "Receipt scan in progress"

Workspace

Sign up as a teacher using the Save The World menu, note down the principal email
Split a receipt with the workspace chat, but don't edit it.
Log into the principal's account, make sure the policy expense chat says "No activity yet', and not 'Split $0.0'

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

P2P

Start a split bill by using Global Create => Scan => Tap 'split' on a participant
Confirm the split
Tap on the split action to edit it while the fields are still empty
Resize the page and make it vertically smaller, make sure the 'Split' button remains bottom docked, and the page is scrollable.
Click 'Split'
Make sure the last message of the report says "Receipt scan in progress"

Workspace

Sign up as a teacher using the Save The World menu, note down the principal email
Split a receipt with the workspace chat, but don't edit it.
Log into the principal's account, make sure the policy expense chat says "No activity yet', and not 'Split $0.0'

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

(The error that occurs in the beginning is on dev only and is a warning, we have an issue to fix it later and make split commands 1:1:1)

Screen.Recording.2023-10-15.at.21.44.39.mov

MacOS: Desktop

…ollable

cloudflare-workers-and-pages · 2023-10-13T14:01:44Z

Deploying with Cloudflare Pages

Latest commit:	`71064ed`
Status:	✅ Deploy successful!
Preview URL:	https://86ba1924.helpdot.pages.dev
Branch Preview URL:	https://youssef-cleanup-completespli.helpdot.pages.dev

View logs

…tions

melvin-bot · 2023-10-13T16:03:31Z

@thesahindia 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]

situchan · 2023-10-13T17:21:29Z

    isScanRequest: false,
    shouldShowSmartScanFields: true,
    isPolicyExpenseChat: false,
+    shouldAllowScrollingInputs: false,


This prop is not used anywhere

situchan · 2023-10-13T17:23:09Z

    shouldShowSmartScanFields: PropTypes.bool,

+    /** Whether we should show the amount, date, and merchant fields. */
+    isScrollable: PropTypes.bool,


I think we can remove this prop and make this component always scrollable.
And instead remove scroll views in MoneyRequestConfirmPage

I don't think we can do that. It would alter the behavior in the search page where we want the search bar to remain fixed on top.

This component is used only in 2 places - SplitBillDetailsPage, MoneyRequestConfirmationPage
There's no search bar

Ah yes sorry. I mistakenly thought the comment was under BaseOptionsSelector

You might have got confused with OptionsSelector that has search input

situchan · 2023-10-13T17:53:05Z

+
+        const {shouldAllowScrollingChildren} = this.props;
+        function ScrollWrapper({children}) {
+            if (!shouldAllowScrollingChildren) {
+                return children;
+            }
+            return (
+                <ScrollView contentContainerStyle={[styles.flexGrow1]}>
+                    <ScrollView
+                        horizontal
+                        contentContainerStyle={[styles.flex1, styles.flexColumn]}
+                    >
+                        {children}
+                    </ScrollView>
+                </ScrollView>
+            );
+        }
+


Can we move this out of component lifecycle?

Not sure where, what do you suggest? And what's the issues if we keep it here? thanks!

I was thinking of static function but I think it's better to make separate component in /components.
I see double scroll view was used in several places so it can be reused once we make it.

Ended up removing the component all together, I searched in the codebase and could only find the double scroll view used once in MoneyConfirmationList, let me know what you think!

Fine to me. I found only one occurrence in AddressSearch

App/src/components/AddressSearch/index.js

Lines 266 to 273 in fe282b4

/*

* The GooglePlacesAutocomplete component uses a VirtualizedList internally,

* and VirtualizedLists cannot be directly nested within other VirtualizedLists of the same orientation.

* To work around this, we wrap the GooglePlacesAutocomplete component with a horizontal ScrollView

* that has scrolling disabled and would otherwise not be needed

*/

<ScrollView

horizontal

situchan · 2023-10-13T21:07:19Z

please fix lint

situchan · 2023-10-14T23:17:21Z

Is ready for review?

youssef-lr · 2023-10-15T08:03:24Z

Yup @situchan, thanks!

situchan · 2023-10-15T11:16:22Z

I am seeing $0.00 everywhere for failed receipt request. Is it expected?

situchan · 2023-10-15T21:54:20Z

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 / Chrome
- iOS / native
- iOS / 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 approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- 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(themeColors.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 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.

Screenshots/Videos

Web

web1.mov

web2.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mov

Desktop

iOS

ios.mov

Android

android.mov

Co-authored-by: Situ Chandra Shil <108292595+situchan@users.noreply.github.com>

melvin-bot · 2023-10-16T08:14:07Z

@Gonals 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]

melvin-bot · 2023-10-16T08:14:11Z

🎯 @situchan, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #29657.

youssef-lr · 2023-10-16T08:20:46Z

Thanks @situchan for the thorough review!

mountiny

thanks for the thorough review @situchan

I am slightly worried about the changes to the BaseOptionsSelection in sense that there might be regressions coming. Lets be on a look out for some potentially weird behaviour in staging.

mountiny · 2023-10-16T08:55:32Z

cc @tgolen this touches the MoneyRequestConfirmationList FYI

situchan · 2023-10-16T08:59:10Z

Hope there would be no regressions as affected only when shouldAllowScrollingChildren=true 🙏

youssef-lr · 2023-10-16T09:02:06Z

Also note for the future, the nestedScrollEnabled prop we added to BaseOptionsSelector (which passes it to <OptionsList>) ended up being unused. We disabled scrolling instead by removing the max-height set from here https://github.com/Expensify/App/pull/29568/files#diff-36ce762672f28ae8eb68ed94b2d3707e161579f9912ed6aa9c07e607c6be489fL384

mountiny · 2023-10-16T09:03:03Z

Yep I agree there should not be regressions hopefully, still just ideally we are cautious of this in case some odd issues pop over now and tomorrow

OSBotify · 2023-10-16T09:04: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.

tgolen · 2023-10-16T16:05:01Z

    isScanRequest: PropTypes.bool,

+    /** Whether we're editing a split bill */
+    isEditingSplitBill: PropTypes.bool,


For the future, please name the props for the effect they should have inside the component. When only reviewing code inside this component, if you saw the prop isEditingSplitBill, you would have no idea what that prop should do or how it should be applied. For example, the prop below is called shouldShowSmartScanFields which is much more explicit and its purpose is understood immediately.

Hey @tgolen - sorry for the late reply! Will keep this in mind, however, I think it's a bit difficult in this component as it handles different IOUs. What would be a better naming here?

OSBotify · 2023-10-16T21:48:54Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.85-0 🚀

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

OSBotify · 2023-10-17T23:13:39Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀

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

OSBotify · 2023-10-18T00:00:03Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.86-0 🚀

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

OSBotify · 2023-10-18T23:57:33Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.3.86-5 🚀

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

s77rt · 2023-10-26T20:03:38Z

            result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;
        } else {
-            result.alternateText = lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');
+            result.alternateText = lastAction && lastMessageTextFromReport.length > 0 ? lastMessageText : Localize.translate(preferredLocale, 'report.noActivityYet');


This seems unintended? The lastAction check is not needed for this case

youssef-lr added 2 commits October 13, 2023 16:23

Fix proptypes

d940b33

Add configurable option to OptionsSelector and make split details scr…

ce96b01

…ollable

youssef-lr added 3 commits October 13, 2023 22:46

Show receipt scanning message for splits as well

ae0a3b6

Do not display last message from backend if report has no actions

8cf9b1e

Only make BaseOptionsSelector scrollable when inputs are below the op…

e2a5657

…tions

youssef-lr self-assigned this Oct 13, 2023

Merge branch 'main' into youssef_cleanup_completeSplitBill

a36590f

youssef-lr marked this pull request as ready for review October 13, 2023 16:03

youssef-lr requested a review from a team as a code owner October 13, 2023 16:03

melvin-bot Bot requested review from thesahindia and removed request for a team October 13, 2023 16:03

Fix proptype name

1127b20

situchan mentioned this pull request Oct 13, 2023

[$500] Web - Request Money - Long description is showing without "Paid by" and "Split with" #28073

Closed

6 tasks

trjExpensify requested review from situchan and removed request for thesahindia October 13, 2023 16:57

situchan reviewed Oct 13, 2023

View reviewed changes

Address comments

c35cbc6

youssef-lr added 6 commits October 14, 2023 15:19

Add partial merchant when creating transaction

4983055

CLeanup checking for missing fields

e622ab6

Remove extra component

d52729d

Make isEditingSplitBill depend on only the fields being empty

9e2e935

Merge branch 'main' into youssef_cleanup_completeSplitBill

87851c7

Fix missing money request status details

fc1e3ff

Remove redundant prop

18769cf

situchan reviewed Oct 15, 2023

View reviewed changes

Comment thread src/components/MoneyRequestConfirmationList.js Outdated

situchan mentioned this pull request Oct 15, 2023

[HOLD for payment 2023-10-24] [$125] IOU - Expense preview card is wider than the Pay button #29639

Closed

6 tasks

mountiny self-requested a review October 15, 2023 21:55

Update src/components/MoneyRequestConfirmationList.js

71064ed

Co-authored-by: Situ Chandra Shil <108292595+situchan@users.noreply.github.com>

situchan approved these changes Oct 16, 2023

View reviewed changes

melvin-bot Bot requested a review from Gonals October 16, 2023 08:14

melvin-bot Bot mentioned this pull request Oct 16, 2023

Payment issue for E/App PR#29568 #29657

Closed

mountiny approved these changes Oct 16, 2023

View reviewed changes

mountiny merged commit ae36e7b into main Oct 16, 2023

mountiny deleted the youssef_cleanup_completeSplitBill branch October 16, 2023 08:59

mountiny mentioned this pull request Oct 16, 2023

[CP Staging] Redefine useLocalize in SplitDetailsPage #29682

Merged

59 tasks

tgolen reviewed Oct 16, 2023

View reviewed changes

OSBotify mentioned this pull request Oct 16, 2023

Deploy Checklist: New Expensify 2023-10-16 #29717

Closed

OSBotify mentioned this pull request Oct 17, 2023

Deploy Checklist: New Expensify 2023-10-17 #29837

Closed

bernhardoj mentioned this pull request Oct 26, 2023

[$500] LHN - "No activity yet message showed in LHN rooms with conv history after coming back online #30392

Closed

6 tasks

s77rt reviewed Oct 26, 2023

View reviewed changes

	/*
	* The GooglePlacesAutocomplete component uses a VirtualizedList internally,
	* and VirtualizedLists cannot be directly nested within other VirtualizedLists of the same orientation.
	* To work around this, we wrap the GooglePlacesAutocomplete component with a horizontal ScrollView
	* that has scrolling disabled and would otherwise not be needed
	*/
	<ScrollView
	horizontal

Uh oh!

Conversation

youssef-lr commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

P2P

Workspace

Offline tests

QA Steps

P2P

Workspace

PR Author Checklist

Screenshots/Videos

Uh oh!

cloudflare-workers-and-pages Bot commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

melvin-bot Bot commented Oct 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youssef-lr Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

situchan commented Oct 13, 2023

Uh oh!

situchan commented Oct 14, 2023

Uh oh!

youssef-lr commented Oct 15, 2023

Uh oh!

situchan commented Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

situchan commented Oct 15, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

melvin-bot Bot commented Oct 16, 2023

Uh oh!

melvin-bot Bot commented Oct 16, 2023

Uh oh!

youssef-lr commented Oct 16, 2023

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny commented Oct 16, 2023

Uh oh!

situchan commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youssef-lr commented Oct 16, 2023

Uh oh!

mountiny commented Oct 16, 2023

Uh oh!

OSBotify commented Oct 16, 2023

Uh oh!

youssef-lr commented Oct 13, 2023 •

edited

Loading

cloudflare-workers-and-pages Bot commented Oct 13, 2023 •

edited

Loading

youssef-lr Oct 15, 2023 •

edited

Loading

situchan commented Oct 15, 2023 •

edited

Loading

situchan commented Oct 16, 2023 •

edited

Loading

s77rt Oct 26, 2023 •

edited

Loading