[HOLD for payment 2023-08-28] [$1000] When deleting a file, there is a space between the username and the '[Deleted message]' #24216

kavimuru · 2023-08-07T15:46:03Z

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:

Open a chat.
Send an attachment.
Click on 'Reply in thread.'
Delete the thread file.
Notice the space between username and delete message
Now check that after deleting a text message

Expected Result:

When deleting a file or text message, there should be consistent spacing between the username and the '[Deleted message]'

Actual Result:

When deleting a file, there is a space between the username and the '[Deleted message]' message, causing an inconsistency in spacing compared to deleting a text 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?

Version Number: 1.3.50-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-recording-2023-08-01-at-33634-pm_OkJMrkI5.1.mp4

Recording.5751.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019a73e78df922007d
Upwork Job ID: 1688865815884058624
Last Price Increase: 2023-08-08
Automatic offers:

cubuspl42 | Reviewer | 26111804
bernhardoj | Contributor | 26111806

namhihi237 · 2023-08-07T15:46:05Z

Proposal

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

When deleting a file or text message, there should be consistent spacing between the username and the '[Deleted message]'

What is the root cause of that problem?

Here is the difference between space after deleting text message and attachment
If it's an attachment, we pass styles.mt2 here

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

Line 330 in c3bafa6

!props.displayAsGroup && isAttachment ? styles.mt2 : undefined,

We have 2 cases:

Online => delete => show [Delete message]
Offline => delete => keep it intact => online => show [Deleted message]

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

Here we should check more if the attachment has been deleted or not

network: networkPropTypes.isRequired,

 const pendingAction = lodashGet(props.action, 'pendingAction', '');

!props.displayAsGroup &&
    isAttachment &&
    (!ReportActionsUtils.isDeletedAction(props.action) ||
        (props.network.isOffline && ReportActionsUtils.isDeletedAction(props.action) && pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)) ?
    styles.mt2 :
    undefined,

memo(
    ReportActionItem,
    (prevProps, nextProps) =>
    //...
    prevProps.network.isOffline === nextProps.network.isOffline,
),

Result:

Online

Screen.Recording.2023-08-08.at.00.36.26.mov

Offline

Screen.Recording.2023-08-08.at.00.38.29.mov

What alternative solutions did you explore? (Optional)

N/A

DanutGavrus · 2023-08-07T15:46:06Z

Proposal

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

When deleting a file, there is a space between the username and the '[Deleted message]'

What is the root cause of that problem?

In ReportActionItem, we set a top margin if the action is an attachment, but we do not verify that it was not deleted:

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

Line 330 in 4230f28

!props.displayAsGroup && isAttachment ? styles.mt2 : undefined,

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

We should also verify that:

!props.displayAsGroup && isAttachment && !ReportActionsUtils.isDeletedAction(props.action) ? styles.mt2 : undefined,

What alternative solutions did you explore?

If we want also want to keep the space while waiting for the attachment to be deleted, we should update to:

!props.displayAsGroup && isAttachment && (!ReportActionsUtils.isDeletedAction(props.action) || props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) ? styles.mt2 : undefined,

melvin-bot · 2023-08-07T15:46:08Z

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

melvin-bot · 2023-08-07T15:46:10Z

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

yh-0218 · 2023-08-07T17:46:14Z

Proposal

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

When deleting a file, there is a space between the username and the '[Deleted message]'

What is the root cause of that problem?

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

Line 330 in 8639fb6

!props.displayAsGroup && isAttachment ? styles.mt2 : undefined,

Because we have styles.mt2 for all attachments.

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

We have to remove styles.mt2 for only deleted attachments.
We can get deleted status from action ReportActionsUtils.isDeletedAction(props.action).

What alternative solutions did you explore? (Optional)

bernhardoj · 2023-08-08T04:58:22Z

Proposal

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

After deleting a thread message that is an attachment, the [Deleted Message] still has the margin value that is supposed to be for an attachment only.

What is the root cause of that problem?

When the message is an attachment, we apply a margin-top of 8.

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

Line 314 in 2ce4328

    
           const isAttachment = _.has(props.action, 'isAttachment') ? props.action.isAttachment : ReportUtils.isReportMessageAttachment(message);

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

Line 330 in 2ce4328

!props.displayAsGroup && isAttachment ? styles.mt2 : undefined,

