Improve cancelling money in a different currency offline #13329

youssef-lr · 2022-12-05T17:47:02Z

Details

This PR improves the experience of cancelling money offline in a different currency. Since we cannot update the total while we're offline, as the currency conversion happens in the server, we can now let the user now that the total of the IOU is not final until they're back online.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/245284
$ #14015

Tests

Verify that no errors appear in the JS console

Offline tests

Request Money - multiple currencies (offline)

Disable the internet connection on the device
Create a money request in a 1:1 conversation i.e 100 USD
Verify the IOU is created in conversation but it is grayed out
Create a second money request but this time in a different currency to the iouReport i.e 100 AED
Verify the IOU Preview total does not update
Verify the IOU Preview has a message explaining that the total will update when you come back online
Click on the IOU Preview to access the details in the rightDocked side modal
Verify the IOU Preview in the right pane matches the IOU Preview displayed in the main chat
Enable the internet connection on the device
Verify the IOU Preview is no longer greyed out
Verify the total in the IOU Preview updates correctly in both the chat and the rightDocked side modal, displayed in the iouReport's consolidated currency (in this example, that would be USD)
Refresh/close and reopen app
Verify the IOU is still displaying correctly in both places

Cancelling money requests - multiple currencies (offline)

Create a money request in the report's currency, i.e. 100 USD.
Go offline.
Create a money request in a different currency, i.e. 20 AED.
Make sure you see the message in the IOU Preview saying that total will update when you're back online.
Cancel the 20 AED money request.
Make sure the message is no longer there, as we are not waiting for currency conversion anymore.

QA Steps

Verify that no errors appear in the JS console
Request Money - multiple currencies (offline)

Disable the internet connection on the device
Create a money request in a 1:1 conversation i.e 100 USD
Verify the IOU is created in conversation but it is grayed out
Create a second money request but this time in a different currency to the iouReport i.e 100 AED
Verify the IOU Preview total does not update
Verify the IOU Preview has a message explaining that the total will update when you come back online
Click on the IOU Preview to access the details in the rightDocked side modal
Verify the IOU Preview in the right pane matches the IOU Preview displayed in the main chat
Enable the internet connection on the device
Verify the IOU Preview is no longer greyed out
Verify the total in the IOU Preview updates correctly in both the chat and the rightDocked side modal, displayed in the iouReport's consolidated currency (in this example, that would be USD)
Refresh/close and reopen app
Verify the IOU is still displaying correctly in both places

Cancelling money requests - multiple currencies (offline)

Create a money request in the report's currency, i.e. 100 USD.
Go offline.
Create a money request in a different currency, i.e. 20 AED.
Make sure you see the message in the IOU Preview saying that total will update when you're back online.
Cancel the 20 AED money request.
Make sure the message is no longer there, as we are not waiting for currency conversion anymore.

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-01-02.at.15.20.12.mov

Mobile Web - Chrome

chrome.android.-.Screen.Recording.2023-01-03.at.22.47.18.mov

Mobile Web - Safari

safari.ios.-.RPReplay_Final1672781322.MP4

Desktop

desktop.-.Screen.Recording.2023-01-03.at.21.37.34.mov

iOS

ios.-.Screen.Recording.2023-01-03.at.22.24.34.mov

Android

android.-.Screen.Recording.2023-01-03.at.22.38.30.mov

…is different

mountiny

That is quite rather non-trivial logic right there 😄

src/components/ReportActionItem/IOUAction.js

mountiny · 2022-12-05T18:05:57Z

src/components/ReportActionItem/IOUAction.js

+
+    const shouldShowIOUPreview = (
+        props.isMostRecentIOUReportAction && Boolean(props.action.originalMessage.IOUReportID)
+    ) || props.action.originalMessage.type === 'pay';


Isnt there some const for the pay type?

Yeah will update.

mountiny · 2022-12-05T18:06:40Z

src/components/ReportActionItem/IOUAction.js

+            <IOUPreview
+                pendingAction={lodashGet(props.action, 'pendingAction', null)}
+                iouReportID={props.action.originalMessage.IOUReportID.toString()}
+                chatReportID={props.chatReportID}
+                shouldShowPendingConversionMessage={shouldShowPendingConversionMessage}
+                onPayButtonPressed={launchDetailsModal}
+                onPreviewPressed={launchDetailsModal}
+                containerStyles={[
+                    styles.cursorPointer,
+                    props.isHovered
+                        ? styles.iouPreviewBoxHover
+                        : undefined,
+                ]}
+                isHovered={props.isHovered}


Indentation

hmm I think this is the correct indentation, my editor auto indents the file as per eslint rules. I'll double check.

Maybe I am wrong, definitely not as smart as IDE 😄

src/libs/IOUUtils.js

mountiny · 2022-12-05T18:08:48Z

src/libs/IOUUtils.js

@@ -63,7 +64,73 @@ function updateIOUOwnerAndTotal(iouReport, actorEmail, amount, currency, type =
    return iouReportUpdate;
 }

+/**
+ * Returns the list of IOU actions


Maybe a bit more context for this method would be useful.

Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>

youssef-lr · 2022-12-05T18:17:27Z

That is quite rather non-trivial logic right there 😄

Comments didn't help? 😕

mountiny · 2022-12-05T18:18:27Z

Comments didn't help? 😕

Nono, I get it and how it works, but it is definitely not as easy as we would hope, especially for someone who does not have broader understanding of the IOU flow.

youssef-lr · 2022-12-05T18:35:10Z

Nono, I get it and how it works, but it is definitely not as easy as we would hope, especially for someone who does not have broader understanding of the IOU flow.

This is solving more than just the issue mentioned here. I've added these improvements:

Cancelling every request offline will remove the IOUPreview, just like when we're online.
Cancelling request made offline in a different currency, will revert back to showing the IOUPreview without the pending message, as there is no conversion to be made.
Cancelling a request in the report's currency while we have pending request in a different currency, will not update the IOU report locally, to prevent this bug:

Screen.Recording.2022-12-05.at.14.36.25.mov

But yes..I'm definitely open to suggestions to make this more straightforward!

… conversion

src/components/ReportActionItem/IOUPreview.js

mountiny · 2022-12-06T12:19:36Z

src/libs/IOUUtils.js

+        const areAllRequestsInDifferentCurrencyCancelled = _.every(
+            pendingCancelledRequestsInDifferentCurrency,
+            requestTransactionID => _.contains(pendingRequestsInDifferentCurrency, requestTransactionID),
+        );
+        if (areAllRequestsInDifferentCurrencyCancelled) {
+            return false;
+        }


I think we could simplify this by calling underscore intersection on this https://underscorejs.org/#intersection such that if pendingCancelledRequestsInDifferentCurrency intersection pendingRequestsInDifferentCurrency length === pendingCancelledRequestsInDifferentCurrency.length that means it is proper subset hence the same.

Personally that is a bit easier but i like Venn diagrams so that might be why :D

I simplified this even more by simply checking if both arrays are identical :D.

mananjadhav

Left some minor comments.

mananjadhav · 2022-12-27T16:19:09Z

src/components/ReportActionItem/IOUAction.js

@@ -30,9 +34,21 @@ const propTypes = {
        hasOutstandingIOU: PropTypes.bool.isRequired,
    }),

+    /** IOU report data object */
+    iouReport: PropTypes.shape({


qq: We have iouReportPropTypes in pages/iouReportPropTypes. Any specific reason we're creating a new PropTypes.shape here?

I think I copied this from IOUAction and the good thing is that is shows just what we need to use from the object, in this case only currency is needed. But I'm happy to change this.

mananjadhav · 2022-12-27T16:20:18Z

src/components/ReportActionItem/IOUAction.js

    /** Whether the IOU is hovered so we can modify its style */
    isHovered: PropTypes.bool,

+    network: {


qq: again we have networkPropTypes defined, so any specific reason we're defining again instead of importing?

src/languages/en.js

mananjadhav · 2022-12-27T16:29:11Z

src/libs/IOUUtils.js

+function getIOUReportActions(reportActions, iouReport, type = '', pendingAction = '', filterRequestsInDifferentCurrency = false) {
+    return _.chain(reportActions)
+        .filter(action => action.originalMessage
+            && action.actionName === 'IOU'


Can we pick this from CONST? May be REPORT.ACTIONS.TYPE.IOU ?

src/pages/home/report/ReportActionItem.js

src/pages/iou/IOUTransactions.js

mananjadhav · 2022-12-27T16:38:42Z

@youssef-lr Can you also update the Author checklist after fixing the comments? We also need to update the screenshots along with the author checklist. I can only then fill the reviewer checklist.

Meanwhile I'll start testing as the core logic seems to be looking good.

techievivek · 2022-12-28T17:34:31Z

tests/unit/IOUUtilsTest.js

+
+    // Default is to create requests offline, if this is specified then we need to remove the pendingAction
+    if (isOnline) {
+        moneyRequestAction = {


Can't we do moneyRequestAction.pendingAction = null or if we want to delete the key then delete moneyReportAction.pendingAction? Any specific reason we are copying it?

Yeah totally, will update. It's just a habit of mine to do immutable stuff 😄

Aaah, I see. It's fine then I guess.

techievivek · 2022-12-28T17:39:24Z

tests/unit/IOUUtilsTest.js

+        expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(false);
+    });
+
+    test('Cancelling a request made online while wxe have on made offline will show the preview', () => {


Suggested change

test('Cancelling a request made online while wxe have on made offline will show the preview', () => {

test('Cancelling a request made online while we have one made offline will show the pending conversion message', () => {

techievivek · 2022-12-28T17:42:28Z

tests/unit/IOUUtilsTest.js

+        expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(true);
+    });
+
+    test('Cancelling a request offline in the report\'s currency when we have requests in a different currency does not show the pending ui', () => {


Suggested change

test('Cancelling a request offline in the report\'s currency when we have requests in a different currency does not show the pending ui', () => {

test('Cancelling a request offline in the report\'s currency when we have requests in a different currency does not show the pending conversion message', () => {

techievivek · 2022-12-28T17:46:27Z

tests/unit/IOUUtilsTest.js

+});
+
+describe('isIOUReportPendingCurrencyConversion', () => {
+    test('Requesting money offline in a different currency shows the IOUReport as pending', () => {


The test description doesn't seem consistent with other descriptions, please try to keep it consistent.

Requesting money offline in a different currency will show the pending conversion message

techievivek

Looks good mostly; great work with this PR.

youssef-lr · 2022-12-29T15:09:09Z

Updated! Working on screen recordings.

mountiny

@youssef-lr nice job, havent had time to test but left couple of NABs related to comments. Would you be able to also leave some short info int he details section?

@mananjadhav Will you be able to complete the reviewer's checklist along with Youssef working on the authors list? 🙇

src/libs/IOUUtils.js

tests/unit/IOUUtilsTest.js

Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>

mananjadhav · 2023-01-02T18:59:05Z

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 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 correct English and 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 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web-offline-multicurrency-iou.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

desktop-offline-multicurrency-iou.mov

iOS

ios-offline-multicurrency-iou.mov

Android

android-offline-multicurrency-iou.mov

@mountiny @techievivek I did start reviewing and testing in parallel. I was able to test this in all platforms except mobile web due to the IP issue! Can I get some help here?

Also @youssef-lr screencasts for platforms excluding Web are pending. Can you please update them?

mountiny

thanks @mananjadhav

@mountiny @techievivek I did start reviewing and testing in parallel. I was able to test this in all platforms except mobile web due to the IP issue! Can I get some help here?

Also @youssef-lr screencasts for platforms excluding Web are pending. Can you please update them?

@youssef-lr Would you be able to update the details section and provide videos for this one so we can merge 🙇 Thanks!

youssef-lr · 2023-01-03T17:46:12Z

Would you be able to update the details section and provide videos for this one so we can merge

On it!

youssef-lr · 2023-01-03T21:55:48Z

@mountiny added screen recordings for all platforms as well as updated the details section.

mountiny · 2023-01-04T13:06:21Z

Gonna look into this later tonight or tomorrow, sorry a bit run out of time. Can you merge main in after the reportActionID changes as well to confirm that works fine?

the PR author checklist seems to be failing too @youssef-lr

mountiny · 2023-01-05T11:49:51Z

Mobile ios

I think there is another improvement we could do down the line although I am worried the logic might get quite complicated.

As you can see here, I have created $100 request online, went offline, requested AED20, cancelled the $100 request, then there was no IOUPreview, but we should show it since we only had one currency I guess. then adding another request in another currency AFN20 just showed the preview with that AND no message about calculation as it basically disregards the offline made request.

I would not worry about it now.

completing the PR reviewing checklist

Android chrome also worked fine

mountiny

Thanks @youssef-lr and @mananjadhav

Issue for payment is here #14015

OSBotify · 2023-01-05T11:59:01Z

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

OSBotify · 2023-01-05T17:27:32Z

🚀 Deployed to staging by @mountiny in version: 1.2.49-0 🚀

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

OSBotify · 2023-01-06T19:27:49Z

🚀 Deployed to production by @luacmartins in version: 1.2.49-0 🚀

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

sobitneupane · 2023-02-02T04:26:24Z

This PR caused a regression. #14355

mananjadhav · 2023-04-27T09:44:36Z

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

youssef-lr added 5 commits December 5, 2022 01:46

Display a pending message offline when currency request or cancelled …

157184d

…is different

Improve cancel money request flow when dealing with different currencies

099cca6

Cleanup

ed02180

Set iouReport total to 0 if all requests have been cancelled

cf638e5

Fix logic in isIOUReportPendingCurrencyConversion

7038110

youssef-lr force-pushed the youssef_iou_pending_currency_conversion branch from 38ae98f to 7038110 Compare December 5, 2022 17:47

youssef-lr added 2 commits December 5, 2022 18:48

Cleanup

d531cec

Improve comments

b7c2ac9

mountiny assigned mountiny and youssef-lr and unassigned mountiny Dec 5, 2022

Fix comments, propTypes, and JSDocs

c4492d2

youssef-lr force-pushed the youssef_iou_pending_currency_conversion branch from 129c34f to c4492d2 Compare December 5, 2022 18:02

mountiny reviewed Dec 5, 2022

View reviewed changes

Apply suggestions from code review

37e457c

Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>

youssef-lr added 2 commits December 5, 2022 19:21

use CONST for report action type

b1442f8

Add more context to function comment

18ec67e

youssef-lr added 4 commits December 6, 2022 03:55

Simplify non trivial logic for deciding if an IOU is pending currency…

ce98c1e

… conversion

Fix logic and clarify comments

2ade174

Revert back to using previous logic as it's reliable

865c684

Remove bug fix to be fixed in a separate issue

85f17d3

mountiny reviewed Dec 6, 2022

View reviewed changes

Simply logic once more

966e018

youssef-lr force-pushed the youssef_iou_pending_currency_conversion branch from 9168f88 to 966e018 Compare December 6, 2022 13:23

youssef-lr added 3 commits December 6, 2022 14:33

Fix CSS styling and add converting message

5f030c6

Small code improvement

fb6a160

Add initial tests

db6a7bd

mananjadhav reviewed Dec 27, 2022

View reviewed changes

techievivek reviewed Dec 28, 2022

View reviewed changes

Apply suggestions from code review

8a683dd

mountiny reviewed Dec 29, 2022

View reviewed changes

src/libs/IOUUtils.js Outdated Show resolved Hide resolved

src/libs/IOUUtils.js Outdated Show resolved Hide resolved

tests/unit/IOUUtilsTest.js Outdated Show resolved Hide resolved

Apply suggestions from code review

ba2ab0b

Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>

mananjadhav approved these changes Jan 2, 2023

View reviewed changes

mountiny approved these changes Jan 2, 2023

View reviewed changes

mountiny mentioned this pull request Jan 5, 2023

[HOLD for payment 2023-01-13] C+ payment for internal PR review #14015

Closed

mountiny approved these changes Jan 5, 2023

View reviewed changes

mountiny merged commit 8c675d4 into main Jan 5, 2023

mountiny deleted the youssef_iou_pending_currency_conversion branch January 5, 2023 11:54

OSBotify mentioned this pull request Jan 5, 2023

Deploy Checklist: New Expensify 2023-01-05 #14023

Closed

15 tasks

sobitneupane mentioned this pull request Feb 2, 2023

[PAID] [$1000] App crashes when splitting a bill offline #14355

Closed

6 tasks

alexxxwork mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed

	test('Cancelling a request made online while wxe have on made offline will show the preview', () => {
	test('Cancelling a request made online while we have one made offline will show the pending conversion message', () => {

	test('Cancelling a request offline in the report\'s currency when we have requests in a different currency does not show the pending ui', () => {
	test('Cancelling a request offline in the report\'s currency when we have requests in a different currency does not show the pending conversion message', () => {

Improve cancelling money in a different currency offline #13329

Improve cancelling money in a different currency offline #13329

Conversation

youssef-lr commented Dec 5, 2022 • edited by mountiny Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

mountiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youssef-lr commented Dec 5, 2022

mountiny commented Dec 5, 2022

youssef-lr commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youssef-lr Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav commented Dec 27, 2022

Choose a reason for hiding this comment

youssef-lr Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techievivek Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techievivek left a comment

Choose a reason for hiding this comment

youssef-lr commented Dec 29, 2022

mountiny left a comment

Choose a reason for hiding this comment

mananjadhav commented Jan 2, 2023 • edited by mountiny Loading

Reviewer Checklist

Screenshots/Videos

mountiny left a comment

Choose a reason for hiding this comment

youssef-lr commented Jan 3, 2023

youssef-lr commented Jan 3, 2023

mountiny commented Jan 4, 2023

mountiny commented Jan 5, 2023

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 5, 2023

OSBotify commented Jan 5, 2023

OSBotify commented Jan 6, 2023

sobitneupane commented Feb 2, 2023

mananjadhav commented Apr 27, 2023

youssef-lr commented Dec 5, 2022 •

edited by mountiny

Loading

youssef-lr Dec 29, 2022 •

edited

Loading

youssef-lr Dec 28, 2022 •

edited

Loading

techievivek Dec 28, 2022 •

edited

Loading

mananjadhav commented Jan 2, 2023 •

edited by mountiny

Loading