[HOLD on regression test ] Workspace - Crash when removing a user offline in Manage members

[kbecciv]

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

---

Action Performed:

1. Access staging.new.expensify.com
2. Sign into any valid account
3. Tap on Profile > Workspace > Manage members
4. Invite any member
5. Turn off the internet connection
6. Remove a user from the workspace while offline

Expected Result:

User expects the name to get crossed out and then afterwards removed when the connection is reestablished.

Actual Result:

The app crashes

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.2.38.0

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5858097_Crash_after_removing_offline.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[melvin-bot]

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

[situchan]

Proposal

This is a regression from #13398, especially this change:

so crash happens on these 2 lines:

App/src/components/OptionRow.js

Lines 85 to 86 in 10dd188

	const displayNameStyle = [styles.optionDisplayName, ...textUnreadStyle, props.style];
	const alternateTextStyle = [textStyle, styles.optionAlternateText, styles.textLabelSupporting, props.style];

When offline, props.style is set like this:

So {} inside [] inside [] in style doesn't work and it crashes on web

Solution:
We should flatten those styles:

- const displayNameStyle = [styles.optionDisplayName, ...textUnreadStyle, props.style];
+ const displayNameStyle = StyleSheet.flatten([styles.optionDisplayName, ...textUnreadStyle, props.style]);
- const alternateTextStyle = [textStyle, styles.optionAlternateText, styles.textLabelSupporting, props.style];
+ const alternateTextStyle = StyleSheet.flatten([textStyle, styles.optionAlternateText, styles.textLabelSupporting, props.style]);

or we can use original code which uses StyleUtils.combineStyles:

- const displayNameStyle = [styles.optionDisplayName, ...textUnreadStyle, props.style];
+ const displayNameStyle = StyleUtils.combineStyles([styles.optionDisplayName, ...textUnreadStyle], props.style);
- const alternateTextStyle = [textStyle, styles.optionAlternateText, styles.textLabelSupporting, props.style];
+ const alternateTextStyle = StyleUtils.combineStyles([textStyle, styles.optionAlternateText, styles.textLabelSupporting], props.style);

[aldo-expensify]

@situchan thanks!, that indeed seems to be the cause and what you proposed fixes it. I wondering a bit now when do we choose to use StyleUtils.combineStyles and when StyleSheet.flatten. I tested the second and it also works and I see we are using a few lines below.

[aldo-expensify]

Ready for review: #13525

[chiragsalian]

The CP PR passed QA. Removing deploy blocker label.

[melvin-bot]

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

• Flatten styles to avoid crash #13525

If no regressions arise, payment will be issued on 2022-12-20. 🎊

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

[melvin-bot]

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

• Flatten styles to avoid crash #13525

If no regressions arise, payment will be issued on 2022-12-22. 🎊

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: Improve performance of optionslist #13398
• [@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: https://github.com/Expensify/App/pull/13398/files#r1050234584
• [@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:
• [@stephanieelliott] 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]

@kbecciv was this issue catch while running the test suite or doing QA?

[stephanieelliott]

Erm this was fixed internally, no payments needed here!

Will leave this open til we've agreed on how to handle the regression test -- I've started the discussion for the test case here: https://expensify.slack.com/archives/C01SKUP7QR0/p1671741994888809

[situchan]

I appreciate if I can get compensation here since I found the exact root cause and my solution was used.
cc: @aldo-expensify

[melvin-bot]

@stephanieelliott, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[stephanieelliott]

Hey @situchan, thanks for raising that point! Chatted with @aldo-expensify and we agree that your comment did help save us quite a bit of time with the investigation. I created an Upwork job to pay you out for this, can you please claim it when you get a chance? https://www.upwork.com/jobs/~01d6c0e9c64b2148e8

[situchan]

@stephanieelliott applied. Thank you

[situchan]

@stephanieelliott accepted your offer

[stephanieelliott]

Thanks @situchan, I've paid you in Upwork!

No regression test needed for this one (context here), so we can close this one out!