Currently, we check if it's an attachment from the report action isAttachment and ReportUtils.isReportMessageAttachment(message)

isAttachment is a local onyx data that is set when we add a new action/message.

App/src/libs/ReportUtils.js

Line 1507 in 2ce4328

const isAttachment = _.isEmpty(text) && file !== undefined;

App/src/libs/ReportUtils.js

Line 1539 in 2ce4328

isAttachment,

This property is not available on the BE, so if you logout, isAttachment will be lost, and therefore the attachment check will rely on ReportUtils.isReportMessageAttachment(message).

The problem is, when we delete a message, we never set the isAttachment to false. So, the [Deleted message] text will still have the margin-top of 8.

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

Set isAttachment to false when we delete a message.

App/src/libs/actions/Report.js

Lines 882 to 890 in 2ce4328

    
           const optimisticReportActions = { 
        
               [reportActionID]: { 
        
                   pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, 
        
                   previousMessage: reportAction.message, 
        
                   message: deletedMessage, 
        
                   errors: null, 
        
                   linkMetadata: [], 
        
               }, 
        
           };

What alternative solutions did you explore? (Optional)

Remove props.action.isAttachment checks.

melvin-bot · 2023-08-08T10:52:45Z

Job added to Upwork: https://www.upwork.com/jobs/~019a73e78df922007d

melvin-bot · 2023-08-08T10:52:48Z

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

melvin-bot · 2023-08-08T10:52:54Z

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

trjExpensify · 2023-08-08T10:53:07Z

Reproducible, moving this on external! @cubuspl42 you're up! :)

cubuspl42 · 2023-08-08T10:57:49Z

@namhihi237 I think you're missing a # in your proposal text. Expensify relies heavily on automation, so it's best to stick to templates as closely as possible. Thank you for understanding.

Proposal

namhihi237 · 2023-08-08T11:05:58Z

@cubuspl42 thanks let me update

cubuspl42 · 2023-08-08T11:06:03Z

@yh-0218 Your proposal looks very similar to this one. Is there any additional value?

cubuspl42 · 2023-08-08T11:26:24Z

@bernhardoj I like the direction of your root cause analysis and your "alternative solutions" section.

Remove props.action.isAttachment checks.

Could you elaborate?

cubuspl42 · 2023-08-08T11:32:17Z

@DanutGavrus The essence of your proposal is similar to the one above. Yours is shorter because it skips reasoning about the offline/online state.

Could you comment on this? Are offline/online checks unnecessary in your opinion?

namhihi237 · 2023-08-08T11:34:37Z

I think an offline check is necessary because if no offline check we delete the image, it will remove mt2 even though the image is still showing.

DanutGavrus · 2023-08-08T11:46:02Z

@cubuspl42

Yours is shorter because it skips reasoning about the offline/online state.

I was originally thinking that ReportActionsUtils.isDeletedAction(props.action) wouldn't return true before the action was actually deleted, thus it wouldn't return true when deleting an action while offline. It seems I wasn't right.

I'm not sure about slow internet connections in which isOffline returns false, but the call may take quite long to execute. I'm thinking relying only on the pendingAction to be enough while we wait for the attachment to be deleted, regarding the online/offline state. Updated alternate Proposal with that approach if anything.

bernhardoj · 2023-08-08T13:08:19Z

Could you elaborate?

@cubuspl42 Yes, because the action.isAttachment is a local property, we can remove props.action.isAttachment from here

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

Line 314 in 2ce4328

    
           const isAttachment = _.has(props.action, 'isAttachment') ? props.action.isAttachment : ReportUtils.isReportMessageAttachment(message);

So, the condition will be

const isAttachment = ReportUtils.isReportMessageAttachment(message);

getusha · 2023-08-08T14:10:51Z

Proposal

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

There is a margin after deleting an attachment.

What is the root cause of that problem?

The external margin is only need for attachments that are not displayed as a group (for attachments that are the first message with avatar and name). Since the margin is applied to a component that renders every type of comment, it will also be applied to the '[Deleted message]' because the message is the first comment and not displayed as a group.

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

We should move the margin and conditions from the current place from here:

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

Line 330 in dd21a0b

!props.displayAsGroup && isAttachment ? styles.mt2 : undefined,

to:

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

