[HOLD for payment 2023-06-29] [HOLD for payment 2023-06-28] [$1000] Search - Group chat displays blank in Search results

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to staging.new.expensify.com
2. Log in with HT account or any account where you have a lot of group conversation
3. Click on magnify glass and scroll the page

Expected Result:

Group chat displays without issue

Actual Result:

Group chat displays blank in Search result

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.29-6

Reproducible in staging?: yes

Reproducible in production?: no

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Recording.3170.mp4

Recording.5060.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01541118f633ad9ca1
• Upwork Job ID: 1671222841055092736
• Last Price Increase: 2023-06-20

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

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

[Beamanator]

Looks still reproducible in staging & main

[Beamanator]

(@amyevans I'm not planning to look into the fix for now, let me know if you need me to)

[amyevans]

Thanks, I can repro on main too, taking a closer look

[jeet-dhandha]

Its a basic change. We had to pass participantsList when we have a list of participants.

[jeet-dhandha]

Before:

After:

[amyevans]

@jeet-dhandha Your fix isn't working in my local tests

(this.props.option.accountID is 0)

[Beamanator]

@amyevans just checking which account has option.accountID of 0? It should almost never be 0 ideally 😬

[Pujan92]

App/src/components/OptionRow.js

Line 138 in 8ba2234

(this.props.option.participantsList || this.props.option.accountID ? [this.props.option] : []).slice(0, 10),

I think this condition should be(if participants not available then only check the accountID)

(this.props.option.participantsList || (this.props.option.accountID ? [this.props.option] : [])).slice(0, 10),

[amyevans]

@Beamanator this is the data (I'm not clear yet where that data flows from, so can't say more than that at the moment - maybe it's just bad local data somehow?):

@Pujan92 that condition worked for me 👍 , how does it look to you @Beamanator? (cc @luacmartins)

[Beamanator]

Ooh @Pujan92 that could be correct 🙃

Sadly I'm able to see this bug @amyevans but am investigating another bug, let me know if you need me to help with this one, but seems we may be on track for the fix already 🙏

[amyevans]

@Pujan92 do you have capacity/interest in putting together a PR for this fix quickly?

If not, I will handle once I'm out of a meeting in about 30-40 min.

[Pujan92]

Yes @amyevans , I can raise a PR now for the fix.

[luacmartins]

So the accountID is 0 because we default it to 0 here, but we only set it if the report doesn't have multiple participants here. Is that the expected behavior @Beamanator?

[Beamanator]

Aah interesting @luacmartins - I believe I only default to 0 cuz I thought that made sense, but seeing that everything else starts out as null maybe the accountID should NOT start as 0 - good find

[luacmartins]

I think it's fine to initialize with 0 or null, but what do we want to store for group chats?

[Beamanator]

I guess we'd use result.participantsList for group chats?

[luacmartins]

Yea, that's what @Pujan92's solution is doing.

[Pujan92]

Yes, currently due to this condition (this.props.option.participantsList || this.props.option.accountID ? [this.props.option] : []).slice(0, 10),
It always pass either this.props.option or [], so wrapping with () the second condition solves the issue and pass this.props.option.participantsList when it is available.

[luacmartins]

@Pujan92 feel free to tag me as a reviewer too

[luacmartins]

Thank you!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Fix: Search - Group chat displays blank in Search results #21148

If no regressions arise, payment will be issued on 2023-06-28. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

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

• Fix: Search - Group chat displays blank in Search results #21148

If no regressions arise, payment will be issued on 2023-06-29. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR: Add tooltips and nav to bill details #20746
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: 685169c#r120008049
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: think a Slack discussion is not needed because this seemed to have been a side-effect of another change which was not caught during the testing.
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: N/A

[joekaufmanexpensify]

This qualifies for a speed bonus, so we need to issue the following payments here:

• @Pujan92 - $1,500 ($1,000 for PR, and $500 speed bonus).
• @allroundexperts - $1,500 ($1,000 for review, and $500 speed bonus).
• No reporting bonus here.

[joekaufmanexpensify]

@Pujan92 offer sent for $1,000 ($500 will be issued as bonus)

[joekaufmanexpensify]

@allroundexperts offer sent for $1,000 ($500 will be issued as bonus)

[joekaufmanexpensify]

@allroundexperts Could you please complete your portion of the BZ checklist when you have a chance? Thanks!

[joekaufmanexpensify]

All set to issue payment soon once the BZ checklist is complete.

[allroundexperts]

1. The PR that introduced the bug has been identified. Link to the PR: Add tooltips and nav to bill details #20746
2. The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: 685169c#r120008049
3. I think a Slack discussion is not needed because this seemed to have been a side-effect of another change which was not caught during the testing.
4. Regression test would be helpful.

Regression Test Proposal

1. Login and click on search icon
2. Verify in the search list that Group chats display text isn't empty

Do we agree 👍 or 👎

[amyevans]

I don't think we need a regression test specific to this case, Group Chat flows have plenty of regression tests already and blank text would be very obvious to the testers already without calling it out as a separate item.

[joekaufmanexpensify]

BZ checklist is complete! All set to issue payment.

[joekaufmanexpensify]

@Pujan92 $1,500 sent and contract ended!

[joekaufmanexpensify]

@allroundexperts $1,500 sent and contract ended!

[joekaufmanexpensify]

Upwork job closed.

[joekaufmanexpensify]

Bug is fixed, BZ checklist complete, and all payment issued. Closing as this is all set. Thanks everyone!