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 2023-07-24] [$1000] Reply in thread count is not update while send message faster #19445

Closed
1 of 5 tasks
kavimuru opened this issue May 23, 2023 · 59 comments
Closed
1 of 5 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Needs Reproduction Reproducible steps needed

Comments

@kavimuru
Copy link

kavimuru commented May 23, 2023

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 any chat
  2. send message
  3. reply in thread
  4. send some messages on thread fast
  5. get back to main chat
  6. observe the reply count

Expected Result:

Reply count should be accurate

Actual Result:

take some time to update reply count

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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.16.3
Reproducible in staging?: Needs reproduction
**Reproducible in production?:**Needs reproduction
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

Screen.Recording.2023-05-19.at.6.35.33.PM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e0fdd621bd8a21ae
  • Upwork Job ID: 1661086126594076672
  • Last Price Increase: 2023-06-13
@kavimuru kavimuru added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels May 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

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

@alitoshmatov
Copy link
Contributor

alitoshmatov commented May 23, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

Reply in thread count is not update while send message faster

What is the root cause of that problem?

We are solely relying on backend update here, we don't have optimistic data to handle this

What changes do you think we should make in order to solve the problem?

In addActions

function addActions(reportID, text = '', file) {

We should check if this report has parent action item to determine if it is a report thread by checking allReport[reportID].parentReportActionID.
Then we should retrieve parent report action by

    const report = allReports[reportID];
    const parentReportActionItem = allReportActions[report.parentReportID][report.parentReportActionID];

We should update this parent action item with following updated fields: childVisibleActionCount, childOldestFourEmails, childCommenterCount

  1. Then we can increase childVisibleActionCount to +1.
  2. Check if childOldestFourEmails contains currentUserEmail if yes, replace it to the last part of this list. If not add it to the last of this list.
  3. Increase childCommenterCount by one, but we cannot determine if current user is alredy included or not, we can see childOldestFourEmails but it is limited to the last 4 emails

What alternative solutions did you explore? (Optional)

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label May 23, 2023
@melvin-bot melvin-bot bot changed the title Reply in thread count is not update while send message faster [$1000] Reply in thread count is not update while send message faster May 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01e0fdd621bd8a21ae

@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 23, 2023

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

@dukenv0307
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Thread item in parent report is not update while send message faster in thread

What is the root cause of that problem?

When add a message in thread, we dont't update the optimisticData of parent report action in addActions function. So that thread item is not update immediately. It waits for api return data before updating.

What changes do you think we should make in order to solve the problem?

When add a comment in thread, we should update the optimisticData of parent report action in addActions function with fields that are childCommenterCount, childLastVisibleActionCreated, childOldestFourEmails, childVisibleActionCount. In addActions we should get the report with reportID from allReports. use isThread fucntion to check the report is a thread or not. If it is a thread we should update optimisticData of parent report action as follows:

  1. Get parentReportAction from allReportActions by getting parentReportID and parentReportActionID from current report
  2. Increase childVisibleActionCount by 1
  3. If the thread doesn't have any message parentReportAction.childCommenterCount is undefined, we should update childCommenterCount to 1 to display thread item else we update it to parentReportAction.childCommenterCount because we cannot be sure that current user has been sent message yet.
  4. childLastVisibleActionCreated should be updated to currentTime
    const currentTime = DateUtils.getDBTime();
  5. We should split parentReportAction.childOldestFourEmails into an array. And then, we check if currentUserEmail is not in array, we should push it to the last of array and also remove the first email if the length of array is equal 4. Otherwise we replace currentUserEmail to the last of array.
  6. Push optimisticParentReportActionData to optimisticData

We should update optimisticData of parent report action because it needs to update thread item immediately and also in offline mode.

What alternative solutions did you explore? (Optional)

@thesahindia
Copy link
Member

Hi, I'll be OOO today and tomorrow. Please reassign. Sorry for the short notice!

@thesahindia thesahindia removed their assignment May 25, 2023
@melvin-bot melvin-bot bot added the Overdue label May 25, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label May 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 25, 2023

📣 @0xmiroslav You have been assigned to this job by @puneetlath!
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 📖

@puneetlath puneetlath added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 25, 2023
@puneetlath
Copy link
Contributor

@chiragsalian @stitesExpensify assigning you guys as CME on this issue as discussed here, since you guys are the one I see on the threads tracking issue and this is a threads bug. Thanks y'all!

@stitesExpensify
Copy link
Contributor

@0xmiroslav do you have opinions on the above proposals?

@melvin-bot melvin-bot bot removed the Overdue label May 29, 2023
@puneetlath
Copy link
Contributor

Looks like the PR is close. Just waiting on a final review from @stitesExpensify

@puneetlath
Copy link
Contributor

How's the PR going? Are we close?

@dukenv0307
Copy link
Contributor

We are waiting the finally review from stitesExpensify today.

@stitesExpensify
Copy link
Contributor

Yep, we're chugging along!

@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2023

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

  • when @dukenv0307 got assigned: 2023-06-14 16:31:09 Z
  • when the PR got merged: 2023-07-13 19:19:14 UTC
  • days elapsed: 21

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jul 17, 2023
@melvin-bot melvin-bot bot changed the title [$1000] Reply in thread count is not update while send message faster [HOLD for payment 2023-07-24] [$1000] Reply in thread count is not update while send message faster Jul 17, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 17, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 17, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.41-3 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 2023-07-24. 🎊

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 @niravkakadiya25
  • Contributor that fixed the issue @dukenv0307
  • Contributor+ that helped on the issue and/or PR @0xmiroslav

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
Copy link

melvin-bot bot commented Jul 17, 2023

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:

  • [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
  • [@0xmiroslav] 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:
  • [@0xmiroslav] 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:
  • [@0xmiroslav] Determine if we should create a regression test for this bug.
  • [@0xmiroslav] 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.
  • [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 23, 2023
@puneetlath
Copy link
Contributor

puneetlath commented Jul 24, 2023

@niravkakadiya25 can you please apply to this job? https://www.upwork.com/ab/applicants/1683499299447517184/hired

@dukenv0307 @0xmiroslav sent you contracts.

@0xmiroslav friendly reminder on the checklist.

@niravkakadiya25
Copy link

@puneetlath can you please sent contract to me as well?
Here is my upwork link:
https://www.upwork.com/freelancers/~01b750750a0d76226c

@niravkakadiya25
Copy link

@puneetlath accepted

@puneetlath
Copy link
Contributor

@dukenv0307 and @niravkakadiya25 have been paid. @0xmiroslav just waiting on the checklist for you.

@0xmiros
Copy link
Contributor

0xmiros commented Jul 25, 2023

This is more like feature request. We never implemented optimistic data for parent report action before.

@0xmiros
Copy link
Contributor

0xmiros commented Jul 25, 2023

can you please hold my payment until further notice? I am working on some stuff due to recent measurements in my region. And update issue to Monthly. Thanks

@puneetlath
Copy link
Contributor

@0xmiroslav can we pay this one out now? Can you link me the Upwork account that we should be paying?

@0xmiros
Copy link
Contributor

0xmiros commented Aug 21, 2023

@puneetlath
Copy link
Contributor

Offer sent.

@puneetlath
Copy link
Contributor

All paid!

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 Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Needs Reproduction Reproducible steps needed
Projects
None yet
Development

No branches or pull requests

9 participants