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-10-10] [HOLD for payment 2023-10-09] [$500] Chat - Error appears on click of notification and does not redirect to page #28328

Closed
2 of 6 tasks
kbecciv opened this issue Sep 27, 2023 · 43 comments
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

@kbecciv
Copy link

kbecciv commented Sep 27, 2023

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. From userA, send message to userB
  2. From userB, click on the notification

Expected Result:

It should redirect to userA chat

Actual Result:

Nothing happens on click of notification and console error appears (ROUTES__WEBPACK_IMPORTED_MODULE_13_.default.getReportRoute is not a function)

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 -requested
  • MacOS / Desktop

Version Number: Dev 1.3.74-2
Reproducible in staging?: n
Reproducible in production?: n
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-09-26.at.11.58.34.AM.1.mov
Screen.Recording.2023-09-28.at.11.06.03.AM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01d1b259b2d9260fc0
  • Upwork Job ID: 1707070231351926784
  • Last Price Increase: 2023-09-27
  • Automatic offers:
    • s77rt | Reviewer | 26983371
    • gadhiyamanan | Reporter | 26983373
@kbecciv kbecciv added the External Added to denote the issue can be worked on by a contributor label Sep 27, 2023
@melvin-bot melvin-bot bot changed the title DEV: Chat - Error appears on click of notification and does not redirect to page [$500] DEV: Chat - Error appears on click of notification and does not redirect to page Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

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

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 27, 2023
@redpanda-bit
Copy link
Contributor

redpanda-bit commented Sep 27, 2023

Proposal

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

Error shows in console after tapping notification and does not navigate as expected.

What is the root cause of that problem?

The notification is using a function that is no longer there.

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

The function was moved from ROUTES.getReportRoute(reportID) to ROUTES.REPORT_WITH_ID.getRoute(reportID)

App/src/libs/actions/Report.js

Lines 1652 to 1656 in 13b2291

const notificationParams = {
report,
reportAction,
onClick: () => Navigation.navigate(ROUTES.getReportRoute(reportID)),
};

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@Charan-hs
Copy link
Contributor

Charan-hs commented Sep 28, 2023

Proposal

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

DEV: Chat - Error appears on click of notification and does not redirect to page

What is the root cause of that problem?

This is a regression from this PR #28050
In this PR refactored ROUTES.ts here and failed to update here

onClick: () => Navigation.navigate(ROUTES.getReportRoute(reportID)),

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

just we have to update here

onClick: () => Navigation.navigate(ROUTES.getReportRoute(reportID)),

like this

- onClick: () => Navigation.navigate(ROUTES.getReportRoute(reportID))
+ onClick: () => Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(reportID))

What alternative solutions did you explore? (Optional)

@cubuspl42
Copy link
Contributor

@redpanda-bit

The function was moved from ROUTES.REPORT_WITH_ID.getRoute(reportID) toROUTES.getReportRoute(reportID)

I'm having trouble understanding what's the proposed solution. Could you try rephrasing it?

@redpanda-bit
Copy link
Contributor

TL;DR: The function ROUTES.getReportRoute(reportID) no longer exists and the function calls to this function need to be updated to ROUTES.REPORT_WITH_ID.getRoute(reportID)

Thanks for asking for clarification!

It's pretty much the same as @Charan-hs 's proposal. I just had the changes (from -> to) backwards because of rushing to be the first proposal. I think both proposals achieve the exact same result.

@redpanda-bit
Copy link
Contributor

Proposal

Updated

@kbecciv kbecciv added DeployBlockerCash This issue or pull request should block deployment and removed External Added to denote the issue can be worked on by a contributor Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 29, 2023
@kbecciv kbecciv changed the title [$500] DEV: Chat - Error appears on click of notification and does not redirect to page [$500] Chat - Error appears on click of notification and does not redirect to page Sep 29, 2023
@kbecciv
Copy link
Author

kbecciv commented Sep 29, 2023

Issue in staging now, changing status of report to Deploy Blocker

@OSBotify
Copy link
Contributor

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

