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 2024-11-13] Redundant OpenReport requests when opening a thread #51680

Closed
1 of 8 tasks
tgolen opened this issue Oct 29, 2024 · 29 comments
Closed
1 of 8 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@tgolen
Copy link
Contributor

tgolen commented Oct 29, 2024

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


Version Number: v9.0.55-3
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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
Expensify/Expensify Issue URL:
Issue reported by: @tgolen
Slack conversation: Reported it in #49162 (comment)

Action Performed:

  1. Have User 1 and User 2 in a chat together
  2. User 1 adds a comment
  3. User 1 opens the comment in a thread
  4. User 1 adds a comment to the thread
  5. User 2 opens the thread

Expected Result:

User 2 should only have a single call to OpenReport

Actual Result:

Use 2 has two calls to OpenReport

Workaround:

Can the user still use Expensify without this being fixed? Yes, it is non-breaking.

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

image

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @twisterdotcom
@tgolen tgolen added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 29, 2024
@tgolen tgolen self-assigned this Oct 29, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 29, 2024
Copy link

melvin-bot bot commented Oct 29, 2024

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

@tgolen tgolen removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 29, 2024
Copy link

melvin-bot bot commented Oct 29, 2024

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 29, 2024

@twisterdotcom @situchan This was something I reported in #49162 (comment) and @nkdengineer already debugged it and wrote a quick proposal for it. I'm opening this issue so the problem can be fixed. We can skip looking at other proposals for this one.

@nkdengineer Can you please comment on this issue so I can assign it to you?

@nkdengineer
Copy link
Contributor

@tgolen I'm here.

@nkdengineer
Copy link
Contributor

Thanks, I will open the PR tomorrow morning.

@twisterdotcom
Copy link
Contributor

Zim zam zoom. Here to watch the masters at work and pay out.

@c3024
Copy link
Contributor

c3024 commented Oct 29, 2024

Can I be the C+ here because I was the C+ auto-assigned for the linked issue? 😅

@twisterdotcom twisterdotcom assigned c3024 and unassigned situchan Oct 29, 2024
@situchan
Copy link
Contributor

I think this is dupe of #49948

@bernhardoj
Copy link
Contributor

@tgolen Yes, this is a dupe of #49948

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 30, 2024
@nkdengineer
Copy link
Contributor

@c3024 The PR is here.

@shubham1206agra
Copy link
Contributor

@tgolen I am really sorry, but I am bit busy writing the implementations of projects. So, can you assign @bernhardoj here and let him complete the PR, and let @c3024 be the C+.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 30, 2024

OK, regarding the duplicate... this is kind of a tough one and I am on the fence.

The other issue was opened first, and @bernhardoj's proposal is almost the same as @nkdengineer's. However, that other issue moved a bit slower and @nkdengineer already has a PR written.

I think it's probably best to close out the other issue and let @nkdengineer get paid for the PR that has already been created to fix this.

@bernhardoj
Copy link
Contributor

@situchan and I already mentioned that this was a dupe before the PR was open. I've been explaining things on the other issue waiting for an approval. I think it would be unfair if the original issue is closed in favor of a dupe issue.

@twisterdotcom
Copy link
Contributor

I'm open to just paying out for both contributors for their time, but we need to prioritise which one PR is pushed and reviewed.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 31, 2024

The PR from @nkdengineer is already merged, so I don't think anything needs prioritized here.

@twisterdotcom
Copy link
Contributor

Nice okay. I think we should pay out:

All after deploy + 7 days.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 6, 2024
@melvin-bot melvin-bot bot changed the title Redundant OpenReport requests when opening a thread [HOLD for payment 2024-11-13] Redundant OpenReport requests when opening a thread Nov 6, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 6, 2024
Copy link

melvin-bot bot commented Nov 6, 2024

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

Copy link

melvin-bot bot commented Nov 6, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.57-10 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 2024-11-13. 🎊

For reference, here are some details about the assignees on this issue:

  • @c3024 requires payment (Needs manual offer from BZ)
  • @nkdengineer requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Nov 6, 2024

@c3024 @twisterdotcom The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@c3024
Copy link
Contributor

c3024 commented Nov 13, 2024

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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: https://github.com/Expensify/App/pull/15994/files#r1839487684

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: No discussion was started because this bug could not have been identified earlier with specific checklist items.

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

  • None

Test:

  1. [User A] Open the chat with user B.
  2. [User A] Send a message.
  3. [User A] Click on "Reply in thread" and send some messages in this thread.
  4. [User B] Open the DM with user A.
  5. Open the browser Developer Tools > Network.
  6. [User B] Open the thread created at step 3.
  7. Verify that only one OpenReport request is made in the Network tab.

Do we agree 👍 or 👎

Copy link

melvin-bot bot commented Nov 13, 2024

Payment Summary - WIP

Upwork Job

BugZero Checklist (@twisterdotcom)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@shubham1206agra
Copy link
Contributor

@tgolen @twisterdotcom This seems to be still happening according to #49948 (comment). Can anyone check?

@tgolen
Copy link
Contributor Author

tgolen commented Nov 13, 2024

Anyone should be able to check and see if they can reproduce it. @shubham1206agra can you give it a try first?

@tgolen
Copy link
Contributor Author

tgolen commented Nov 13, 2024

OK, I followed the links (so many trails to follow) and I ended up at a Slack thread which talks about multiple OpenApp requests, which is different than this (only OpenReport).

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2024
@twisterdotcom
Copy link
Contributor

Okay, let's pay out for this OpenReport fix.

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2024
@twisterdotcom
Copy link
Contributor

Just need to pay out the two offers.

@melvin-bot melvin-bot bot added the Overdue label Nov 18, 2024
Copy link

melvin-bot bot commented Nov 18, 2024

@tgolen, @twisterdotcom, @c3024, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@twisterdotcom
Copy link
Contributor

@nkdengineer please accept the upwork offer. I will pay out when I'm going through my upwork inbox at some point this week.

@JmillsExpensify
Copy link

$125 approved for @bernhardoj

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants