Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2022-07-15] [$250] Web - Request money - Five most recent chats aren't shown correctly #8220

Closed
kbecciv opened this issue Mar 18, 2022 · 54 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Mar 18, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

Pre-Condition: Have a couple/several chats already open with other accounts

  1. Access http://staging.new.expensify.com
  2. Log in to a high traffic account
  3. Click on the green "Plus" icon on the right side of chat history
  4. Click on "Request Money"
  5. Enter any amount in Big Number Pad (BNP) and any currency (For example: 1 Real)
  6. Click green "Next" button

Expected Result:

The user expects to see five most recent chats appear under the Recents header (Excluding group chats and concierge chat)

Actual Result:

The user sees five chats in this section, however they are not the five most recent chats. There is one chat that should not be placed within this list

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.44.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester: applausetester+ebezerra@applause.expensifail.com

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Recording.82.mp4
Bug5495612_Request.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Mar 18, 2022

Triggered auto assignment to @roryabraham (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Mar 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Mar 21, 2022

Triggered auto assignment to @dylanexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@dylanexpensify
Copy link
Contributor

@melvin-bot
Copy link

melvin-bot bot commented Mar 21, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

@MelvinBot
Copy link

📣 @parasharrajat You have been assigned to this job by @melvin-bot[bot]!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Mar 21, 2022

Current assignee @roryabraham is eligible for the Exported assigner, not assigning anyone new.

@metehanozyurt
Copy link
Contributor

Proposal

Cause

IOUParticipantsRequest component calls getNewChatOptions from OptionsListUtils, and it calls getOptions function which sorts contacts by lastVisitedTimestamp by default.

Solution

We should pass sortByLastMessageTimestamp parameter through these functions:

  • getNewChatOptions
  • getSearchOptions (This will change order of the new chat search order also, if this is wanted)
  • getOptions (Already takes sortByLastMessageTimestamp as a parameter)

To do this we should take sortByLastMessageTimestamp as a parameter and pass it to the getOptions in the following functions in OptionsListUtils:

function getSearchOptions(  
    reports,  
    personalDetails,  
    searchValue = '',  
    betas,  
    sortByLastMessageTimestamp = false,  
) {  
    return getOptions(reports, personalDetails, 0, {  
        betas,  
        searchValue: searchValue.trim(),  
        includeRecentReports: true,  
        includeMultipleParticipantReports: true,  
        maxRecentReportsToShow: 0, // Unlimited  
  prioritizePinnedReports: false,  
        prioritizeDefaultRoomsInSearch: false,  
        sortByReportTypeInSearch: true,  
        showChatPreviewLine: true,  
        showReportsWithNoComments: true,  
        includePersonalDetails: true,  
        sortByLastMessageTimestamp: !!sortByLastMessageTimestamp,  
        forcePolicyNamePreview: true,  
        prioritizeIOUDebts: false,  
    });  
}

// ...

function getNewChatOptions(  
    reports,  
    personalDetails,  
    betas = [],  
    searchValue = '',  
    selectedOptions = [],  
    excludeLogins = [],  
    sortByLastMessageTimestamp = false,  
) {  
    return getOptions(reports, personalDetails, 0, {  
        betas,  
        searchValue: searchValue.trim(),  
        selectedOptions,  
        excludeDefaultRooms: true,  
        includeRecentReports: true,  
        includePersonalDetails: true,  
        maxRecentReportsToShow: 5,  
        excludeLogins,  
        sortByLastMessageTimestamp,  
    });  
}

We should update OptionsListUtils.getNewChatOptions function call with the new parameter in the constructor of IOUParticipantsRequest component:

constructor(props) {  
    super(props);  
  
    this.addSingleParticipant = this.addSingleParticipant.bind(this);  
  
    const {  
        recentReports,  
        personalDetails,  
        userToInvite,  
    } = OptionsListUtils.getNewChatOptions(  
        props.reports,  
        props.personalDetails,  
        props.betas,  
        '',  
        [],  
        CONST.EXPENSIFY_EMAILS,  
        true, // New parameter
    );  
  
    this.state = {  
        recentReports,  
        personalDetails,  
        userToInvite,  
        searchValue: '',  
    };  
}

Before ;

20220328_012133.mp4

After Fix issue ;

20220328_012243.mp4

@parasharrajat
Copy link
Member

@metehanozyurt's proposal looks good to me.

cc: @roryabraham

Rory, Do we need the same behavior everywhere where we are showing newChat Contacts?

If so, we will not have to add another param to OptionsListUtils.getNewChatOptions. It will just modify the call to getOptions in the above proposal.

🎀 👀 🎀 C+ reviewed

@roryabraham
Copy link
Contributor

@metehanozyurt's proposal looks good, but I am unwilling to approve any proposal or PR that touches OptionsListUtils without also creating unit tests to cover the change.

@metehanozyurt If you agree to also create unit tests, 👍 this message and I will assign you the job.

@parasharrajat
Copy link
Member

Oh, yeah. I was thinking the same just forgot to ask here.

@dylanexpensify
Copy link
Contributor

Checking in, seems we will have someone for the job @parasharrajat? Just checking in case we need to double the price

@parasharrajat
Copy link
Member

No need to double. we are good here.

@mountiny mountiny changed the title Web - Request money - Five most recent chats aren't shown correctly [$250] Web - Request money - Five most recent chats aren't shown correctly Mar 31, 2022
@metehanozyurt
Copy link
Contributor

metehanozyurt commented Apr 2, 2022

Hi @parasharrajat and @roryabraham. I add some tests to OptionsListUtilsTest. Hope you like it.

I'm sorry for being late. I wrote before, but I was very nervous because the results I wanted did not come out. Then I noticed that the 'lastMessageTimestamp' values ​​were entered the same value in test file. Then i check the other results too. I wrote test for 'getSearchOptions' and 'getNewChatOptions' functions.

here edited OptionsListUtilsTest file

https://github.com/metehanozyurt/testFile/blob/main/OptionsListUtilsTest.js

I know this issue title about 'Request money' issue but when it will fixed 'Split Money' , 'New Chat' and 'New Group' isues can came up again.

When we change this file (IOUParticipantsRequest), other files will sort differently.
IOUParticipantsSplit, NewChatPage,

i can set the parameter if you want.

before ;

20220402_172031.mp4

after

20220402_174322.mp4

@dylanexpensify
Copy link
Contributor

@parasharrajat still good with where we are here on price?

@parasharrajat
Copy link
Member

parasharrajat commented Apr 5, 2022

@dylanexpensify If we are good to hire the selected proposal then I don't think a price increase is needed.

@parasharrajat
Copy link
Member

@metehanozyurt I didn't understand this post #8220 (comment). Could you please try to rephrase it?

Are you trying to say that, your proposed solution does not have desired results?

@NicMendonca NicMendonca added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jul 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2022

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 4, 2022
@NicMendonca
Copy link
Contributor

deployed to staging, so will be ready for payment soon

@NicMendonca NicMendonca added Weekly KSv2 and removed Daily KSv2 labels Jul 4, 2022
@JmillsExpensify
Copy link

Great thanks! I'll handle the payment once we clear the regression period.

@JmillsExpensify
Copy link

This issue might be affected by the late production deploy. PR still isn't showing we're in the regression period.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 8, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.79-17 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-07-15. 🎊

@melvin-bot melvin-bot bot changed the title [$250] Web - Request money - Five most recent chats aren't shown correctly [HOLD for payment 2022-07-15] [$250] Web - Request money - Five most recent chats aren't shown correctly Jul 8, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week)

