[HOLD for payment 2022-11-29] Redbrick path broken for Settings > Workspaces

[aldo-expensify]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

The red dots showing the path of an error is interrupted

Action Performed:

Using an account that is a domain

1. Create a workspace in New Dot
2. Go to old dot and set the workspace as the preferred policy of a domain's group
3. In new dot, try to delete the workspace, this will cause the API to throw an error
4. Check that the red bricks path is broken at the Settings level. The Workspaces entry should have a red dot.

Screen.Recording.2022-10-31.at.4.18.06.PM.mov

Expected Result:

The should have been a red dot next to the Workspaces entry in the Settings menu

Actual Result:

No red dot there

Workaround:

The user can use the app as normal, it is just bad feedback on where the error is.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[miljakljajic]

hey Aldo - I'm just going through your steps to try and reproduce, what does this mean?

Go offline with the incognito browser

How do I go offline on a specific browser window?

[trjExpensify]

I typically use like web/mobile, but you can open the JS console in the incognito browser > Network tab > then next to the wifi symbol switch the dropdown to Offline to follow the steps here:

[chiragsalian]

Once the PR is merged let's remember to also add regressions steps for QA to test this flow since I've told them to test RBR on settings -> workspaces if there is a member error. So it will be nice if they add this flow as well to their regression testing.

[JmillsExpensify]

+1000. Should we proactively open that issue back up?

[chiragsalian]

Which issue, the one to add QA regression steps? If so then no. I would rather create new issues to add QA steps to keep the process cleaner and easier to process through. Steps mentioned here.

[JmillsExpensify]

Cool, yeah I was referring to the previous one.

[aldo-expensify]

PR ready for review

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• The PR that introduced the bug has been identified. Link to the PR:
• 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:
• A discussion in #contributor-plus 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:
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• The PR that introduced the bug has been identified. Link to the PR:
• 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:
• A discussion in #contributor-plus 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:
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

• A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• The PR that introduced the bug has been identified. Link to the PR:
• 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:
• A discussion in #contributor-plus 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:
• Payment has been made to the issue reporter (if applicable)
• Payment has been made to the contributor that fixed the issue (if applicable)
• Payment has been made to the contributor+ that helped on the issue (if applicable)

[miljakljajic]

Hey - just kicking off the above checklist. Which PR caused this bug? What do you think we could have done differently in the original PR to prevent this bug from being introduced - and who is responsible for adding the appropriate test?

[chiragsalian]

Which PR caused this bug?

My PR here - #11784 caused the bug

What do you think we could have done differently in the original PR to prevent this bug from being introduced

Not sure, I missed testing for this usecase. There are a number of error usecases and while we did test for RBR I didn't realize this one would show up visually different.

and who is responsible for adding the appropriate test?

I think its best for @aldo-expensify to add this scenario to our regression tests since he found it. Is that alright with you aldo?

[aldo-expensify]

I think its best for @aldo-expensify to add this scenario to our regression tests since he found it. Is that alright with you aldo?

Sure! we already have a test that triggers an error at the level of the workspace: https://github.com/Expensify/Expensify/issues/233249#issuecomment-1282947636

I think we just have add step to it to verify that the red brick path is fine at each level.

do you think that makes sense?

[chiragsalian]

Yup that makes sense to me, thanks aldo 👍

[aldo-expensify]

Created issue requesting applause to update the test: https://github.com/Expensify/Expensify/issues/240986

[aldo-expensify]

Should we close this?

[aldo-expensify]

Hmm, just saw that this other PR https://github.com/Expensify/App/pull/12117/files from @Justicea83 fixed it the other way I mentioned here. This ended up making the workspace have an extra red dot here:

I think we should revert change because the dot there means to me that there will be an error when we go in that workspace.

[chiragsalian]

Should we close this?

It should auto-close when your PR hits production so nothing extra needs to be done here. If it doesn't close once your code is live then yeah we can manually close this.

I think we should revert change because the dot..

Oh sorry, I'm not sure what was the exact decision we landed on. I think it might be justice's PR because there was a long thread with a number of people pulled in here. Maybe you can ask there to confirm whats best.

But yes i think i agree with you that i would think either the error message shows below the workspace or we have red dot such that clicking into it will show the error. I don't think it should be both since that is confusing. Unless maybe he had a workspace and member error?

[aldo-expensify]

Unless maybe he had a workspace and member error?

Should we close this?

It should auto-close when your PR hits production so nothing extra needs to be done here. If it doesn't close once your code is live then yeah we can manually close this.

Lol, you are right

I don't think it should be both since that is confusing. Unless maybe he had a workspace and member error?

I agree. I'll ask in the slack you linked https://expensify.slack.com/archives/C01GTK53T8Q/p1666955764560879

[aldo-expensify]

Fixing red indicator: #12491

[melvin-bot]

@miljakljajic, @aldo-expensify Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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:

• @miljakljajic A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:
• @aldo-expensify The PR that introduced the bug has been identified. Link to the PR:
• @miljakljajic 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:
• @miljakljajic A discussion in #contributor-plus 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:
• @miljakljajic Payment has been made to the issue reporter (if applicable)
• @miljakljajic Payment has been made to the contributor that fixed the issue (if applicable)
• @miljakljajic Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot]

@miljakljajic, @aldo-expensify Still overdue 6 days?! Let's take care of this!

[aldo-expensify]

No overdue, the PR was merged yesterday

[miljakljajic]

Apologies for the hold up on the regression test but I am about to go OOO until Tuesday. I will put the tests in then. Since there are no external contributors on this issue we don't need to pay anyone.

[aldo-expensify]

@miljakljajic we have to pay the C+ for the PR reviews, more info here: #12329 (comment)

[miljakljajic]

Ah apologies I missed the linked PR. I have asked for help here but I haven't had an answer yet and I am OOO, I'll reassign so the C+ isn't waiting longer for payment.

[melvin-bot]

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

[dylanexpensify]

paid this out

[melvin-bot]

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

• Remove incorrect red brick indicator #12491

If no regressions arise, payment will be issued on 2022-11-29. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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]

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:

• [@aldo-expensify] The PR that introduced the bug has been identified. Link to the PR:
• [@aldo-expensify] 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:
• [@aldo-expensify] 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:
• [@dylanexpensify] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: