[HOLD for payment 2024-04-09] [$500] Collect admin - Admin badge is not crossed out when removing admin offline

[lanitochka17]

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

---

Version Number: 1.4.47-2
Reproducible in staging?: Y
Reproducible in production?: Y
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

• User is the owner of Collect workspace
• The Collect workspace has admin

1. Launch New Expensify app
2. Go to workspace settings > Collect workspace
3. Go to Members
4. Go offline
5. Select an admin
6. Tap on the dropdown > Remove member
7. Remove the admin

Expected Result:

The admin badge will be crossed out (web behavior)

Actual Result:

The admin badge is not crossed out

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a65f30dbfbc53bd4
• Upwork Job ID: 1765131584509988864
• Last Price Increase: 2024-03-12
• Automatic offers:
• mkhutornyi | Contributor | 0
• apeyada | Contributor | 0

[melvin-bot]

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

[lanitochka17]

@anmurali 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]

We think that this bug might be related to #wave8
CC @zanyrenney

[Krishna2323]

Proposal

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

Collect admin - Admin badge is not crossed out when removing admin offline

What is the root cause of that problem?

We don't have any check for if the pending action for a user is policyMember.pendingAction === 'delete' and we don't apply isPendingRemove && styles.lineThrough to badge textStyles.

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 316 in e3b10f7

textStyles={styles.textStrong}

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

Check for const isPendingRemove = policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE; and apply isPendingRemove && styles.lineThrough to badge textStyles.

We will also look up for similar bugs in other components.

Result

admin_badge_line_through.mp4

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a65f30dbfbc53bd4

[melvin-bot]

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

[apeyada]

Proposal

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

Admin badge is not crossed out when removing admin offline
(This is only native bug. Works fine on web)

What is the root cause of that problem?

Each item row is already wrapped with OfflineWithFeedback in BaseListItem so cross out is supposed to work.
But only doesn't work on badge.
This happens because strikethrough style from OfflineWithFeedback isn't applied to Text in Badge.

App/src/components/Badge.tsx

Lines 56 to 61 in ec64dc1

	<Text
	style={[styles.badgeText, textColorStyles, textStyles]}
	numberOfLines={1}
	>
	{text}
	</Text>

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

Option1: add style to Text

     style={[styles.badgeText, textColorStyles, textStyles, style]} 

Option2: rename textStyles to style

[tienifr]

Proposal

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

The admin badge is not crossed out

What is the root cause of that problem?

In here, we already attempted to apply strikethrough text style for all nested children of OfflineWithFeedback.tsx

This works well in case of direct children like here, it will have style appended with the strike through style here. That's why the display name and email of the workspace member have strike-through style correctly.

However, the children recursive search logic will not work in case the Text is nested inside another custom component like the Badge here. In this case, when iterating here, the children of Badge element will be undefined, because children only works for elements directly exposed inside the children of OfflineWithFeedback.tsx. This leads to the Text (a child in Badge structure) not having the strike-through style applied.

And this issue will happen again any time we have a Text that's deeply nested inside OfflineWithFeedback, a very common use case.

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

The challenge is: Apply strike-through style to all Text components inside OfflineWithFeedback.

We need to use a more robust approach to do this than recursive search on children, due to the limitation above.

Fortunately, we already have a similar app-wide pattern which is the useTheme that can be done similarly for this case:

1. Wrap the children of OfflineWithFeedback here inside a CustomStyleContext.Provider (similar to how we use a ThemeProvider), this provider will provide the strikethrough-related style as the value. This can be used for any use cases where we want all sub-components to follow the same styles, just like this case where we want all Text inside OfflineWithFeedback to have strikethrough style.
2. create a useCustomStyle hook, similar to the useTheme hook, that returns the style defined in the provider, if there's no Provider, simply return undefined/empty object
3. In Text here, use the useCustomStyle hook and adds the style gotten from there to componentStyle

This pattern is similar to the theme pattern we're already using and is much more robust than the recursive-children approach currently being used. And any use case of we want all sub-components to follow the same styles can use this same provider and hook.

What alternative solutions did you explore? (Optional)

NA

[wildan-m]

Proposal

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

Collect admin - Admin badge is not crossed out when removing admin offline

What is the root cause of that problem?

The strikethrough style from OfflineWithFeedback is not applied to text in the badge.

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

If we want to give a badge special condition in OfflineWithFeedback, we can change const props: StrikethroughProps

To

                const style = StyleUtils.combineStyles(child.props.style, styles.offlineFeedback.deleted, styles.userSelectNone)
                const isChildBadge = isBadge(child);
                const props: StrikethroughProps = {
                    style: isChildBadge ? undefined : style,
                    textStyle: isChildBadge ? style : undefined,
                };

What alternative solutions did you explore? (Optional)

[c3024]

Crossing of the Admin badge works fine on web but not on native.

Substituting the Badge component with the returning node

<View/Banner><Text>....</Text></View>

of Badge at roleBadge in WorkspaceMembersPage works correctly on native as well.

Can we identify the exact RCA for why using Badge affects and it affects only for native?

[agent3bood]

Proposal

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

Collect admin - Admin badge is not crossed out when removing admin offline

What is the root cause of that problem?

Each item in the members list is wrapped by OfflineWithFeedback which will apply the line cross through to style prop here but the Badge is using passing textStyles prop to the Text.
Web works because if you apply a css higher in the tree it will be applied to the children, but React Native does not do that instead it strictly apply styles to individual views.

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

