Fix IOU Options bug #8839

sobitneupane · 2022-04-30T04:59:43Z

Show Request and Send Money options when there are two participants, Splitbill option when there are more than two participants and no option if there is only one participant

Details

Fixed Issues

$ #8357

Tests

Open one-to-one chats, group chats and chat rooms with single participant, two participants and multiple participants
Click on Action (+) Button in ReportAction Compose box to see various options.
Verify that
- there are "Request money", "Send money" and "Add attachment" options in one-to-one chats and chat rooms with two participants.
- there are "Split bill" and "Add attachment" options in group chats and chat rooms with more than two participants.
- there is only "Add attachment" option in chat room with single participant.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Open one-to-one chats, group chats and chat rooms with single participant, two participants and multiple participants
Click on Action (+) Button in ReportAction Compose box to see various options.
Verify that
- there are "Request money", "Send money" and "Add attachment" options in one-to-one chats and chat rooms with two participants.
- there are "Split bill" and "Add attachment" options in group chats and chat rooms with more than two participants.
- there is only "Add attachment" option in chat room with single participant.

Verify that no errors appear in the JS console

Screenshots

Web

Chat Room With only 1 member	Chat Room With 2 members	Chat Room With more than 2 members

One-to-One Chat	Group Chat

Mobile Web

Chat Room With only 1 member	Chat Room With 2 members	Chat Room With more than 2 members

One-to-One Chat	Group Chat

Desktop

Chat Room With only 1 member	Chat Room With 2 members	Chat Room With more than 2 members

One-to-One Chat	Group Chat

iOS

Chat Room With only 1 member	Chat Room With 2 members	Chat Room With more than 2 members

One-to-One Chat	Group Chat

Android

Chat Room With only 1 member	Chat Room With 2 members	Chat Room With more than 2 members
]

One-to-One Chat	Group Chat

Show Request and Send Money options when there are two participants, Splitbill option when there are more than two participants and no option if there is only one participant

Santhosh-Sellavel

Can you check out this #8357 (comment)

I've asked to move the logic to a new method!

sobitneupane · 2022-05-02T17:29:53Z

Can you check out this #8357 (comment)

I've asked to move the logic to a new method!

Done.

@iwiznia @Santhosh-Sellavel I have found another existing bug. While splitting bill, the logged in user(Chaudary Dai in this case) is counted twice.

I think changing

participants={this.props.hasMultipleParticipants
                                                    ? this.state.participants
                                                    : _.filter(this.state.participants, email => this.props.myPersonalDetails.login !== email.login)}

to

participants={_.filter(this.state.participants, email => this.props.myPersonalDetails.login !== email.login)}

in IOUModal.js solves the issue.

Should I go through whole process of reporting? Or will I be compensated if I solved it here?

Santhosh-Sellavel · 2022-05-02T17:23:56Z

src/pages/home/report/ReportActionCompose.js

