[$1000] reply text appears as strikethrough on delete parent message in offline mode

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. go to any chat
2. send any message
3. hover on that message and select reply in thread option
4. go to that thread report screen
5. send message in thread
6. go back to parent report screen
7. go offline
8. delete original message

   Expected Result:

   only message should be show as strikethrough text not the reply and counter text

   Actual Result:

   reply text appears as strikethrough on delete parent message

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.3.40-1 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/ee1164c1-66eb-435d-81c5-ef74a896f268

https://github.com/Expensify/App/assets/43996225/9f889048-0c8f-4a5b-902c-8b40c8a5604a

Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689249243007879

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01783ca3e14a37fff1
• Upwork Job ID: 1680963716129968128
• Last Price Increase: 2023-10-11

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Nodebrute]

Proposal

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

reply text appears as strikethrough on delete parent message in offline mode

What is the root cause of that problem?

In OfflineWithFeedback we are applying strikethrough to all children. In this case reply should not get strike through as we are only deleting parent here

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

in OfflineWithFeedback here

https://github.com/Expensify/App/blob/754b6edf175a10deb6c6876bdba13c2a99e0ffec/src/components/OfflineWithFeedback.js#L77

return early if children has childReportID. We can do something like this

 if(child.props.children?.props?.childReportID){
            return child;
        }

We can also do this for any other children that we don't want to show strikethrough on.

What alternative solutions did you explore? (Optional)

we can also do it like this

        if(child.props.children && child.props.children.props && child.props.children.props.childReportID){
            return child
        }

[Victor-Nyagudi]

I'm a little confused on the expected behavior here.

If the message is deleted, the reply below it shouldn't be struck through?

Shouldn't deleting the parent message also delete the replies?

[gadhiyamanan]

Shouldn't deleting the parent message also delete the replies?

no, it will not delete replies. you can check slack as well

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01783ca3e14a37fff1

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[kevinksullivan]

Reproduced ad sending to the pool

[melvin-bot[bot]]

📣 @muhammadkashaf! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[hoangzinh]

Proposal

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

reply text appears as strikethrough on delete parent message in offline mode

What is the root cause of that problem?

The mechanism of applying strike through function when there is a pending delete in Offline mode is: It goes through each React element and its children, then add additional the props.style with styles.offlineFeedback.deleted https://github.com/Expensify/App/blob/ab3d0e876e311ed1ce3cd859ad6dd77a5d8e3cdf/src/components/OfflineWithFeedback.js#L82-L86

If there is any children element that won't pick up the props.style and apply for on its style, it won't have affect of strikethrough. It explain why name of sender + time is not strike through affect (Because the ReportActionItemSingle will pick the props.wrapperStyles not the props.styles)

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

We can do the same how we pass props.wrapperStyles into ReportActionItemSingle for ReportActionItemThread component. More details:

• In component ReportActionItemThread, we introduce a new props called props.wrapperStyles and assign it here
• In component ReportActionItem, we need remove View element here. And passing style draftMessageRightAlign to the new prop that we added for ReportActionItemThread component above. Almost same here

Results:

What alternative solutions did you explore? (Optional)

In case we would like to remove grayed-out style for replies comment as well. I think the parent report message and child Thread replies should be displayed dependently. That means, in this case, the parent report message is strikethrough, but the child thread (replies) still display and click to view as usual. In order to to that, I have an idea:

• Firstly, for displaying, we need to move displaying thread replies out of OfflineWithFeedback of parent report message, and put it as sibling here, after this line => thus, the offline status of parent report message won't effect the Thread replies. (Ofc, we need to add margin styles for it as well, to make vertical align with previous messages).

• Secondly, for clickable on Thread replies when parent report message is pending delete, we need to update this line as well, to exclude the case it has Thread replies. I.e:

  pointerEvents={(props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !shouldDisplayThreadReplies) ? 'none' : 'auto'}

• Last by not least, we need to optimistic set childType to 'chat' for parent report action as well in this function. Otherwise, it will show emoji reaction in context menu when create parent message offline and delete it offline. I.e :

Results:

https://github.com/Expensify/App/assets/9639873/b4d2864f-4905-4785-93a1-12ab5eb23b29

[ShogunFire]

So what happen if we delete the replies offline ? Should the line with "x replies" be striked through ? I think we should pass a new parameter to applyStrikeThrough that dictate the type of child we want to strike through, otherwise it's going to be hard to handle every cases (I think I saw other issues with this method)

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot[bot]]

@kevinksullivan, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[fedirjh]

Will review it later today

[kevinksullivan]

Hi @fedirjh did you get a chance to review this last week?

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

@kevinksullivan @fedirjh 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]]

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

[fedirjh]

Missed this one , reviewing shortly.

[fedirjh]

@Nodebrute and @hoangzinh I think both of your proposal doesn’t cover that greyed out style. I believe if it's not strikethrough then it shouldn’t be greyed out as well.

[fedirjh]

So what happen if we delete the replies offline ? Should the line with "x replies" be striked through ?