melvin-bot bot commented Sep 29, 2023

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

@s77rt
Copy link
Contributor

s77rt commented Sep 29, 2023

@redpanda-bit's proposal looks good to me. Please raise a PR asap

@redpanda-bit
Copy link
Contributor

On it @s77rt.

redpanda-bit added a commit to redpanda-bit/Expensify-App that referenced this issue Sep 29, 2023
@situchan
Copy link
Contributor

This is regression from #27299, not from #28050.
Especially this change:
https://github.com/Expensify/App/pull/27299/files#diff-8afe5b71ee0436c21364148c86dadd640f2bff3e3d916addbb1f1f6f7e5b6a43R1654

@redpanda-bit
Copy link
Contributor

Going through PR checklist still.

@redpanda-bit
Copy link
Contributor

Is anyone available to help me test the platforms I am missing?

  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop

Or is it ok this time since the change is only a one liner?

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.75-12 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-10-09. 🎊

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

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

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 Oct 2, 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:

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

@s77rt
Copy link
Contributor

s77rt commented Oct 2, 2023

@s77rt
Copy link
Contributor

s77rt commented Oct 2, 2023

@cubuspl42 Sorry I didn't mean to take the C+ role. Just wanted to push this since it was a deploy blocker. Please apply in the Upwork job instead.

@cubuspl42
Copy link
Contributor

@s77rt No need; you did the actual job. If it was a deploy blocker, that was the right way to proceed.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 3, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-10-09] [$500] Chat - Error appears on click of notification and does not redirect to page [HOLD for payment 2023-10-10] [HOLD for payment 2023-10-09] [$500] Chat - Error appears on click of notification and does not redirect to page Oct 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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

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

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

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 Oct 3, 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:

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

@s77rt
Copy link
Contributor

s77rt commented Oct 3, 2023

Checklist filled already in #28328 (comment)

@flaviadefaria
Copy link
Contributor

Switching this to daily so that I can issue payment this week.

@flaviadefaria flaviadefaria added Daily KSv2 and removed Weekly KSv2 labels Oct 9, 2023
@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Oct 9, 2023
@flaviadefaria
Copy link
Contributor

Payment Summary:

For reference, here are some details about the assignees on this issue:
@s77rt requires payment offer (Reviewer)
@redpanda-bit requires payment
@gadhiyamanan requires payment offer (Reporter)

@s77rt requires - $500 - 50%(regression) = $250
@redpanda-bit - $500 - 50%(regression) = $250
@gadhiyamana - $50 (reporter)

@s77rt
Copy link
Contributor

s77rt commented Oct 11, 2023

@flaviadefaria This did not cause a regression (it was a regression from another PR). No penalties here.

@flaviadefaria
Copy link
Contributor

Thanks @s77rt, here's the updated Payment Summary:

For reference, here are some details about the assignees on this issue:
@s77rt requires payment offer (Reviewer)
@redpanda-bit requires payment
@gadhiyamanan requires payment offer (Reporter)

@s77rt requires - $500
@redpanda-bit - $500
@gadhiyamana - $50 (reporter)

Issuing payment now.

@flaviadefaria
Copy link
Contributor

@s77rt requires - offer sent
@redpanda-bit - please share your UW profile link since I can't find you to send an offer
@gadhiyamana - $50 (reporter) - paid

@redpanda-bit
Copy link
Contributor

@flaviadefaria here it is https://www.upwork.com/freelancers/~010e9950f7b5f0f0eb.

@flaviadefaria
Copy link
Contributor

@redpanda-bit offer sent!

@flaviadefaria
Copy link
Contributor

@s77rt requires - offer sent
@redpanda-bit - paid
@gadhiyamana - paid

@s77rt please accept the offer so that I can pay you and close this GH.

@s77rt
Copy link
Contributor

s77rt commented Oct 12, 2023

@flaviadefaria No payment is due for me here. Just wanted to resolve the deploy blocker. This can be closed.

@flaviadefaria
Copy link
Contributor

Ah, thanks for clarifying - closing!

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

10 participants