+    /**
+     * Returns the list of report compose actions
+     *
+     * @param {Boolean} showSplitBill
+     * @param {Boolean} showSendRequestMoney
+     * @param {Boolean} hasExcludedIOUEmails
+     * @param {Function} openPicker
+     * @param {Function} displayFileInModal
+     * @returns {Array<object>}
+     */
+    getReportComposeActions(showSplitBill, showSendRequestMoney, hasExcludedIOUEmails, openPicker, displayFileInModal) {
+        return [


Do we really need to compute and pass everything to this function it does not make any sense?

Santhosh-Sellavel · 2022-05-02T17:28:39Z

src/pages/home/report/ReportActionCompose.js

+            ...(!hasExcludedIOUEmails
+                && Permissions.canUseIOU(this.props.betas)
+                && showSplitBill ? [
+                    {
+                        icon: Expensicons.Receipt,
+                        text: this.props.translate('iou.splitBill'),
+                        onSelected: () => {
+                            Navigation.navigate(
+                                ROUTES.getIouSplitRoute(
+                                    this.props.reportID,
+                                ),
+                            );
+                        },
+                    },
+                ] : []),
+            ...(!hasExcludedIOUEmails
+                && Permissions.canUseIOU(this.props.betas)
+                && showSendRequestMoney ? [
+                    {
+                        icon: Expensicons.MoneyCircle,
+                        text: this.props.translate('iou.requestMoney'),
+                        onSelected: () => {
+                            Navigation.navigate(
+                                ROUTES.getIouRequestRoute(
+                                    this.props.reportID,
+                                ),
+                            );
+                        },
+                    },
+                ] : []),
+            ...(!hasExcludedIOUEmails && Permissions.canUseIOUSend(this.props.betas) && showSendRequestMoney ? [
+                {
+                    icon: Expensicons.Send,
+                    text: this.props.translate('iou.sendMoney'),
+                    onSelected: () => {
+                        Navigation.navigate(
+                            ROUTES.getIOUSendRoute(
+                                this.props.reportID,
+                            ),
+                        );
+                    },
+                },
+            ] : []),
+            {
+                icon: Expensicons.Paperclip,
+                text: this.props.translate('reportActionCompose.addAttachment'),
+                onSelected: () => {
+                    openPicker({
+                        onPicked: (file) => {
+                            displayFileInModal({file});
+                        },
+                    });
+                },
+            },
+        ];


Code is not DRY, we have redundant checks here. Can you refactor this?

Refactoring this might result into nested ternary operators. I was discouraged to use nested ternary operators in previous PRs. Any suggestion?

We could use simple if/else no need for ternary here.

It will look something like this. Any suggestion?

const actions = []; if (!hasExcludedIOUEmails) { if (Permissions.canUseIOU(this.props.betas)) { if (showSplitBill) { actions.push( { icon: Expensicons.Receipt, text: this.props.translate('iou.splitBill'), onSelected: () => { Navigation.navigate( ROUTES.getIouSplitRoute( this.props.reportID, ), ); }, }, ); } if (showSendRequestMoney) { actions.push({ icon: Expensicons.MoneyCircle, text: this.props.translate('iou.requestMoney'), onSelected: () => { Navigation.navigate( ROUTES.getIouRequestRoute( this.props.reportID, ), ); }, }); } } if (Permissions.canUseIOUSend(this.props.betas) && showSendRequestMoney) { actions.push({ icon: Expensicons.Send, text: this.props.translate('iou.sendMoney'), onSelected: () => { Navigation.navigate( ROUTES.getIOUSendRoute( this.props.reportID, ), ); }, }); } } actions.push({ icon: Expensicons.Paperclip, text: this.props.translate('reportActionCompose.addAttachment'), onSelected: () => { openPicker({ onPicked: (file) => { displayFileInModal({file}); }, }); }, }); return actions;

Let's just move IOU options computation alone to a new method, leave Add Attachment untouched thanks!

getIOUOptions(reportParticipants) { const iouOptions = []; const participants = _.filter(reportParticipants, email => this.props.myPersonalDetails.login !== email).length > 1; const hasExcludedIOUEmails = lodashIntersection(reportParticipants, CONST.EXPENSIFY_EMAILS).length > 0; const hasMultipleParticipants = participants.length > 1; if (hasExcludedIOUEmails || participants.length === 0 || Permissions.canUseIOU(this.props.betas)) { return iouOptions; } if (hasMultipleParticipants) { // append split } else { if (Permissions.canUseIOUSend(this.props.betas)) { // append send } // append request } return iouOptions; }

cc: @iwiznia

Santhosh-Sellavel · 2022-05-02T17:40:51Z

src/pages/iou/IOUModal.js

+                                                participants={this.props.hasMultipleParticipants
+                                                    ? this.state.participants
+                                                    : _.filter(this.state.participants, email => this.props.myPersonalDetails.login !== email.login)}


Should this be done here, I think it could be moved along the lines here

App/src/pages/iou/IOUModal.js

Lines 110 to 111 in 4c64bec

const participants = lodashGet(props, 'report.participants', []);

const participantsWithDetails = _.map(OptionsListUtils.getPersonalDetailsForLogins(participants, props.personalDetails), personalDetails => ({

Participants are updated during IOUParicipantsPage step. So, it cannot be moved here. We need to make use of updated participants list.

IOUParicipantsPage will not be used for Room or group flow. So no issues there.

Check and let me know!

I don't think it's a good idea. It adds more complication.

Santhosh-Sellavel · 2022-05-02T17:55:28Z

@sobitneupane I believe this is well within the scope of this issue.
If it's going to get compensated then it's for reporting only. It was evidently reported by someone else before you in this #8357 (comment)

cc @iwiznia

sobitneupane · 2022-05-02T18:26:34Z

@sobitneupane I believe this is well within the scope of this issue. If it's going to get compensated then it's for reporting only. It was evidently reported by someone else before you in this #8357 (comment)

I have already solved the issue mentioned in this comment while solving this issue as it is part of this issue. It would have been an issue introduced while solving this issue. But while testing I found that the issue I mentioned also occurs. If it is believed to be part of this issue I will solve it here. It was an existing issue.

Santhosh-Sellavel · 2022-05-02T18:55:16Z

@sobitneupane I believe this is well within the scope of this issue. If it's going to get compensated then it's for reporting only. It was evidently reported by someone else before you in this #8357 (comment)

I have already solved the issue mentioned in this comment while solving this issue as it is part of this issue. It would have been an issue introduced while solving this issue. But while testing I found that the issue I mentioned also occurs. If it is believed to be part of this issue I will solve it here. It was an existing issue.

Thanks, I would let @iwiznia take the final call!

sobitneupane · 2022-05-03T03:06:01Z

@sobitneupane I believe this is well within the scope of this issue. If it's going to get compensated then it's for reporting only. It was evidently reported by someone else before you in this #8357 (comment)

I have already solved the issue mentioned in this comment while solving this issue as it is part of this issue. It would have been an issue introduced while solving this issue. But while testing I found that the issue I mentioned also occurs. If it is believed to be part of this issue I will solve it here. It was an existing issue.

Thanks, I would let @iwiznia take the final call!

Solved this issue as well. I am okay either way.

iwiznia · 2022-05-03T15:23:19Z

I think it's basically the same issue or very closely related, so original amount should apply. If you disagree, please let me know and we can discuss.

iwiznia · 2022-05-03T15:27:25Z

src/pages/home/report/ReportActionCompose.js

+                    onSelected: () => {
+                        Navigation.navigate(
+                            ROUTES.getIOUSendRoute(
+                                this.props.reportID,
+                            ),
+                        );
+                    },


Given it's only one param per method and line is not too long, I think this is more readable

Suggested change

onSelected: () => {

Navigation.navigate(

ROUTES.getIOUSendRoute(

this.props.reportID,

),

);

},

onSelected: () => Navigation.navigate(ROUTES.getIOUSendRoute(this.props.reportID)),

made the requested change

iwiznia · 2022-05-03T15:27:54Z

src/pages/home/report/ReportActionCompose.js

+     * @returns {Array<object>}
+     */
+    getIOUOptions(reportParticipants) {
+        const iouOptions = [];


Remove this (see other comments)

made the requested change

iwiznia · 2022-05-03T15:27:59Z

src/pages/home/report/ReportActionCompose.js

+        const hasMultipleParticipants = participants.length > 1;
+
+        if (hasExcludedIOUEmails || participants.length === 0 || !Permissions.canUseIOU(this.props.betas)) {
+            return iouOptions;


Suggested change

return iouOptions;

return [];

made the requested change

iwiznia · 2022-05-03T15:29:25Z

src/pages/home/report/ReportActionCompose.js

+            iouOptions.push(
+                {
+                    icon: Expensicons.Receipt,
+                    text: this.props.translate('iou.splitBill'),
+                    onSelected: () => {
+                        Navigation.navigate(
+                            ROUTES.getIouSplitRoute(
+                                this.props.reportID,
+                            ),
+                        );
+                    },
+                },
+            );
+        } else {


Early return here

Suggested change

iouOptions.push(

{

icon: Expensicons.Receipt,

text: this.props.translate('iou.splitBill'),

onSelected: () => {

Navigation.navigate(

ROUTES.getIouSplitRoute(

this.props.reportID,

),

);

},

},

);

} else {

return [{

icon: Expensicons.Receipt,

text: this.props.translate('iou.splitBill'),

onSelected: () => Navigation.navigate(ROUTES.getIouSplitRoute(this.props.reportID)),

}];

}

made the requested change

iwiznia · 2022-05-03T15:29:49Z

src/pages/home/report/ReportActionCompose.js

+                },
+            );
+        } else {
+            iouOptions.push({


Suggested change

iouOptions.push({

const iouOptions = [];

iouOptions.push({

else statement is removed as it is no longer needed and variable is declared at the top of the function.

iwiznia · 2022-05-03T15:30:22Z

src/pages/home/report/ReportActionCompose.js

+                onSelected: () => {
+                    Navigation.navigate(
+                        ROUTES.getIouRequestRoute(
+                            this.props.reportID,
+                        ),
+                    );
+                },


Suggested change

onSelected: () => {

Navigation.navigate(

ROUTES.getIouRequestRoute(

this.props.reportID,

),

);

},

onSelected: () => Navigation.navigate(ROUTES.getIOUSendRoute(this.props.reportID)),

made the requested change

iwiznia · 2022-05-04T14:26:37Z

@Santhosh-Sellavel if everything looks good, please approve the PR.

Santhosh-Sellavel · 2022-05-04T16:29:18Z

src/pages/home/report/ReportActionCompose.js

+        myPersonalDetails: {
+            key: ONYXKEYS.MY_PERSONAL_DETAILS,
+        },


prop type is defined for myPersonalDetails can you add that, thanks!

Santhosh-Sellavel · 2022-05-04T18:59:14Z

PR Reviewer Checklist

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 verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
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 was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team 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 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 */
- Any functional components have the displayName property
- 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 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.

Santhosh-Sellavel · 2022-05-04T19:04:31Z

@sobitneupane
Add what to verify as result step with options to show based on different chat types. Add it to the Tests & QA Test sections.

Example:
In 1:1 verify that only request, send, attachment options are shown ..

Santhosh-Sellavel

@iwiznia
I have left a comment here to update the test steps.

Everything else looks good, tests well!

iwiznia · 2022-05-05T10:56:28Z

Waiting till the comment above is addressed before merging

sobitneupane · 2022-05-05T13:52:03Z

@sobitneupane Add what to verify as result step with options to show based on different chat types. Add it to the Tests & QA Test sections.

Added.

Santhosh-Sellavel · 2022-05-05T17:20:34Z

@iwiznia We are good to go here!

OSBotify · 2022-05-09T20:02:35Z

🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

OSBotify · 2022-05-13T16:11:32Z

🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀

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

Fix IOU Options bug

3677e33

Show Request and Send Money options when there are two participants, Splitbill option when there are more than two participants and no option if there is only one participant

sobitneupane marked this pull request as ready for review May 2, 2022 14:25

sobitneupane requested a review from a team as a code owner May 2, 2022 14:25

melvin-bot bot requested review from iwiznia and Santhosh-Sellavel and removed request for a team May 2, 2022 14:25

Santhosh-Sellavel suggested changes May 2, 2022

View reviewed changes

Move Report Action Compose Logic

2a601ad

Santhosh-Sellavel suggested changes May 2, 2022

View reviewed changes

Move variable calculations to specific function

03a88f4

sobitneupane added 2 commits May 3, 2022 08:09

Use if/else statements instead of ternary operators

4819a4b

Solve the issue of the logged in user occuring twice in splitbills

657693b

iwiznia requested changes May 3, 2022

View reviewed changes

sobitneupane added 2 commits May 4, 2022 09:18

Refactor getIOUOptions code

9e3952a

Merge branch 'main' into sn_bug-iou-options

189ec6b

iwiznia previously approved these changes May 4, 2022

View reviewed changes

Santhosh-Sellavel reviewed May 4, 2022

View reviewed changes

Add propType for myPersonalDetails

980ba2e

sobitneupane dismissed iwiznia’s stale review via 980ba2e May 4, 2022 16:51

Santhosh-Sellavel approved these changes May 4, 2022

View reviewed changes

iwiznia approved these changes May 5, 2022

View reviewed changes

iwiznia merged commit ec828a2 into Expensify:main May 5, 2022

OSBotify mentioned this pull request May 10, 2022

Deploy Checklist: New Expensify 2022-04-25 #8777

Closed

This was referenced May 11, 2022

[High]: Chrome+Jaws: Screen reader: Role and label are not defined for controls. #8434

Closed

[med] Chrome + Jaws: Add button (+): Action button (+) is not accessible using screen reader. #8478

Closed

Puneet-here mentioned this pull request Nov 27, 2022

[Regression test steps needed] [$1000] Request/Send money available in default-rooms/chatrooms with 2 members, error if used. #13069

Closed

	const participants = lodashGet(props, 'report.participants', []);
	const participantsWithDetails = _.map(OptionsListUtils.getPersonalDetailsForLogins(participants, props.personalDetails), personalDetails => ({

Fix IOU Options bug #8839

Fix IOU Options bug #8839

Conversation

sobitneupane commented Apr 30, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

sobitneupane commented May 2, 2022 • edited Loading

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

sobitneupane May 2, 2022 • edited Loading

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

Choose a reason for hiding this comment

Santhosh-Sellavel commented May 2, 2022

sobitneupane commented May 2, 2022 • edited Loading

Santhosh-Sellavel commented May 2, 2022

sobitneupane commented May 3, 2022

iwiznia commented May 3, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobitneupane May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia commented May 4, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel commented May 4, 2022 • edited Loading

PR Reviewer Checklist

Santhosh-Sellavel commented May 4, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

iwiznia commented May 5, 2022

sobitneupane commented May 5, 2022

Santhosh-Sellavel commented May 5, 2022

OSBotify commented May 9, 2022

OSBotify commented May 13, 2022

sobitneupane commented Apr 30, 2022 •

edited

Loading

sobitneupane commented May 2, 2022 •

edited

Loading

sobitneupane May 2, 2022 •

edited

Loading

sobitneupane commented May 2, 2022 •

edited

Loading

sobitneupane May 4, 2022 •

edited

Loading

Santhosh-Sellavel commented May 4, 2022 •

edited

Loading