Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

kakajann · 2021-05-12T14:43:20Z

Details

We are now consistently show the recents and contacts headers on the participant selector pages across all four of these flows: Request Money, Split Bill, New Chat and New Group.

Fixed Issues

Fixed #2760

Tests

Click the + icon on the home screen to access the CreateMenu
Select Request Money 3. Enter an amount and click next to access the single-user picker on the IOUParticipants page

OR

Click the + icon on the home screen to access the CreateMenu
Select New Chat to access the single-user picker on the Participants page

QA Steps

See above

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…page used in the Request Money and New Chat flows

github-actions · 2021-05-12T14:43:39Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Julesssss · 2021-05-17T08:51:49Z

src/libs/Permissions.js

-    return _.contains(betas, CONST.BETAS.IOU) || canUseAllBetas();
+    // return _.contains(betas, CONST.BETAS.IOU) || canUseAllBetas();
+    return true;
 }


Please revert this

Julesssss · 2021-05-17T08:54:20Z

src/pages/NewChatPage.js

+        sections.push({
+            title: this.props.translate('iou.recents'),
+            data: this.state.recentReports,
+            shouldShow: this.state.recentReports.length > 0,


!_.isEmpty would be preferred here (example)

I just changed the code.

And I noticed that OptionsSelector ignores shouldShow condition but it requires it as prop.

Julesssss · 2021-05-17T08:59:06Z

tests/unit/OptionsListUtilsTest.js

+        // We should expect maxmimum of 5 recent reports to be returned
+        // minus the currently logged in user and recent reports count


Not sure that this is correct.

We should expect maxmimum of 5 recent reports to be returned minus the currently logged in user and recent reports count

I think this is two different things, no?

Recent reports should have a max of 5

personalDetails --> all the personalDetails should be returned minus the currently logged in user and recent reports count

I got inspired by getNewGroupOptions() test and I think this is correct

I think it was previously two different comments that have been merged into one?

The code is fine

What I meant here is that there should be two separate comments, but I think you have combined them into one.

// We should expect maxmimum of 5 recent reports to be returned --> this is referring to `recentReports` // minus the currently logged in user and recent reports count --> why would the `recentReports` count be `X - 5 - logged in user`?

I think it should say this:

// We should expect maxmimum of 5 recent reports to be returned // We should expect all personalDetails to be returned, minus the currently logged in user and recent reports count

Does that make sense?

Yes, I refactored the commenting

tests/unit/OptionsListUtilsTest.js

Julesssss · 2021-05-17T09:04:01Z

tests/unit/OptionsListUtilsTest.js

-        expect(results.personalDetails.length).toBe(1);
-        expect(results.personalDetails[0].text).toBe('Spider-Man');
+        expect(results.recentReports.length).toBe(1);
+        expect(results.recentReports[0].text).toBe('Spider-Man');



We should test both instead of replacing the old personalDetails test

Now I expect personalDetails to be empty

Now I expect personalDetails to be empty

Could we add that as a test case then?

Yes, here it is.

tests/unit/OptionsListUtilsTest.js

Julesssss · 2021-05-17T09:06:19Z

tests/unit/OptionsListUtilsTest.js

-        expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 2);
+        expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 2 - 5);


5 is used quite a lot, can we add a test that returns less than 5 recentReports?

@kakajann can you create a var for 5, something like let maxRecentReports = 5;

5 is used quite a lot, can we add a test that returns less than 5 recentReports?

In this line, we are testing recentReports that returns less than 5.

@kakajann can you create a var for 5, something like let maxRecentReports = 5;

Of course, I did add a file-level constant variable for maxRecentReports in my last push

…Reports and the personalDetails

…io504-add-recents-to-search-options

Julesssss · 2021-05-17T16:10:34Z

Hi @kakajann, some of the automated tests are failing. I'll take another look at the PR once these are passing.

kakajann · 2021-05-17T16:33:05Z

Hi @kakajann, some of the automated tests are failing. I'll take another look at the PR once these are passing.

Hello. I fixed the issue that failing

Julesssss · 2021-05-18T12:37:47Z

@kakajann please resolve the final check by following this comment: #2818 (comment)

kakajann · 2021-05-18T14:46:12Z

I have read the CLA Document and I hereby sign the CLA

…io504-add-recents-to-search-options

Julesssss · 2021-05-19T09:42:56Z

@kakajann the CI checks are still failing

…io504-add-recents-to-search-options

kakajann · 2021-05-19T18:48:27Z

@kakajann the CI checks are still failing

Sorry, I don't wanna keep bothering you but I tested everything in local and found no issue.
To trigger checks again I pushed an empty commit. If this won't work I don't have an idea how to pass the checks.

