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.14k stars 2.64k forks source link

Group name - RBR doesn't disappear from LHN after removing error message #42765

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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!


Version Number: 1.4.77-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/41826

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Create a group chat
  3. Go offline
  4. Rename the group to something invalid for example "adoadoadoajdoajdoadoajdoajadoadoadoajdoajdoadoajdoajadoadoadoajdoajdoadoajdoajadoadoadoajdoajdoadoaỏ"
  5. Go online
  6. Follow the RBR and remove the error message

Expected Result:

RBR on LHN gets removed from LHN as well

Actual Result:

RBR stays on LHN even after removing error message

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/6e75cea5-a83b-4fde-b247-7bb71a228f5d

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @Nodebrute
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @danieldoglas (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: 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.
lanitochka17 commented 1 month ago

@danieldoglas FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 month ago

We think that this bug might be related to #vip-vsp

Nodebrute commented 1 month ago

Proposal

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

RBR doesn't disappear from LHN after removing error message

What is the root cause of that problem?

Here we call clearPolicyRoomNameErrors https://github.com/Expensify/App/blob/53f9e576808a7e6839c4d1f70a86242e4fcb9f10/src/pages/settings/Report/ReportSettingsPage.tsx#L76 and in clearPolicyRoomNameErrors we only clear errorFields but not errors https://github.com/Expensify/App/blob/c244fa19b0c53f99042281660d2275f17b07003c/src/libs/actions/Report.ts#L2220-L2229 and here when we fail we set errors to common.genericErrorMessage https://github.com/Expensify/App/blob/169a3aa3af1bc6f9b01976bb6132246fb892c87d/src/libs/actions/Report.ts#L665-L667

So the RCA is when "x" is pressed we clear errorFields but not errors

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

Solution 1 Instead of setting errors here set "errorFields" error to invalid Name https://github.com/Expensify/App/blob/169a3aa3af1bc6f9b01976bb6132246fb892c87d/src/libs/actions/Report.ts#L665-L667

Note:we can keep errors here too.

What alternative solutions did you explore? (Optional)

In clearPolicyRoomNameErrors also clear errors or create a new function to use here https://github.com/Expensify/App/blob/c244fa19b0c53f99042281660d2275f17b07003c/src/libs/actions/Report.ts#L2220-L2229

        errors: {
                    reportName: 'null',
                },
puneetlath commented 1 month ago

Doesn't seem blocker-worthy to me. Let's treat as a regular bug.

melvin-bot[bot] commented 1 month ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @muttmuure (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.

muttmuure commented 1 month ago

Catching up from OOO

muttmuure commented 1 month ago

Not overdue

Nodebrute commented 1 month ago

@rojiphil bump

rojiphil commented 1 month ago

Will review tomorrow my day and share update

rojiphil commented 1 month ago

Instead of setting errors here set "errorFields" error to invalid Name

@Nodebrute Can you please explain why the errorFields need to be explicitly set? Wouldn't this be set by BE?

melvin-bot[bot] commented 1 month ago

@rojiphil, @danieldoglas, @muttmuure Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 month ago

@rojiphil @danieldoglas @muttmuure this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Nodebrute commented 1 month ago

@rojiphil The backend is setting errorFields

Screenshot 2024-06-12 at 11 11 24 PM

In optimisticData we are setting errorFields to null https://github.com/Expensify/App/blob/d1d9dfa305c0eec107566cfef3a7d794ac146819/src/libs/actions/Report.ts#L632-L634 and even in clearPolicyRoomNameErrors we are setting errorFields to null https://github.com/Expensify/App/blob/d1d9dfa305c0eec107566cfef3a7d794ac146819/src/libs/actions/Report.ts#L2228-L2230 but here in failure data we are setting errors https://github.com/Expensify/App/blob/d1d9dfa305c0eec107566cfef3a7d794ac146819/src/libs/actions/Report.ts#L656-L658

So when we press 'x' clearPolicyRoomNameErrors is called. It sets the errorFields to null but errors stays there. So in below code block we should also set errorFields instead of errors https://github.com/Expensify/App/blob/169a3aa3af1bc6f9b01976bb6132246fb892c87d/src/libs/actions/Report.ts#L665-L667

rojiphil commented 1 month ago

Thanks for the details and the proposal. Your alternate solution (i.e. clearing errors on clearPolicyRoomNameErrors) is good enough to resolve the problem. @Nodebrute proposal LGTM 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @danieldoglas is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

danieldoglas commented 1 month ago

Assigned

danieldoglas commented 1 week ago

Assigning @mountiny to see the end of this while I'm OOO

melvin-bot[bot] commented 4 days ago

This issue has not been updated in over 15 days. @rojiphil, @danieldoglas, @mountiny, @muttmuure, @Nodebrute eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

mountiny commented 6 hours ago

@Nodebrute @rojiphil lets try to wrap the PR up

Nodebrute commented 1 hour ago

Waiting for @rojiphil's response.