[HOLD for payment 2024-03-07] MEDIUM: [$500] Chat-In offline, uploading attachment & deleting it, is not showing strike-through

[lanitochka17]

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

---

Version Number: 1.4.42
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:

1. Launch app
2. Go offline
3. Tap plus icon & select add attachment
4. Upload an image using camera
5. Long press the message " Uploading attachment "
6. Tap delete comment and confirm delete

Expected Result:

In offline, uploading attachment & deleting it, must show strike-through message

Actual Result:

In offline, uploading attachment & deleting it, is not showing strike-through message

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

Bug6381208_1708029869521.at.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011966647b4d15a032
• Upwork Job ID: 1758512787167080448
• Last Price Increase: 2024-02-16
• Automatic offers:
• abdulrahuman5196 | Reviewer | 0
• Krishna2323 | Contributor | 0

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp
CC @quinthar

[lanitochka17]

@bz 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

[Krishna2323]

Proposal

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

Chat-In offline, uploading attachment & deleting it, is not showing strike-through

What is the root cause of that problem?

We don't have any implementation to handle strike through for deleted attachment action in offline mode.

App/src/pages/home/report/ReportActionItemFragment.tsx

Lines 98 to 104 in b0bdf6b

	if (ReportUtils.isReportMessageAttachment(fragment)) {
	return (
	<AttachmentCommentFragment
	source={source}
	html={fragment.html ?? ''}
	addExtraMargin={!displayAsGroup}
	/>

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

We should add a prop styleAsDeleted inside AttachmentCommentFragment and if true we will wrap the html with <del></del> tag and then pass styleAsDeleted={!!(isOffline && isPendingDelete)} from ReportActionItemFragment to AttachmentCommentFragment. We do the same thing in TextCommentFragment.

Result

show_as_deleted.mp4

Alternative

We can use the TextCommentFragment below AttachmentCommentFragment to render the html but we have to introduce a new flag inside AttachmentCommentFragment to render as html if the comment is an attachment.

App/src/pages/home/report/ReportActionItemFragment.tsx

Lines 98 to 118 in bea6aaf

	if (ReportUtils.isReportMessageAttachment(fragment)) {
	return (
	<AttachmentCommentFragment
	source={source}
	html={fragment.html ?? ''}
	addExtraMargin={!displayAsGroup}
	/>
	);
	}
	return (
	<TextCommentFragment
	source={source}
	fragment={fragment}
	styleAsDeleted={!!(isOffline && isPendingDelete)}
	iouMessage={iouMessage}
	displayAsGroup={displayAsGroup}
	style={style}
	/>
	);
	}

[CortneyOfstad]

@lanitochka17 I think that this could be handled by external engineers, similar to the issue here. Going to get eyes on this 👍

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011966647b4d15a032

[melvin-bot]

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

[abdulrahuman5196]

Seems to be a new issue. Will check on proposals today.

[dukenv0307]

Proposal

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

In offline, uploading attachment & deleting it, is not showing strike-through message

What is the root cause of that problem?

We disabled strike-through here

App/src/pages/home/report/ReportActionItem.js

Line 773 in eeb6836

shouldDisableStrikeThrough

And we don't implement the deleted style for AttachmentCommentFragment

App/src/pages/home/report/ReportActionItemFragment.tsx

Line 104 in eeb6836

<AttachmentCommentFragment

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

We should render the attachment comment action as TextCommentFragment if it's uploading since this component already has covered the delete case and when the attachment is uploading the HTML is only a text

const isUploadingAttachment = ReportUtils.isReportMessageAttachment(fragment) && fragment.html === CONST.ATTACHMENT_UPLOADING_MESSAGE_HTML;


if (ReportUtils.isReportMessageAttachment(fragment) && !isUploadingAttachment) {
    return (
        <AttachmentCommentFragment
        
return (
                <TextCommentFragment
                    source={source}
                    fragment={!isUploadingAttachment ? fragment : {...fragment, text: fragment?.html ?? ''}}

What alternative solutions did you explore? (Optional)

NA

[CortneyOfstad]

@abdulrahuman5196 any updates on the reviews on the proposals? Thanks so much!

[abdulrahuman5196]

Reviewing now

[abdulrahuman5196]

@Krishna2323 's proposal here #36639 (comment) looks good and works well.

🎀 👀 🎀
C+ Reviewed

[melvin-bot]

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

[Beamanator]

Hmm we currently use OfflineWithFeedback to show the strikethrough for normal comments (if we delete them while offline)... Do we not currently use this for attachment report actions? If not, shouldn't we start using that? I don't see this component mentioned in any proposal so far, can someone first clarify this point?

[dukenv0307]

@Beamanator As I mentioned in my proposal, we are disable strike-through in ReportActionItem and apply it in child component TextCommentFragment. When we upload an attachment it's report attachment action so it is rendered by AttachmentCommentFragment and we missed the Uploading attachment ... case to apply strike-through in this.

We have two options:

1. Render it as TextCommentFragment if it is uploading
2. Implement deleted style in AttachmentCommentFragment

[Beamanator]

Ok I followed the code you're talking about @dukenv0307 - do you have any idea why we added shouldDisableStrikeThrough to OfflineWithFeedback in ReportActionItem.js? Maybe because there's too many child components, the strikethrough wasn't being rendered in the correct place?

Anyway, it seems a bit hacky to me that we added shouldDisableStrikeThrough in the past, but it is probably too much work to get that cleaned up at this exact moment for this fix

I'm honestly leaning a bit toward @dukenv0307 's proposal of using TextCommentFragment to render the strikethrough text in this case, because

1. It's already built & technically at this point we do only have text to display
2. AttachmentCommentFragment will probably never need a strikethrough in other cases (we wouldn't put a strikethrough in images / attachments i believe)

What do you think @abdulrahuman5196 ?

[Krishna2323]

@Beamanator, I already purposed about using TextComponentFragment in my alternative solution.
Due to this issue we add shouldDisableStrikeThrough .

cc: @abdulrahuman5196

[dukenv0307]

1. @Beamanator This is added to fix this issue [HOLD for payment 2023-09-29] [$1000] Offline: Deleting edited inlinecode block and codeblock results in extra hyphen (strikethrough) #17365. The proposal of this is here [HOLD for payment 2023-09-29] [$1000] Offline: Deleting edited inlinecode block and codeblock results in extra hyphen (strikethrough) #17365 (comment)
   [HOLD for payment 2023-09-29] [$1000] Offline: Deleting edited inlinecode block and codeblock results in extra hyphen (strikethrough) #17365 (comment)

I already purposed about using TextComponentFragment in my alternative solution.
Due to #17365 we add shouldDisableStrikeThrough .

We can use the TextCommentFragment below AttachmentCommentFragment to render the html but we have to introduce a new flag inside AttachmentCommentFragment to render as html if the comment is an attachment.

We should render the attachment comment action as TextCommentFragment if it's uploading since this component already has covered the delete case and when the attachment is uploading the HTML is only a text

2. It is not the same as what I proposed

[abdulrahuman5196]

Hi, @Beamanator , Sorry for not chiming in earlier.

Anyway, it seems a bit hacky to me that we added shouldDisableStrikeThrough in the past, but it is probably too much work to get that cleaned up at this exact moment for this fix

Yep. This was one of the reasons from my end to suggest the approved proposal.

Between Using AttachmentCommentFragment or TextCommentFragment

There where certain pointers for me go ahead with using AttachmentCommentFragment

1. IMO, as per the current code, we have clear responsibility split, like attachment messages are using AttachmentCommentFragment and others are using TextCommentFragment
2. It is relatively very simple to just add the styleAsDeleted check to AttachmentCommentFragment similar to how the TextCommentFragment has it now, which would solve this issue.
   

   App/src/pages/home/report/ReportActionItemFragment.tsx

   Lines 113 to 116 in eeb6836

   	<TextCommentFragment
   	source={source}
   	fragment={fragment}
   	styleAsDeleted={!!(isOffline && isPendingDelete)}

3. If we are to use AttachmentCommentFragment, then any specific logic present as part of that component should be moved or supported in TextCommentFragment if not present already(for eg: logic of addExtraMargin flag in AttachmentCommentFragment)

So if we use TextCommentFragment, we would be adding checks to see if the attachment is in upload state and then use the TextCommentFragment. This would just end up adding unwanted complexity specifically.

So IMO, at this point, it wouldn't be beneficial for us to migrate the usage for AttachmentCommentFragment to TextCommentFragment for this specific style issue.

[Beamanator]

@abdulrahuman5196 thanks so much for your thoughts & points, I think you convinced me here 👍 Lets move forward with implementing the change in AttachmentCommentFragment 👍

Also thanks @Krishna2323 and @dukenv0307 for discussing peacefully the different options & proposals

[melvin-bot]

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

[Krishna2323]

@abdulrahuman5196, PR ready for review 🙃

[melvin-bot]

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

[melvin-bot]

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

• fix: Chat-In offline, uploading attachment & deleting it, is not showing strike-through #37179

If no regressions arise, payment will be issued on 2024-03-07. 🎊

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

• @abdulrahuman5196 requires payment automatic offer (Reviewer)
• @Krishna2323 requires payment automatic offer (Contributor)

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

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

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

Not a regression. Seems to be case from beginning.

Determine if we should create a regression test for this bug.

Yes.

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.

1. Go offline
2. Open any report
3. Add any attachment
4. Delete it
5. Verify line-though is shown on Uploading attachment... text

cc: @CortneyOfstad

[Krishna2323]

@CortneyOfstad friendly bump for payments here :)

[CortneyOfstad]

@Krishna2323 sorry! I was OoO last week! Getting the payment completed now!

[CortneyOfstad]

Payment Summary

@Krishna2323 paid $500 via Upwork
@abdulrahuman5196 paid $500 via Upwork

[CortneyOfstad]

Regression test created here — https://github.com/Expensify/Expensify/issues/377645

Closing!