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

aldo-expensify commented 2 years 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!

The red dots showing the path of an error is interrupted

Action Performed:

Using an account that is a domain

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

https://user-images.githubusercontent.com/87341702/199128893-e4f819ab-7df2-428c-81c5-9ff939e46643.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[bot] commented 2 years ago

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

miljakljajic commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

chiragsalian commented 2 years ago

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 commented 2 years ago

Cool, yeah I was referring to the previous one.

aldo-expensify commented 2 years ago

PR ready for review

melvin-bot[bot] commented 2 years ago

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[bot] commented 2 years ago

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[bot] commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Which PR caused this bug?

My PR here - https://github.com/Expensify/App/pull/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 commented 2 years ago

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 commented 2 years ago

Yup that makes sense to me, thanks aldo 👍

aldo-expensify commented 2 years ago

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

aldo-expensify commented 2 years ago

Should we close this?

aldo-expensify commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Fixing red indicator: https://github.com/Expensify/App/pull/12491

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years 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:

[ ] @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:
[x] @miljakljajic Payment has been made to the issue reporter (if applicable)
[x] @miljakljajic Payment has been made to the contributor that fixed the issue (if applicable)
[x] @miljakljajic Payment has been made to the contributor+ that helped on the issue (if applicable)

melvin-bot[bot] commented 2 years ago

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

aldo-expensify commented 1 year ago

No overdue, the PR was merged yesterday

miljakljajic commented 1 year ago

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 commented 1 year ago

@miljakljajic we have to pay the C+ for the PR reviews, more info here: https://github.com/Expensify/App/pull/12329#issuecomment-1317500038

miljakljajic commented 1 year ago

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[bot] commented 1 year ago

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

dylanexpensify commented 1 year ago

paid this out

melvin-bot[bot] commented 1 year ago

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

https://github.com/Expensify/App/pull/12491

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

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[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:

[ ] [@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:

Expensify / App