Also, my another PR is somehow passes every checks.

Julesssss · 2021-05-20T11:08:18Z

Looks like your last push resolved the checks somehow. there's just a few additional comments to resolve.

Julesssss · 2021-05-21T09:53:24Z

src/pages/NewChatPage.js

@@ -77,10 +80,17 @@ class NewChatPage extends Component {
    getSections() {
        const sections = [];

+        sections.push({
+            title: this.props.translate('iou.recents'),


Now that we're using this String for non-IOU pages, I think it should be refactored to a different group. Could you move the iou.contacts and iou.recents to common.contacts and common.recents please.

Yes I moved

Add the Recents and Contacts headers to the single-user participants …

8e6e216

…page used in the Request Money and New Chat flows

kakajann requested a review from a team as a code owner May 12, 2021 14:43

MelvinBot requested review from sketchydroide and removed request for a team May 12, 2021 14:43

resolve conflicts

d484819

kakajann mentioned this pull request May 13, 2021

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2760

Closed

resolve conflicts again

ba7c1c5

Julesssss reviewed May 17, 2021

View reviewed changes

tests/unit/OptionsListUtilsTest.js Show resolved Hide resolved

Julesssss reviewed May 17, 2021

View reviewed changes

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

Julesssss reviewed May 17, 2021

View reviewed changes

kakajann added 4 commits May 17, 2021 19:18

use underscore isEmpty function to check if we should show the recent…

e41360a

…Reports and the personalDetails

Merge branch 'main' of github.com:studio-504/Expensify.cash into stud…

2d825e6

…io504-add-recents-to-search-options

revert beta permissions

43b5932

test OptionsListUtils searching by email

49426e7

linter hotfix

8c05935

kakajann requested a review from Julesssss May 18, 2021 11:17

Merge branch 'main' of github.com:studio-504/Expensify.cash into stud…

443b2b0

…io504-add-recents-to-search-options

kakajann added 2 commits May 19, 2021 22:53

Merge branch 'main' of github.com:studio-504/Expensify.cash into stud…

179d0c8

…io504-add-recents-to-search-options

retrigger checks

e38ae64

create a var for maxRecentReports and make some comments more readable

fe39481

Julesssss reviewed May 21, 2021

View reviewed changes

move iou.recents to common.recents

103ebfe

Julesssss approved these changes May 24, 2021

View reviewed changes

Julesssss merged commit bd51eb4 into Expensify:main May 24, 2021

OSBotify mentioned this pull request May 24, 2021

Deploy Checklist: Expensify.cash 2021-05-22 #3076

Closed

6 tasks

kakajann mentioned this pull request Aug 23, 2021

[HOLD for payment 2021-11-04] Allow inviting multiple users to a workspace by reusing the OptionsSelector component #4775

Closed

marcaaron mentioned this pull request Sep 1, 2021

[HOLD for payment 2021-11-04] Chat - No results found message displayed even when results are found #4993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

kakajann commented May 12, 2021 •

edited

Loading

github-actions bot commented May 12, 2021 •

edited

Loading

Julesssss May 17, 2021 •

edited

Loading

kakajann May 17, 2021

Julesssss May 17, 2021

kakajann May 17, 2021

Julesssss May 17, 2021

Julesssss May 17, 2021

kakajann May 17, 2021

Julesssss May 17, 2021

Julesssss May 17, 2021

Julesssss May 20, 2021

kakajann May 21, 2021

Julesssss May 17, 2021

kakajann May 17, 2021

Julesssss May 17, 2021

kakajann May 17, 2021

Julesssss May 17, 2021

Julesssss May 20, 2021

kakajann May 21, 2021

Julesssss commented May 17, 2021

kakajann commented May 17, 2021

Julesssss commented May 18, 2021

kakajann commented May 18, 2021

Julesssss commented May 19, 2021

kakajann commented May 19, 2021

Julesssss commented May 20, 2021

Julesssss May 21, 2021

kakajann May 22, 2021

		// We should expect maxmimum of 5 recent reports to be returned
		// minus the currently logged in user and recent reports count

		expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 2);
		expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 2 - 5);

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

Conversation

kakajann commented May 12, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented May 12, 2021 • edited Loading

Julesssss May 17, 2021 • 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

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

Choose a reason for hiding this comment

Julesssss commented May 17, 2021

kakajann commented May 17, 2021

Julesssss commented May 18, 2021

kakajann commented May 18, 2021

Julesssss commented May 19, 2021

kakajann commented May 19, 2021

Julesssss commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakajann commented May 12, 2021 •

edited

Loading

github-actions bot commented May 12, 2021 •

edited

Loading

Julesssss May 17, 2021 •

edited

Loading