In the OfflineWithFeedback.applyStrikeThrough we need to add extra checks and apply the styles correctly.

    if (child.type.displayName === 'Badge') {
        props.textStyles = StyleUtils.combineStyles(child.props.textStyles, styles.offlineFeedback.deleted, styles.userSelectNone);
    }

This is more flexible and can accommodate adding new components that will need to be styled.

What alternative solutions did you explore? (Optional)

Use more generic check when applying style in OfflineWithFeedback.applyStrikeThrough

if (child.props.textStyles) {
    props.textStyles = StyleUtils.combineStyles(child.props.textStyles, styles.offlineFeedback.deleted, styles.userSelectNone);
}

[brandonhenry]

@c3024

Proposal

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

The admin badge is not being crossed out when removing an admin while offline in the native mobile apps. This badge strikethrough works as expected on web.

What is the root cause of that problem?

In the OfflineWithFeedback component, the applyStrikeThrough method is responsible for applying the strikethrough style to the child components when a deletion is pending:

[Link code]

This method recursively loops through the child components, and for each valid React element, it combines the existing style prop with the strikethrough styles (styles.offlineFeedback.deleted and styles.userSelectNone) using StyleUtils.combineStyles. It then clones the child element with the updated style prop.

If the child component has children of its own, applyStrikeThrough is called recursively on those children to apply the strikethrough style to them as well.

However, the issue with the Badge component lies here:

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

The strikethrough styles are being applied to the style prop of the child component. But in the Badge component, the Text component (which is the actual child that needs the strikethrough) is not directly receiving these styles. Instead, the styles are being applied to the Badge container .

On web, styles are applied using CSS, and by default, CSS styles cascade down to child elements. In the web version of the OfflineWithFeedback component, the strikethrough style is applied to Badge container parent element that wraps the entire member row. Because of the cascading nature of CSS, this style automatically applies to all child elements, including the Text inside the Badge.

However, React Native styles work differently. In React Native, styles are applied directly to individual components and don't automatically cascade to child components. Each component needs to be explicitly styled.

In the native version of OfflineWithFeedback, the applyStrikeThrough method recursively loops through the child elements, finds Text components, and applies the strikethrough style directly to them:

This works for the member's display name and login because they are direct Text children of OfflineWithFeedback. However, the Text inside Badge is not a direct child, so it doesn't get the strikethrough style applied to it.

To summarize:

• On web, the strikethrough style cascades automatically to all child elements due to the nature of CSS.
• On native, the strikethrough style is explicitly applied to direct Text children of OfflineWithFeedback, but not to the Text inside Badge because it's not a direct child.

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

Instead of blindly applying the strikethrough style to the style prop, we should make the OfflineWithFeedback component more flexible and allow it to apply the style to different props based on the component type.

1. Modify the OfflineWithFeedback component to accept a new prop strikethroughPropName that specifies which prop should receive the strikethrough style.

2. In the applyStrikeThrough method, instead of applying the style to the style prop, apply it to the prop specified by strikethroughPropName.

3. For the Badge component, when rendering it inside OfflineWithFeedback, pass "textStyles" as the strikethroughPropName prop.

4. In the Badge component, apply the received textStyles prop to the inner Text component.

This solution has the following advantages:

1. It keeps the strikethrough logic centralized within the OfflineWithFeedback component, making it easier to maintain and reason about.

2. It provides flexibility to handle different component types that may require the strikethrough style to be applied to different props.

3. It doesn't require introducing new context providers or higher-order components, which can add complexity and potential performance overhead.

4. It follows the existing pattern of passing props down the component tree, making it more straightforward to understand and extend.

With this approach, whenever a new component needs to support the strikethrough style in a different way, we can simply pass the appropriate strikethroughPropName prop when rendering it inside OfflineWithFeedback.

[c3024]

@mkhutornyi will takeover this issue as C+ as they worked on a similar issue and have better context.

[mkhutornyi]

taking this over

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

[brandonhenry]

Cc. @mkhutornyi

[mkhutornyi]

@tienifr does your context solution avoid this kind of issue on native, even if we have custom style prop but not inheriting style from OfflineWithFeedback?

[mkhutornyi]

Inject styles.offlineFeedback.deleted style manually to Text

This is existing pattern throughout the app. We fixed case-by-case, not generally solved.
i.e. #36224

So, while @apeyada proposed working solution first, this time I'm looking for general solution to prevent such bugs further on native.
@tienifr I'd like to test your branch with context approach.
@brandonhenry looks like both main solution and alternative solution are similar to previous proposals.

[rlinoz]

@mkhutornyi @apeyada if you could continue the checklist.

[melvin-bot]

@rlinoz, @anmurali, @mkhutornyi, @apeyada Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rlinoz]

friendly bump @mkhutornyi @apeyada

[melvin-bot]

@rlinoz, @anmurali, @mkhutornyi, @apeyada Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[anmurali]

Still waiting on the checklist here. cc @mkhutornyi @apeyada

[rlinoz]

@mkhutornyi is OOO

[rlinoz]

I am moving to weekly until @mkhutornyi is back.

[anmurali]

I paid @apeyada while we wait for @mkhutornyi to return and complete the checklist.

[rlinoz]

Still waiting for @mkhutornyi

[mkhutornyi]

Regression Test Proposal

• User is the owner of Collect workspace
• The Collect workspace has admin

1. Launch New Expensify app
2. Go to workspace settings > Collect workspace
3. Go to Members
4. Go offline
5. Select an admin
6. Tap on the dropdown > Remove member
7. Remove the admin
8. Verify that the admin badge is crossed out

[anmurali]

@mkhutornyi has been paid and tests added.