Line 127 in fdcae9f

    
           return <RenderHTML html={props.source === 'email' ? `<email-comment>${htmlContent}</email-comment>` : `<comment>${htmlContent}</comment>`} />;

Since the ThumbnailImage will be rendered here, it's the appropriate place to add the margin. We can wrap it with a View and apply the margin based on the conditions.

<View style={[!props.displayAsGroup && props.isAttachment ? styles.mt2 : {}]}>
     <RenderHTML ... />
</View>

What alternative solutions did you explore? (Optional)

cubuspl42 · 2023-08-08T14:18:55Z

@bernhardoj

Yes, because the action.isAttachment is a local property, we can remove props.action.isAttachment from here

If the resulting code is valid, this would be definitely one of the best candidates for a solution. A change consisting solely of code removal is always tempting!

bernhardoj · 2023-08-08T14:20:56Z

A change consisting solely of code removal is always tempting!

totally agree. Sadly it won't cover the offline case #24216 (comment) 😞

cubuspl42 · 2023-08-08T19:24:13Z

@namhihi237 @DanutGavrus
You provided reasonable and working solutions, but the check for adding the mt2 style becomes complex and difficult to reason about, in my opinion.

@getusha
Thank you very much for exploring this other path! I hoped we could find a clear and neat solution by moving the style to the more inner component, but I don't think this particular proposal achieves that.

@bernhardoj
You focused your attention on the contract of the isAttachment property and the isReportMessageAttachment function, which indeed seems to be close to the essence of this problem.

The attachments have different representations at the time when they're uploaded on the frontend and when they return back from the backend (when they're quite opaquely HTML-encoded). isReportMessageAttachment could be considered a source of truth and isAttachment a local prediction of the future result of isReportMessageAttachment, but available before the backend response is known (also offline). isReportMessageAttachment is false for a deleted message, so it's a clear approach to also falsify isAttachment.

I think we should go with @bernhardoj's proposal, with some minor refactoring which should improve the clarity of the mentioned contracts
C+ reviewed 🎀 👀 🎀

melvin-bot · 2023-08-21T21:44:59Z

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

melvin-bot · 2023-08-21T21:45:02Z

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

Fix report action is still detected as attachment even after we delete it #24539

If no regressions arise, payment will be issued on 2023-08-28. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@cubuspl42 requires payment offer (Reviewer)
@bernhardoj requires payment offer (Contributor)
@ayazhussain79 requires payment

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 · 2023-08-21T21:45:04Z

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:

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

cubuspl42 · 2023-08-22T08:45:51Z

The PR that introduced the bug has been identified. Link to the PR:
- New Thread Report Empty State, Header, LHN, OpenReport, Buttons for DEV #18522
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/18522/files#r1301263459
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:
- No need for additional discussion
Determine if we should create a regression test for this bug.
- No need for a new regression test
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.
- N/A

trjExpensify · 2023-08-30T13:26:59Z

Hey everyone! Sorry for the delay, I've been out of office.

Confirming payments as follows:

@bernhardoj - $1,500 for the bug fix & #urgency bonus. - paid!
@cubuspl42 - $1,500 for the C+ review & #urgency bonus - paid!
@ayazhussain79 - $250 for the bug report. - offer sent.

ayazhussain79 · 2023-08-30T13:29:07Z

@trjExpensify offer accepted, Thank you

trjExpensify · 2023-08-30T13:35:11Z

Paid!

bernhardoj · 2023-08-30T14:19:20Z

@trjExpensify Oh no, I think we shouldn't be getting paid yet as we still have a regression here.

