[HOLD for payment 2024-07-10] Android & iOS - Tags - "Enabled" badge is not crossed out when tag is deleted 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.76-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4578912 Issue reported by: Applause - Internal Team

Action Performed:

1. Launch New Expensify app
2. Go to workspace settings
3. Go to Tags
4. Add a tag if there is no tag
5. Go offline
6. Delete the tag

Expected Result:

The "Enabled" badge will be crossed out when tag is deleted offline (web behavior)

Actual Result:

The "Enabled" badge is not crossed out when tag is deleted offline This issue also applies to categories, distance rates and tax rates

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [x] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/dcd0e5b0-83c7-432e-8a93-9445718ef359

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @JmillsExpensify

[melvin-bot[bot]]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@JmillsExpensify 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 #wave-collect - Release 1

[Krishna2323]

Proposal

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

Android & iOS - Tags - "Enabled" badge is not crossed out when tag is deleted offline

What is the root cause of that problem?

The deleted style is only applied to direct child of OfflineWithFeedback, but in we are using ListItemRightCaretWithLabel for the label. https://github.com/Expensify/App/blob/63a20f4b3232809f4d9441b3e4ab60180ff591ae/src/components/SelectionList/BaseListItem.tsx#L58 https://github.com/Expensify/App/blob/63a20f4b3232809f4d9441b3e4ab60180ff591ae/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L102

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

We need to follow the same approach as we did in https://github.com/Expensify/App/pull/39010.

1. Get the style prop in ListItemRightCaretWithLabelProps.
2. Determine the value of isDeleted.

const isDeleted = style && Array.isArray(style) ? style.includes(styles.offlineFeedback.deleted) : false;

3. Use isDeleted to style the text correctly.

We will also look for similar components like ListItemRightCaretWithLabelProps.

What alternative solutions did you explore? (Optional)

Result

https://github.com/Expensify/App/assets/85894871/a40e7895-b74d-463c-bac3-5741ddb44f5d

[bernhardoj]

Proposal

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

Enabled badge of the right component is not crossed out when deleting a tag while offline.

What is the root cause of that problem?

The crossed out style is applied from OfflineWithFeedback. OfflineWithFeedback will give the component a deleted style. https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/components/OfflineWithFeedback.tsx#L116-L118

However, the Enabled badge component itself doesn't accept any style props. https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/components/SelectionList/ListItemRightCaretWithLabel.tsx#L14

So, the deleted style is never applied to it.

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

Add a style prop to ListItemRightCaretWithLabel and append it to the label text style here. https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/components/SelectionList/ListItemRightCaretWithLabel.tsx#L20

What alternative solutions did you explore? (Optional)

We can start passing an additional offlineFeedbackStyle prop from OfflineWithFeedback and use it on a custom component, in our case, the ListItemRightCaretWithLabel. Other components will still work as usual.

style: StyleUtils.combineStyles(childProps.style, styles.offlineFeedback.deleted, styles.userSelectNone),
offlineFeedbackStyle: StyleUtils.combineStyles(styles.offlineFeedback.deleted, styles.userSelectNone),

Then we accept offlineFeedbackStyle as the new prop of ListItemRightCaretWithLabel instead of style.

[melvin-bot[bot]]

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

[JmillsExpensify]

Opening up to the community.

[melvin-bot[bot]]

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

[tienifr]

My solution here is a general fix for issues like this, I'll post it again here in some time

[tienifr]

Bringing my proposal from https://github.com/Expensify/App/issues/37776 which is a general fix

Proposal

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

"Enabled" badge is not crossed out when tag is deleted offline

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

[melvin-bot[bot]]

@JmillsExpensify, @DylanDylann Eep! 4 days overdue now. Issues have feelings too...

[JmillsExpensify]

Still waiting for proposal review. cc @DylanDylann

[DylanDylann]

This issue only reproduces on the native app. I think the RCA will be more complicated than the above proposals

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.

@tienifr Thanks for this point. But it is still grey to me, I will need some time to dive deep into this problem. It would be great if you could detail more

[tienifr]

@DylanDylann Please read this thread for more details: https://stackoverflow.com/q/51776533

Basically, when we have:

<ComponentA>
    <ComponentB>
         <ComponentC/>
    </ComponentB>
</ComponentA>

And we try to access the children of ComponentB, we'll see ComponentC is a children

But when we do

<ComponentA>
    <ComponentB />
</ComponentA>

and

ComponentB = <View>
     <ComponentC/>
</View>

children of ComponentB will be empty, because it indeeds have no children (no elements within the <ComponentB> and </ComponentB> closing and ending tags)

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[melvin-bot[bot]]

@JmillsExpensify @DylanDylann this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

@JmillsExpensify, @DylanDylann Eep! 4 days overdue now. Issues have feelings too...

[DylanDylann]

@mvtglobally I still reproduce on IOS

[DylanDylann]

Update: I am still deep diving into @tienifr's proposal. This bug is specific to native device

[DylanDylann]

@Krishna2323 Your RCA isn't correct to me because we implement the recursive logic to apply strike-through to child components (read @tienifr's proposal for more detail)

[DylanDylann]

@bernhardoj Using the style prop of ListItemRightCaretWithLabel to apply to the Text child component seems like a workaround to me. The RCA here is that the recursive logic can't detect Text component as a child component so the strike-out style isn't applied to Text child component

@tienifr Your RCA is correct, we should apply a general fix to prevent similar bugs in the future. I would prefer @tienifr's proposal. Let's go with them

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-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/43622

If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @tienifr requires payment through NewDot Manual Requests
• @DylanDylann requires payment (Needs manual offer from BZ)

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

• [ ] [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@DylanDylann] 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:
• [ ] [@DylanDylann] 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:
• [ ] [@DylanDylann] Determine if we should create a regression test for this bug.
• [ ] [@DylanDylann] 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.
• [ ] [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot[bot]]

Payment Summary

Upwork Job

• Reviewer: @tienifr owed $250 via NewDot
• ROLE: @DylanDylann paid $250 via Upwork

BugZero Checklist (@JmillsExpensify)

• [x] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• [x] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• [x] I have paid out the Upwork contracts or cancelled the ones that are incorrect
• [x] I have verified the payment summary above is correct

[JmillsExpensify]

@tienifr mind filling out the BZ checklist? I confirm the $250 payment summary above is correct for both contributors.

[JmillsExpensify]

@DylanDylann shall I create an Upwork job/contract for you?

[DylanDylann]

@JmillsExpensify Yes please!

[melvin-bot[bot]]

@JmillsExpensify, @youssef-lr, @tienifr, @DylanDylann Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!