[PAID] [$500] Workspace - Deleted Workspace offline is not crossed out

[kbecciv]

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

---

Issue found when executing PR #17618

Action Performed:

1. Go to staging.new.expensify.com
2. Log in with any account
3. Go to Setting - Workspace
4. Create a new Workspace
5. Disable the internet
6. Delete the Workspace

Expected Result:

Deleted Workspace offline is crossed out

Actual Result:

Deleted Workspace offline is not crossed out

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

Version Number: 1.3.1.3

Reproducible in staging?: Yes

Reproducible in production?: No

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

17618.iOS.mp4

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/~018ed51b0bbc8584dc
• Upwork Job ID: 1648765595714977792
• Last Price Increase: 2023-04-19

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[kbecciv]

@strepanier03 Issue is not reproduced in Production, I did not use the Deploy Blocker label since this issue coming from PR listed above.

[mountiny]

Weird indeed @akinwale this seems to also stem from the revert, its probably some combination of another change. Do you want to have a look at this and incorporate it into the PR we have open so the design of the menu item is correct when worskapce is deleted?

[akinwale]

Weird indeed @akinwale this seems to also stem from the revert, its probably some combination of another change. Do you want to have a look at this and incorporate it into the PR we have open so the design of the menu item is correct when worskapce is deleted?

Never mind my previous comment, didn't initially realise this is a separate issue. I'll look into it.

[akinwale]

@mountiny This is also related to a change to MenuItem in this commit: a453d1e.

Initially, titleTextStyle in the MenuItem component was accepting props.style as part of the style combinations. OfflineWithFeedback updates the style prop of the component with the appropriate style for strikethrough.

App/src/components/OfflineWithFeedback.js

Line 76 in b6f6f6a

const props = {style: StyleUtils.combineStyles(child.props.style, styles.offlineFeedback.deleted, styles.userSelectNone)};

Since the titleTextStyle no longer has props.style combined with the rest of the applicable styles, strikethrough never gets applied to the text in the menu item.

To fix this, I have to modify the applyStrikethrough method in OfflineWithFeedback to also check for a titleStyle prop. The changes would go like so:

let props = {style: StyleUtils.combineStyles(child.props.style, styles.offlineFeedback.deleted, styles.userSelectNone)};
if (child.props.titleStyle) {
    props = {...props, titleStyle: StyleUtils.combineStyles(child.props.titleStyle, styles.offlineFeedback.deleted)};
}

Alternatively, I could add a boolean prop to MenuItem (something like inOfflineState) which will be used to conditionally apply the styles.offlineFeedback.deleted style to titleTextStyle. Then update the applyStrikeThrough method to set the prop to true on the child components.

Both approaches would ultimately require modifying OfflineWithFeedback. Let me know which is preferred.

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~018ed51b0bbc8584dc

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

Upwork job price has been updated to $500

[MelvinBot]

📣 @akinwale You have been assigned to this job by @mountiny!
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 📖

[mountiny]

Thanks, I have exported this with slightly lower price tag since its just a small styling issue. Assigned you and please address it in the PR (I saw you already did it). Thanks @akinwale

@eVoloshchak Could you please chime in what would be the best way to fix this in your opinion, thank you!

[strepanier03]

Thanks for moving this one along @mountiny, I'll continue following along.

[eVoloshchak]

Oh, ok, thanks for linking the comment. No problem, let's fix both issues in one PR

[MelvinBot]

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

[MelvinBot]

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

• fix: when deleting a workspace offline, the layout breaks #17639

If no regressions arise, payment will be issued on 2023-05-03. 🎊

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 - Interal report, not applicable for bug report bonus
• Contributor that fixed the issue - @akinwale - $500
• Contributor+ that helped on the issue and/or PR - @eVoloshchak - $500

Speed bonus analysis: PR submitted on April 18 / PR merged on April 24 = 4 business days not eligible.

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

[MelvinBot]

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:

• [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR: Add Push-to-Page support for IOUConfirmationList #16919
• [@eVoloshchak] 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: Add Push-to-Page support for IOUConfirmationList #16919 (comment)
• [@eVoloshchak] 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: I don't think an additional discussion is needed, this is a visual bug, we already have the following item in reviewer checklist: I turned off my network connection and tested it while offline to ensure it matches the expected behavior
• [@eVoloshchak] Determine if we should create a regression test for this bug.: Update existing test with additional step.
• [@eVoloshchak] 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.
• [@strepanier03] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/281514

[akinwale]

@mountiny @strepanier03 Checking in here as I haven't been hired on the Upwork job yet. Thanks.

[strepanier03]

I'm back from out of office starting today and am working on catching up with this now. I'll update shortly.

[strepanier03]

@eVoloshchak and @akinwale - I have hired you both in Upwork for this job.

---

Eugene, if you can finish up the BugZero checklist, I can update the comment and do the reg test GH if needed.

---

I'll check back in later this afternoon to pay what I can.

[akinwale]

@mountiny @eVoloshchak The point I raised concerning the applyStrikeThrough method in OfflineWithFeedback still needs to be filed as a separate issue. Shall I post it in Slack?

[strepanier03]

@akinwale - I have paid you via Upwork and ended the contract, thank you again!

[akinwale]

@akinwale - I have paid you via Upwork and ended the contract, thank you again!

Got it, thanks! Is this issue eligible for the speed bonus?

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Add Push-to-Page support for IOUConfirmationList #16919

• 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: Add Push-to-Page support for IOUConfirmationList #16919 (comment)

• 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: I don't think an additional discussion is needed, this is a visual bug, we already have the following item in reviewer checklist: I turned off my network connection and tested it while offline to ensure it matches the expected behavior

• Determine if we should create a regression test for this bug. We already have a test called Delete Workspace (Online/Offline), I think it should just be expanded with Offline section. We can add a step 10 In offline, verify that deleted workspace name is crossed out

[akinwale]

@akinwale - I have paid you via Upwork and ended the contract, thank you again!

Got it, thanks! Is this issue eligible for the speed bonus?

@mountiny Just checking to see if this would be eligible for a speed bonus, since it was fixed alongside the other issue with a single PR. Not sure how that would work in this instance. Thanks.

[strepanier03]

@mountiny, I did my normal analysis for the speed bonus and didn't think this was eligible. Below is was my review for the speed bonus:

Speed bonus analysis: PR submitted on April 18 / PR merged on April 24 = 4 business days not eligible.

Is there anything I'm missing with this that means we should make an exception and should pay the bonus?

[mountiny]

I agree not here, it was also a partially regression although hard to catch. I think we are good to go here. I am there will be many more PRs to pay for everyone, thanks fo ryou hard work!

[strepanier03]

Great, thank you @mountiny - I have paid this out and checklist is done so closing.

Thank you everyone!