(I just realized we haven't linked the regression issue here, my bad)

cc: @cubuspl42

cubuspl42 · 2023-08-30T16:06:11Z

@trjExpensify It's my fault. Do you know how we can clean this up, or should I start a topic on Slack?

I also bumped the PRs that are blocking #25415, I hope we'll soon have this whole thing behind us

trjExpensify · 2023-08-30T16:10:57Z

Ah, whoops! I've made the payments on those. I think the easiest thing to do is to simply deduct from your next issue bounties. Do you have any in the works you can make a note on to reduce the amount due accordingly?

bernhardoj · 2023-08-30T16:14:39Z

@trjExpensify I have one that is currently pending for payment #23420

trjExpensify · 2023-08-30T17:45:34Z

Excellent, commented there.

cubuspl42 · 2023-08-31T07:36:00Z

@trjExpensify Here: #25514

trjExpensify · 2023-08-31T15:23:57Z

Thanks, commented!

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 7, 2023

melvin-bot bot assigned trjExpensify Aug 7, 2023

trjExpensify added the External Added to denote the issue can be worked on by a contributor label Aug 8, 2023

melvin-bot bot changed the title ~~When deleting a file, there is a space between the username and the '[Deleted message]'~~ [$1000] When deleting a file, there is a space between the username and the '[Deleted message]' Aug 8, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 8, 2023

melvin-bot bot assigned cubuspl42 Aug 8, 2023

melvin-bot bot assigned Li357 Aug 8, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Aug 21, 2023

This was referenced Aug 22, 2023

Remove unused attachment placeholder code #25557

Merged

New Thread Report Empty State, Header, LHN, OpenReport, Buttons for DEV #18522

Merged

melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 Daily KSv2 labels Aug 28, 2023

melvin-bot bot removed the Overdue label Aug 30, 2023

trjExpensify closed this as completed Aug 30, 2023

trjExpensify mentioned this issue Aug 30, 2023

[$1000] Money request page crash after sign in #23420

Closed

6 tasks

trjExpensify mentioned this issue Aug 31, 2023

[HOLD for payment 2023-09-04] [$1000] Reimburse - 0.5 is allowed, but .5 is not allowed #25514

Closed

6 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 2, 2023

cubuspl42 mentioned this issue Oct 11, 2023

Refactor comment message rendering #25524

Merged

55 tasks

[HOLD for payment 2023-08-28] [$1000] When deleting a file, there is a space between the username and the '[Deleted message]' #24216

[HOLD for payment 2023-08-28] [$1000] When deleting a file, there is a space between the username and the '[Deleted message]' #24216

Comments

kavimuru commented Aug 7, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

namhihi237 commented Aug 7, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

DanutGavrus commented Aug 7, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore?

melvin-bot bot commented Aug 7, 2023

melvin-bot bot commented Aug 7, 2023 • edited by trjExpensify Loading

Bug0 Triage Checklist (Main S/O)

yh-0218 commented Aug 7, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

bernhardoj commented Aug 8, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Aug 8, 2023

melvin-bot bot commented Aug 8, 2023

melvin-bot bot commented Aug 8, 2023

trjExpensify commented Aug 8, 2023

cubuspl42 commented Aug 8, 2023 • edited Loading

namhihi237 commented Aug 8, 2023

cubuspl42 commented Aug 8, 2023

cubuspl42 commented Aug 8, 2023

cubuspl42 commented Aug 8, 2023

namhihi237 commented Aug 8, 2023

DanutGavrus commented Aug 8, 2023 • edited Loading

bernhardoj commented Aug 8, 2023

getusha commented Aug 8, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

cubuspl42 commented Aug 8, 2023

bernhardoj commented Aug 8, 2023

cubuspl42 commented Aug 8, 2023

melvin-bot bot commented Aug 21, 2023

melvin-bot bot commented Aug 21, 2023 • edited by trjExpensify Loading

melvin-bot bot commented Aug 21, 2023

cubuspl42 commented Aug 22, 2023

trjExpensify commented Aug 30, 2023

ayazhussain79 commented Aug 30, 2023

trjExpensify commented Aug 30, 2023

bernhardoj commented Aug 30, 2023

cubuspl42 commented Aug 30, 2023

trjExpensify commented Aug 30, 2023

bernhardoj commented Aug 30, 2023

trjExpensify commented Aug 30, 2023

cubuspl42 commented Aug 31, 2023

trjExpensify commented Aug 31, 2023

kavimuru commented Aug 7, 2023 •

edited by melvin-bot bot

Loading

namhihi237 commented Aug 7, 2023 •

edited

Loading

DanutGavrus commented Aug 7, 2023 •

edited

Loading

melvin-bot bot commented Aug 7, 2023 •

edited by trjExpensify

Loading

cubuspl42 commented Aug 8, 2023 •

edited

Loading

DanutGavrus commented Aug 8, 2023 •

edited

Loading

getusha commented Aug 8, 2023 •

edited

Loading

melvin-bot bot commented Aug 21, 2023 •

edited by trjExpensify

Loading