@JmillsExpensify
Copy link

Almost through the regression period following the production deploy last week.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Jul 14, 2022
@roryabraham
Copy link
Contributor

Looks like we're just awaiting payment here

@melvin-bot melvin-bot bot removed the Overdue label Jul 18, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (4th week)

@JmillsExpensify
Copy link

@roryabraham yes indeed. I just took a look at the Upwork, and we actually still need to hire contributors. I'm going to jump in and send offers now, such that when I'm up tomorrow contracts will be accepted and I can issue payments.

@JmillsExpensify
Copy link

Ah, the saga continues! Looks like the previous post from above is already expired. I've created a fresh one here: https://www.upwork.com/jobs/~01a80e9993680dc31b. @parasharrajat @metehanozyurt I've already invited you both to the job. Please accept and we'll get this processed for you both pronto.

@metehanozyurt
Copy link
Contributor

Thank you for invitation @JmillsExpensify . There was something I wanted to talk about. It was bonus for this job, I wonder if I can still get it. Thanks.
#8220 (comment)

@JmillsExpensify
Copy link

@metehanozyurt Yes absolutely. Thank you so much for pointing that out. I've sent the updated offer (same for @parasharrajat). I'll be on later this evening so maybe we'll cross paths and we can close this out!

@JmillsExpensify
Copy link

Thanks both for your patience. I've issued payments in Upwork, so I'm closing out this issue. Please reach out if we've missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants