Fix: Request money - Five most recent chats aren't shown correctly#8549
Conversation
melvin-bot
Bot
requested review from
parasharrajat and
roryabraham
and removed request for
a team
|
Just leaving a quick comment, but I think it would be easier to default |
There was a problem hiding this comment.
We don't have to pass the config options from the callable location. The whole purpose of creating getNewChatOptions, getSearchOptions functions was to encapsulate the configuration and use it as it is.
Secondly, it seems every function is setting that option to true. SO set it to true by default value and then should be enough. Then explicitly set it false where needed.
|
Thank you so much @parasharrajat , @marcaaron . I will immediately update commit and test file for changes 🙏. |
|
Need to confirm the sort order for various pages. Thanks for the patience. |
|
@metehanozyurt Could you please merge the main so that I can test it? |
|
Now I merged the conflict problem solved. So Sorry for falling behind the main. |
|
I will review this in some time. |
parasharrajat
left a comment
parasharrajat
left a comment
There was a problem hiding this comment.
let me know when ready so that I can test.
| props.personalDetails, | ||
| '', | ||
| props.betas, | ||
| true, |
There was a problem hiding this comment.
As pointed earlier, we should not expose the config option outside. Instead of doing this create another wrapper method like OptionsListUtils#getSearchOptions which sets this flag to true and then use that method.
There was a problem hiding this comment.
After new updates all places using functions started sending true by default (getSearchOptions & getNewChatOptions). Now we dont need wrapper method. Because If I write a new function, the old one will not be used this time. I'm making the changes now as you said. I'm removing the parameter pass support. Thank you.
After this i will update PR videos immediately.
There was a problem hiding this comment.
Sorry, But I didn't understand. Could you please rephrase this?
There was a problem hiding this comment.
getSearchOptions function only used on SearchPage. That's why I fixed the mistake I made by getting extra config. if i wrote another function the old function would be un used.
| }); | ||
|
|
||
| it('getNewChatOptions()', () => { | ||
| it('getSearchOptions() with message timeStampOrder', () => { |
There was a problem hiding this comment.
When you will do the above change, here new test will be created for each function including that new function.
There was a problem hiding this comment.
I will update OptionsListUtilsTest test and removing condition timeStampOrder (true-false). Thank you.
|
#8549 (comment) There is one last thing I want to say @parasharrajat . When I examined the PR files, I realized that it is more beautiful now. Thank you very much for your guidance and help. Sorry for not being able to do it earlier. I hope I didn't seem too emotional, but I really thank you 🙏 . |
|
Bumping this to keep the issue moving forward @parasharrajat @roryabraham 😄 |
|
I will review this today. Apologies for the delay. It just got out of sync. |
| }) | ||
| )); | ||
|
|
||
| // PersonalDetails should be ordered Alphabetically by default acording to issue #8220 |
There was a problem hiding this comment.
Change the issue link to the comment that exactly asks for it.
| showReportsWithNoComments: true, | ||
| includePersonalDetails: true, | ||
| sortByLastMessageTimestamp: false, | ||
| sortByLastMessageTimestamp: true, |
There was a problem hiding this comment.
Set the default value for this true in getOptions definition as I can see that is always true for all methods.
| // even though the currently logged in user matches they should not show. | ||
| expect(results.personalDetails.length).toBe(1); | ||
| expect(results.recentReports.length).toBe(2); | ||
| // should be ordered by lastMessageTimestamp values |
There was a problem hiding this comment.
| // should be ordered by lastMessageTimestamp values | |
| // Should be ordered by lastMessageTimestamp values. |
| results = OptionsListUtils.getSearchOptions(REPORTS, PERSONAL_DETAILS, 'fantastic'); | ||
|
|
||
| // Then we get both values with the pinned value still on top | ||
| // lastMessageTimestamp value should be top |
There was a problem hiding this comment.
| // lastMessageTimestamp value should be top | |
| // Value with latestd lastMessageTimestamp should be at the top. |
| ); | ||
| expect(personalDetailWithExistingReport.reportID).toBe(2); | ||
|
|
||
| // When we call only pass personal details |
There was a problem hiding this comment.
| // When we call only pass personal details | |
| // When we only pass personal details |
| expect(results.recentReports.length).toBe(2); | ||
| expect(results.recentReports[0].text).toBe('Invisible Woman'); | ||
| expect(results.recentReports[1].text).toBe('Spider-Man'); | ||
| // Then all single participant reports that match will show up in the recentReports array, Recent used person should be top |
There was a problem hiding this comment.
| // Then all single participant reports that match will show up in the recentReports array, Recent used person should be top | |
| // Then all single participant reports that match will show up in the recentReports array, Recently used contact should be at the top |
|
No, reports are not supposed to be part of this list. Only contacts. |
|
Ok , We want to list of contacts like LHN but only contacts. Recents on the top am i right? We need For Example New Chat Page; Lines 65 to 76 in fc3e13e If we don't give the reports and only empty array(line 70), it will only give us the contact list(alphabeticly ordered). The recentReports information will never come.
|
|
Ok, I would say just leave the contacts in A-Z order. @roryabraham Do you think the A-Z order is fine for contacts on the Workspace member invite page? |
Yes I think that would be fine in isolation – but for search, new chat, and request pages we'll want to sort results by most recent timestamp. Given that, I would say let's be consistent for the workspace member invite page. |
|
Ok so @metehanozyurt we can just keep the workspace behavior the same as it was before. The only change would be that it will now be sorted using |
|
Thank you @parasharrajat . I updated the QA steps and created a checklist. I hope it was in the requested format and descriptive. |
|
Thanks for doing that. Do you have an update for #8549 (comment)? |
|
I thought the current version covers this. I thought you wanted it like the contact list on other pages. New chat , New group , Request money , Send money, Split bill pages : Workspace invite page before no ordered just The last user added was the last person on list . I can make the workspace page no sorting at all. But now it's beautiful 🥰. Don't you think it's more beautiful like this on phones like a contact list? My humble opinion is that it looks really beautiful now. But of course, you are the decision makers. If you want the old version, I can get it right back no problem @parasharrajat . |
|
I am following this #8549 (comment). Please feel free to open this on Slack to get consensus. |
|
Did you get a chance to ask this on slack? |
|
Thanks @parasharrajat for chance to do this. I asked a question, I hope I did it right. I was a little nervous as this was my first time doing something like this. |
|
I'm a little sorry that my question didn't get much attention 😢. I can't sort by last message time information because it will only be a list of people messaged before. |
|
I have bumped the thread. |
|
There is consensus to leave the page as it is for Workspace Invite page. So you can do the following.
Any questions? |
|
Thank you for helping me on Slack @parasharrajat . function getMemberInviteOptions(
reports,
personalDetails,
betas = [],
searchValue = '',
selectedOptions = [],
excludeLogins = [],
) {
return getOptions(reports, personalDetails, 0, {
betas,
searchValue: searchValue.trim(),
selectedOptions,
excludeDefaultRooms: true,
includePersonalDetails: true,
excludeLogins,
sortPersonalDetailsByAlphaAsc: false,
});
}I am currently updating the test document for this new method. |
|
Hi @parasharrajat , I merged it with main branch. I updated test file , PR steps and videos. Thanks for your time 🙏 . |
| return 0; | ||
| }], ['asc']); | ||
| } | ||
|
|
| * @returns {Object} | ||
| */ | ||
| function getMemberInviteOptions( | ||
| reports, |
There was a problem hiding this comment.
Do we need this argument? We can just remove it and refactor all the usages of this function.
There was a problem hiding this comment.
Thanks @parasharrajat . I removed report parameter and updated all functions. Also removed selectedOptions.
|
@Expensify/applauseleads can we be sure to update the regression test suite to account for the new sorting orders on these pages, and ensure that as part of the TC's we're verifying that these list pages are correctly sorted? Thanks 🙇 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @roryabraham in version: 1.1.79-0 🚀
|
|
🚀 Deployed to production by @roryabraham in version: 1.1.79-17 🚀
|
6 participants
Details
getNewChatOptions and getSearchOptions functions changed for passing sortByLastMessageTimestamp parameter. NewChatPage.js , OUParticipantsRequest.js , IOUParticipantsSplit.js files changed. "New chat" , "New group" , "Send money" , "Request money" , "Split bill" pages recents chat areas fixed.
Fixed Issues
$ #8220
Tests
Recentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderexample reproduce issue video before fix:
20220408_003702.mp4
test video:
20220408_003925.1.mp4
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Recentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderRecentssection should be ordered last message time ,Contactssection should be sorted by alphabetical orderPages that changes recents order (sort by last message time):
Recentssection)Recentssection)Recentssection)Recentssection)Recentssection)Pages that changes contacts order (sort by alphabetically order):
Contactssection)Contactssection)Contactssection)Contactssection)Contactssection)Verify that no errors appear in the JS console
Screenshots
Web
five-recent-Web-Screen-Recording-2022-06-12-at-13.04.21.mp4
Mobile Web
five-recent-ios-web-Screen-Recording-2022-06-12-at-13.27.27.mp4
five-recent-android-web-Screen-Recording-2022-06-12-at-13.37.41.mp4
Desktop
five-recent-desktop-Screen-Recording-2022-06-12-at-13.20.30.mp4
iOS
five-recent-ios-native-Screen-Recording-2022-06-12-at-13.24.52.mp4
Android
five-recent-android-native-Screen-Recording-2022-06-12-at-13.33.16.mp4