Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

[HOLD for payment 2023-06-05] [$1000] Assign task- 404 error in the console appears when create a task #19134

Closed mvtglobally closed 1 year ago

mvtglobally commented 1 year ago

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 URL https://staging.new.expensify.com/
  2. Login
  3. Go to any #announce
  4. Create a task

Expected Result:

No errors in console

Actual Result:

404 error in the console appears

Workaround:

unknown

Platforms:

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

Version Number: v1.3.15-4 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: Any additional supporting documentation

Bug6058823_Untitled

https://github.com/Expensify/App/assets/44479856/5fb0d74a-b670-4f99-ae2b-4233ac98669d

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/~012e1b763b4e7b70a1
  • Upwork Job ID: 1658927033624666112
  • Last Price Increase: 2023-05-19
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

conorpendergrast commented 1 year ago

Yep, reproduced!

image
conorpendergrast commented 1 year ago

@thienlnam I think this should be over to you! I'll label it Internal to get the ball rolling, and will also then assign you too

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~012e1b763b4e7b70a1

conorpendergrast commented 1 year ago

If I'm wrong, let me know!

melvin-bot[bot] commented 1 year ago

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

thienlnam commented 1 year ago

The error is with AuthenticatePusher, not sure why that is showing up. The EditTask command should now be on production and so that error shouldn't be happening anymore

thienlnam commented 1 year ago

I think we can have this external for someone to investigate

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @laurenreidexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

phuchoang23 commented 1 year ago

Proposal

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

Assign task- 404 error in the console appears when create a task

What is the root cause of that problem?

404 error is come from this API to authorize Pusher when our App need to subscribe to a socket channel.

Screenshot 2023-05-20 at 16 03 44

I suspect the API to create Task (+ report) is not finished yet, then we navigate to taskReport in this line The ReportScreen will load and it triggers this line to subscribe to private socket channels of the report https://github.com/Expensify/App/blob/737862677811acc2b05c68209a56ecd58bee59f3/src/pages/home/report/ReportActionsView.js#L230-L233 => Can cause 404 error because the create Task API is not finished yet (report is not created yet)

Screenshots when navigate to a newly chatReport and a newly taskReport - **newly taskReport** Screenshot 2023-05-20 at 14 19 10 - **newly chatReport** Screenshot 2023-05-20 at 16 20 20

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

To solve this problem we need to ensure the taskReport is created successfully before subscribe to its private channels

More details: We need to follow this condition: https://github.com/Expensify/App/blob/737862677811acc2b05c68209a56ecd58bee59f3/src/pages/home/report/ReportActionsView.js#L225-L229 That means when build optimisticTaskReport data, we need to add pendingFields.createChat with CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD (and isOptimisticReport: true so it won't trigger OpenReport API when it's not ready).

                pendingFields: {
                    createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
                },
                isOptimisticReport: true,

And reset those fields in successData.

We used this trick in some other places:

Result:

Screenshot 2023-05-20 at 16 10 17
conorpendergrast commented 1 year ago

Unassigning @laurenreidexpensify as I'm already assigned as BugZero

thesahindia commented 1 year ago

I don't think I will be able to review this today. I have too much on my plate at the moment. @conorpendergrast, can you please reassign?

conorpendergrast commented 1 year ago

Asked in Slack

melvin-bot[bot] commented 1 year ago

πŸ“£ @aimane-chnaif You have been assigned to this job by @conorpendergrast! 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 πŸ“–

aimane-chnaif commented 1 year ago

@phuchoang23 thanks for the detailed explanation. Proposal looks good to me. So task report is built separately from other reports and below line was easily missed while building optimistic data.

                pendingFields: {
                    createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
                },

With this root cause, I found another bug of chat not blurred when created offline.

Before:

https://github.com/Expensify/App/assets/96077027/a6d6a8cc-51bd-4ca3-bfef-328efc7ec959

After:

Screenshot 2023-05-23 at 12 07 41 PM

We can fix them all together.

cc: @thienlnam πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

conorpendergrast commented 1 year ago

@thienlnam Over to you to review and confirm the proposal!

thienlnam commented 1 year ago

Awesome - looks great thanks!

melvin-bot[bot] commented 1 year ago

πŸ“£ @phuchoang23 You have been assigned to this job by @thienlnam! 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 πŸ“–

phuchoang23 commented 1 year ago

PR will ready in a couple hour

phuchoang23 commented 1 year ago

@aimane-chnaif @thienlnam PR is ready to review

phuchoang23 commented 1 year ago

@aimane-chnaif @thienlnam Btw I have signed the CLA but I'm still getting this error

Screen Shot 2023-05-24 at 9 36 08 PM
conorpendergrast commented 1 year ago

Checking that in our internal Slack channel

aimane-chnaif commented 1 year ago

I think it will disappear after re-trigger

conorpendergrast commented 1 year ago

Agreed; re-running the test now

conorpendergrast commented 1 year ago

Yay

image
phuchoang23 commented 1 year ago

look fine now. Thanks for supporting

phuchoang23 commented 1 year ago

@aimane-chnaif @thienlnam bump for code review

conorpendergrast commented 1 year ago

PR reviewed and on Staging

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

If no regressions arise, payment will be issued on 2023-06-05. :confetti_ball:

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.

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

melvin-bot[bot] commented 1 year ago

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:

conorpendergrast commented 1 year ago

Argh, Upwork isn't letting me hire on the existing job. I'll have to create new ones - tomorrow!

conorpendergrast commented 1 year ago

Looks like this was merged in less than 3 business days, so adding a 50% urgency bonus for C and C+

conorpendergrast commented 1 year ago

@aimane-chnaif contract sent via Upwork @phuchoang23 Can you reply here with your Upwork profile URL please?

phuchoang23 commented 1 year ago

@conorpendergrast Here is my Upwork profile https://www.upwork.com/freelancers/~01ae621d43ddc5931a

phuchoang23 commented 1 year ago

@conorpendergrast Did you send contract to me via Upwork?

conorpendergrast commented 1 year ago

I will shortly send the contract, yes!

conorpendergrast commented 1 year ago

@aimane-chnaif paid @phuchoang23 contract sent!

phuchoang23 commented 1 year ago

@conorpendergrast I accepted the offer

conorpendergrast commented 1 year ago

@phuchoang23 Paid!

conorpendergrast commented 1 year ago

Alright, just the BugZero checklist here

phuchoang23 commented 1 year ago

@conorpendergrast Thanks πŸ‘

conorpendergrast commented 1 year ago

@aimane-chnaif Bump on the BugZero checklist here please!

aimane-chnaif commented 1 year ago

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:

  • [x] [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/17992
  • [x] [@aimane-chnaif] 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/17992/files#r1224672801
  • [x] [@aimane-chnaif] 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: N/A
  • [x] [@aimane-chnaif] Determine if we should create a regression test for this bug.
  • [x] [@aimane-chnaif] 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.

This bug was introduced while implementing new feature of create task. This should have been caught earlier in PR review if they tested offline case. As offline test is already in PR checklist, no further regression test step is needed.

conorpendergrast commented 1 year ago

Got it. Sounds like we're all done. Closing, thanks all!