[HOLD for payment 2023-02-08] pinned chat become unpinned when user pinned chat in offline mode

[kavimuru]

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:

1. go to offline
2. click into an existing chat or create a new one
3. click on pin button to pin the chat
4. confirm the pin icon is lit up, i.e. the chat was pinned
5. go to online

Expected Result:

chat should be pinned

Actual Result:

chat becomes unpinned

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

Seems to be only affecting Chrome on computer and mobile device

Version Number: 1.2.59-1
Reproducible in staging?: y
Reproducible in production?: y
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:

Recording.1378.mp4

Screen.Recording.2023-01-20.at.3.01.23.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674207539542629

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018bae5da04b47a887
• Upwork Job ID: 1618712481741504512
• Last Price Increase: 2023-01-26

[melvin-bot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018bae5da04b47a887

[marcaaron]

Can reproduce this one on dev. You don't need to create a new chat with a user while offline though. It happens because we are making an identical request and for some reason we are trying to deduplicate them here:

App/src/libs/actions/NetworkRequestQueue.js

Line 20 in 9193948

persistedRequests = lodashUnionWith(persistedRequests, retryableRequests, _.isEqual);

This is not how the sequential queue is supposed to work so there's a gap in the implementation. Gonna grab this one.

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @sobitneupane (Internal)

[sonialiap]

I can only reproduce the issue on Chrome

Desktop, Android app, and Safari all correctly keep the chat pinned

[sonialiap]

As a heads up, I will be OOO Feb 2-11

[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.2.63-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove lodashUnionWith as it breaks the sequential queue processing #14608

If no regressions arise, payment will be issued on 2023-02-08. 🎊

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:

• [@sobitneupane / @marcaaron] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane / @marcaaron] 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:
• [@sobitneupane / @marcaaron] 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:
• [@sobitneupane] Propose regression test steps to ensure the same bug will not reach production again.
• [@sonialiap] [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

I'm OOO Feb 2-11, reassigning bug0

[marcaaron]

@sobitneupane The regression test could be improved by asking the tester to queue up several "pin" and "unpin" events while offline. The last action should be the one that sticks when they come back from offline. Thank you!

[sobitneupane]

Thanks for the suggestions @marcaaron.

[sobitneupane]

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:

• The PR that introduced the bug has been identified. Link to the PR:

#6556

• 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:

#6556 (comment)

[sobitneupane]

Regression Test Proposal

• Go offline
• Open an existing chat or create a new one
• "Pin" and "Unpin" the chat several times
• Go online
• Verify that the last action that was taken (either pinned or unpinned) is the eventual final state after going online.

Do we agree 👍 or 👎

[sobitneupane]

@marcaaron Please let me know if you see any further room for improvement.

[marcaaron]

I'd update this to:

Verify that the last action that was taken (either pinned or unpinned) is stuck and is not changed after going online.

Verify that the last action that was taken (either pinned or unpinned) is the eventual final state after going online.

(in reality the state could switch between pinned unpinned before eventually landing on the correct state) <- expected behavior

[sobitneupane]

Thanks @marcaaron. Updated the step in Regression Test Proposal.

[abekkala]

• Reported by: @gadhiyamanan $250
• PR Review: @sobitneupane $1000

@marcaaron I see that this one was Internal. can you confirm those are the only two payments needed.

[abekkala]

Offers sent, in Upwork, to @gadhiyamanan and @sobitneupane

[marcaaron]

@abekkala If an issue has only ever been Internal and there were no proposals then my understanding is the C+ and issue reporter are the only ones who require compensation.

[abekkala]

ok, yes, that's what I was thinking too but figured I'd get confirmation to be sure! 😃

[abekkala]

Upwork confirms payments have been sent

[gadhiyamanan]

@abekkala please close the upwork contract

[abekkala]

yes @gadhiyamanan after initiating payments I was also in the process of closing for both you and @sobitneupane!
I've just closed them and came to make note:

Both payments have been sent, the contract has been completed, and the Upwork job post has been closed.

[abekkala]

not overdur

[abekkala]

@marcaaron @sobitneupane Can I confirm that the current TestRail that already exists applies to this. (based on the Regression Test Proposal shown above)

[marcaaron]

@abekkala I think we can update it slightly so that we are specifically testing for several "pin" / "unpin" actions in a row while offline? The issue we fixed here is technically not covered by the existing regression test.

[abekkala]

@marcaaron ah ok, I guess then I'm not sure which one this test would fall under as this particular test says to Pin and Unpin several times.
Our current testing has test steps for the Pin action and then separate test steps for the Unpin action. (both while Offline)

Or, is it just a matter of adding a last step to both of them:
Verify that the last action taken (either pinned or unpinned) is the final state after returning to online

[marcaaron]

this particular test says to Pin and Unpin several times

There are two tests so in that sense tester would pin/unpin more than once. The specific behavior would be something like:

• Go offline
• pin
• unpin
• pin
• unpin
• Go online
• Verify the chat ends up in an upinned state eventually

Lmk if that clears it up!

[abekkala]

omg - yes. Thanks @marcaaron I (for some reason) was thinking about that a different way

[marcaaron]

I think we are done with this one? Gonna close - but feel free to re-open if there's anything left.