@ShogunFire That’s a good question, but I think that should be handled separately. Currently, in both online and offline mode, the count updates optimistically when we delete thread replies, it decreases until it becomes empty. I think addressing that will require changing the logic of optimistic data and that may be complicated as it involves different logics.

[fedirjh]

Proposals are in review.

[fedirjh]

cc @hoangzinh @Nodebrute Little bump for https://github.com/Expensify/App/issues/22810#issuecomment-1654688121

[Nodebrute]

When we delete the parent message, the 'thread count text' becomes unselectable. In that case, I think it makes sense to grey it out.

[melvin-bot[bot]]

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

[fedirjh]

the 'thread count text' becomes unselectable

@Nodebrute Hmm why it becomes unselectable, I guess that style is from offlineFeedback. It makes sense to disable all the offlineFeedback style from thread count text , that's the expected behaviour for this issue. In other words , only the parent message should have offlineFeedback style.

[ShogunFire]

Proposal

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

When we delete the parent message offline, "x replies" is striked through and greyed out

What is the root cause of that problem?

We apply the strike through to every children and we apply the opacity style to the container view

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

We need a new way to apply the offline style that allows us to choose if a children should apply it or not, keeping in mind that if we apply to a parent all his descendants will look like they have the style and if we apply to both parent and child the opacity will be applied twice which makes the text darker.

We can use a new prop stopOfflineWithFeedBackStyle, if a component has this prop

• we don't apply the style to it.
• we don't apply the style to his children
• we don't apply the style to his ancestors

For all the other components the style will be applied but we need to apply the style only once, that's why we should only apply the style to the highest possible ancestor. To find the highest possible ancestor we need to find the ancestor that has a descendant that has the stopOfflineWithFeedBackStyle prop, ancestor will be the parent of the components that we should apply the style to. If we are at the root and no descendant has the prop stopOfflineWithFeedBackStyle we should apply the offline style to the root

The tree will look like this:

The code starts to be a little big so this is my commit: https://github.com/Expensify/App/commit/293c41f1f103bde6234238103977ae6a9d143824

Result:

[melvin-bot[bot]]

@kevinksullivan @fedirjh this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[kevinksullivan]

Hi @laurenreidexpensify I am going OOO for the week so tapping you in the interim. Thanks!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[laurenreidexpensify]

@sophiepintoraetz I am OOO from tomorrow afternoon for rest of week, so reassigning to keep this moving for Kev, thanks

[laurenreidexpensify]

@fedirjh bump on proposal review above pls! thanks

[melvin-bot[bot]]

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

[sophiepintoraetz]

Bumped @fedirjh in slack

[fedirjh]

@ShogunFire The proposed solution involving the childReportID doesn't appear to be a wise approach. What if there's another child component that also possesses a childReportID and requires strikethrough formatting? Can you enhance this solution to make it more versatile, considering the possibility of similar cases arising in the future?

---

cc @hoangzinh In case you missed https://github.com/Expensify/App/issues/22810#issuecomment-1654688121, do you think we can remove the grayed-out style with the wrapperStyles you mentioned?

[hoangzinh]

@fedirjh I'm still finding the solution for this requirement. Basically, for styling, we can. But I'm finding a way to make the thread replies clickable when the parent report message is pending to delete.

[ShogunFire]

Proposal

Updated

[melvin-bot[bot]]

@kevinksullivan @fedirjh @sophiepintoraetz this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @fedirjh is eligible for the Internal assigner, not assigning anyone new.

[sophiepintoraetz]

Just need a review from @fedirjh! And @kevinksullivan - I think you're back now so gracefully bowing out 👍

[fedirjh]

@ShogunFire I'm curious about why we're checking for child.props.children?.props?.stopOfflineWithFeedbackStyle instead of child.props.stopOfflineWithFeedbackStyle. FWIW, applyStrikeThrough is recursively applied to all children. Therefore, it seems logical to examine the props of each child unless I'm overlooking something.

https://github.com/Expensify/App/blob/63945f7f49503f48e384c6819448a9fa452ad43e/src/components/OfflineWithFeedback.js#L84

[ShogunFire]

@fedirjh You are right that this is strange but looking directly at child.props.stopOfflineWithFeedbackStyle doesn't work because the opacity of the parent div is applied to all his children. If we have:

<div1 opacity = 0.5>
    <div2 opacity = 1>
    </div>
</div>

Div2 will appear with 0.5 opacity :/

[hoangzinh]

@fedirjh I just updated my proposal https://github.com/Expensify/App/issues/22810#issuecomment-1639999466 to make the Thread replies displays as normal if the parent message is deleted in offline mode.

[ShogunFire]

Proposal

Updated

I wrote a better proposal, we can play with OfflineWithFeedback and NoOfflineStyleForChild to handle every cases. For example we could have another nested OfflineWithFeedback in ReportActionItemThread and another NoOfflineStyleForChild in it

[melvin-bot[bot]]

@kevinksullivan, @fedirjh Eep! 4 days overdue now. Issues have feelings too...

[kevinksullivan]

@fedirjh when will you be able to get to these reviews?