[HOLD for payment 2024-05-22] Hold expense - Thread header for system message is different from the actual system message #41202

izarutskaya · 2024-04-29T11:51:01Z

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.67-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Go to staging.new.expensify.com
Submit two expenses to a user.
Go to transaction thread of any expense.
Click 3-dot menu > Hold expense.
Enter a reason and save it.
Right click on the hold expense system message > Reply in thread.

Expected Result:

The thread header message will be consistent with the system message.

Actual Result:

The thread header shows "held this money request", while the system message is "held this expense".

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6464774_1714364776428.bandicam_2024-04-29_12-23-13-201.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01de18ac2a60287419
Upwork Job ID: 1786066158847115264
Last Price Increase: 2024-05-02

Issue Owner

Current Issue Owner: @miljakljajic

melvin-bot · 2024-04-29T11:51:10Z

Triggered auto assignment to @miljakljajic (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.

izarutskaya · 2024-04-29T11:51:19Z

We think this issue might be related to the #collect project.

dragnoir · 2024-04-29T13:35:33Z

BE issue.

bernhardoj · 2024-04-30T11:59:18Z

Proposal

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

The thread header title for hold system message is different than the message.

What is the root cause of that problem?

For a thread, the header title will take the parent action message. The hold system message is "held this money request" as shown below

But when we render the hold system message, we render a custom message, that is "held this expense".

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

Lines 584 to 589 in e0c12a5

    
           } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) { 
        
               children = <ReportActionItemBasicMessage message={translate('iou.heldExpense')} />; 
        
           } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD_COMMENT) { 
        
               children = <ReportActionItemBasicMessage message={action.message?.[0]?.text ?? ''} />; 
        
           } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.UNHOLD) { 
        
               children = <ReportActionItemBasicMessage message={translate('iou.unheldExpense')} />;

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

We have 2 options,

First, in ReportUtils.getReportName, if it's a hold action, then we show the custom message ("held this expense").

App/src/libs/ReportUtils.ts

Lines 3057 to 3092 in e0c12a5

    
           if (isChatThread(report)) { 
        
               if (!isEmptyObject(parentReportAction) && ReportActionsUtils.isTransactionThread(parentReportAction)) { 
        
                   formattedName = getTransactionReportName(parentReportAction); 
        
                   if (isArchivedRoom(report)) { 
        
                       formattedName += ` (${Localize.translateLocal('common.archived')})`; 
        
                   } 
        
                   return formattedName; 
        
               } 
        
               if (parentReportAction?.message?.[0]?.isDeletedParentAction) { 
        
                   return Localize.translateLocal('parentReportAction.deletedMessage'); 
        
               } 
        
               const isAttachment = ReportActionsUtils.isReportActionAttachment(!isEmptyObject(parentReportAction) ? parentReportAction : null); 
        
               const parentReportActionMessage = getReportActionMessage(parentReportAction, report?.parentReportID).replace(/(\r\n|\n|\r)/gm, ' '); 
        
               if (isAttachment && parentReportActionMessage) { 
        
                   return `[${Localize.translateLocal('common.attachment')}]`; 
        
               } 
        
               if ( 
        
                   parentReportAction?.message?.[0]?.moderationDecision?.decision === CONST.MODERATION.MODERATOR_DECISION_PENDING_HIDE || 
        
                   parentReportAction?.message?.[0]?.moderationDecision?.decision === CONST.MODERATION.MODERATOR_DECISION_HIDDEN || 
        
                   parentReportAction?.message?.[0]?.moderationDecision?.decision === CONST.MODERATION.MODERATOR_DECISION_PENDING_REMOVE 
        
               ) { 
        
                   return Localize.translateLocal('parentReportAction.hiddenMessage'); 
        
               } 
        
               if (isAdminRoom(report) || isUserCreatedPolicyRoom(report)) { 
        
                   return getAdminRoomInvitedParticipants(parentReportAction, parentReportActionMessage); 
        
               } 
        
               if (parentReportActionMessage && isArchivedRoom(report)) { 
        
                   return `${parentReportActionMessage} (${Localize.translateLocal('common.archived')})`; 
        
               } 
        
               if (ReportActionsUtils.isModifiedExpenseAction(parentReportAction)) { 
        
                   return ModifiedExpenseMessage.getForReportAction(report?.reportID, parentReportAction); 
        
               } 
        
               return parentReportActionMessage; 
        
           }

if (parentReportAction.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) {
    return Localize.translateLocal('iou.heldExpense')
}

OR

Show the original message of the hold action, just like what we did for HOLD_COMMENT.

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

Lines 584 to 589 in e0c12a5

    
           } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) { 
        
               children = <ReportActionItemBasicMessage message={translate('iou.heldExpense')} />; 
        
           } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD_COMMENT) { 
        
               children = <ReportActionItemBasicMessage message={action.message?.[0]?.text ?? ''} />; 
        
           } else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.UNHOLD) { 
        
               children = <ReportActionItemBasicMessage message={translate('iou.unheldExpense')} />;

Depends on the team which message to use.

we need to fix for unhold action too

melvin-bot · 2024-05-02T16:12:12Z

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

melvin-bot · 2024-05-02T16:12:17Z

Triggered auto assignment to Contributor Plus for review of internal employee PR - @Ollyws (Internal)

miljakljajic · 2024-05-02T16:12:31Z

Added the relevant labels

Ollyws · 2024-05-06T10:40:05Z

Seems that replying in thread has been disabled for the hold system message rendering this issue un-reproducable.

bernhardoj · 2024-05-06T10:47:33Z

It's not disabled because it's a system message, but because it's detected as a deleted action. A hold action has an empty text/html, so it's detected as a deleted action

I think we should make an exception for HOLD action

App/src/libs/ReportUtils.ts

Lines 6134 to 6136 in 942cad9

    
           function shouldDisableThread(reportAction: OnyxEntry<ReportAction>, reportID: string): boolean { 
        
               const isSplitBillAction = ReportActionsUtils.isSplitBillAction(reportAction); 
        
               const isDeletedAction = ReportActionsUtils.isDeletedAction(reportAction);

or let BE return a text for HOLD action as we did optimistically.

App/src/libs/ReportUtils.ts

Lines 4427 to 4437 in 942cad9

    
           function buildOptimisticHoldReportAction(created = DateUtils.getDBTime()): OptimisticHoldReportAction { 
        
               return { 
        
                   reportActionID: NumberUtils.rand64(), 
        
                   actionName: CONST.REPORT.ACTIONS.TYPE.HOLD, 
        
                   pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, 
        
                   actorAccountID: currentUserAccountID, 
        
                   message: [ 
        
                       { 
        
                           type: CONST.REPORT.MESSAGE.TYPE.TEXT, 
        
                           style: 'normal', 
        
                           text: Localize.translateLocal('iou.heldExpense'),

Ollyws · 2024-05-06T11:07:01Z

@bernhardoj Are you suggesting this was unintentional and that replying in threads should be enabled for the HOLD action?

bernhardoj · 2024-05-06T11:08:34Z

yes

Ollyws · 2024-05-06T11:25:23Z

Yeah I agree we shouldn't be considering the HOLD message as a deleted request, and it's better that we use iou.heldExpense so they're congruent and also because the backend message will not translate to Spanish otherwise.

@bernhardoj's proposal LGTM.

🎀👀🎀 C+ reviewed

melvin-bot · 2024-05-06T11:25:34Z

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

neil-marcellini · 2024-05-08T16:40:37Z

Yeah I agree we shouldn't be considering the HOLD message as a deleted request, and it's better that we use iou.heldExpense so they're congruent and also because the backend message will not translate to Spanish otherwise.

@bernhardoj's proposal LGTM.

🎀👀🎀 C+ reviewed

Sounds good to me. @bernhardoj if you would please update your original proposal with the fix for not considering HOLD messages as deleted, that would be good for people looking at the proposal in the future. I've heard that now we have a deleted key on deleted messages, so that might be an alternative. Please start on a PR.

bernhardoj · 2024-05-09T09:15:41Z

PR is ready

I see that after we reload the page (retriggers the OpenReport), the BE returns the message for the HOLD/UNHOLD action, so I don't include the code to make an exception for HOLD/UNHOLD action in isDeletedAction code. I think we should fix the HoldRequest and UnHoldRequest requests to return the correct action message.

I've heard that now we have a deleted key on deleted messages, so that might be an alternative.

Yes, we already use it but we still check for the text and html of message as the legacy way.

App/src/libs/ReportActionsUtils.ts

Lines 109 to 112 in 807c945

    
           // A legacy deleted comment has either an empty array or an object with html field with empty string as value 
        
           const isLegacyDeletedComment = message.length === 0 || message[0]?.html === ''; 
        
           return isLegacyDeletedComment || !!message[0]?.deleted;

For legacy support, we're still hiding ADDCOMMENT actions that have an empty message.

Based on the PR comment that implements it, we want to use the legacy way for ADDCOMMENT action, so maybe we should start doing that (on a separate issue).

cc: @Ollyws @neil-marcellini

melvin-bot · 2024-05-15T09:41:40Z

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

melvin-bot · 2024-05-15T09:41:44Z

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

Fix thread header for hold/unhold system message is different from the actual system message #41884

If no regressions arise, payment will be issued on 2024-05-22. 🎊

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

@Ollyws requires payment (Needs manual offer from BZ)
@bernhardoj requires payment (Needs manual offer from BZ)

melvin-bot · 2024-05-15T09:41:45Z

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:

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

melvin-bot · 2024-05-22T18:01:42Z

Payment Summary

Upwork Job

ROLE: @Ollyws paid $(AMOUNT) via Upwork (LINK)
ROLE: @bernhardoj paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@miljakljajic)

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

miljakljajic · 2024-05-22T18:28:07Z

Contracts extended to you both!

Ollyws · 2024-05-22T18:51:57Z

Requested payment in ND for this one.

Ollyws · 2024-05-23T11:35:33Z

BugZero Checklist:

The PR that introduced the bug has been identified. Link to the PR:

#38884

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:

#38884 (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:

N/A

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.

Regression Test Proposal

1. Submit a new expense to anyone
2. Open the transaction thread and put it on hold
3. Unhold the expense
4. There is currently a BE bug, so refresh/reopen the transaction thread
5. Right-click or long-press the hold/unhold system message
6. Choose Reply in thread
7. Verify the thread title for hold system message is "held this expense"
8. Verify the thread title for unhold system message is "unheld this expense"

Do we agree 👍 or 👎

neil-marcellini · 2024-05-23T16:55:34Z

I don't think we really need a regression test for this one, at least not until we fix the backend bug you mentioned.

miljakljajic · 2024-05-27T21:05:51Z

Payment summary: @Ollyws is owed 500 USD for their work on this issue

miljakljajic · 2024-05-27T21:07:12Z

Bernard has been paid

Ollyws · 2024-05-27T21:37:45Z

Payment summary: @Ollyws is owed 500 USD for their work on this issue

@miljakljajic Was this one a $500 not $250?

JmillsExpensify · 2024-05-28T17:49:09Z

Reached out to Milja via DM to confirm.

izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 29, 2024

melvin-bot bot assigned miljakljajic Apr 29, 2024

melvin-bot bot added the Overdue label May 1, 2024

miljakljajic added the Internal Requires API changes or must be handled by Expensify staff label May 2, 2024

melvin-bot bot assigned Ollyws May 2, 2024

melvin-bot bot removed the Overdue label May 2, 2024

melvin-bot bot added the Overdue label May 6, 2024

melvin-bot bot removed the Overdue label May 6, 2024

melvin-bot bot assigned neil-marcellini May 6, 2024

neil-marcellini assigned bernhardoj May 8, 2024

bernhardoj mentioned this issue May 9, 2024

Fix thread header for hold/unhold system message is different from the actual system message #41884

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels May 9, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 15, 2024

melvin-bot bot changed the title ~~Hold expense - Thread header for system message is different from the actual system message~~ [HOLD for payment 2024-05-22] Hold expense - Thread header for system message is different from the actual system message May 15, 2024

melvin-bot bot removed the Reviewing Has a PR in review label May 15, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 21, 2024

Ollyws mentioned this issue May 23, 2024

Fix - Add BA thread is missing Add button, incomplete thread header and "Hidden" user #38884

Merged

50 tasks

melvin-bot bot added the Overdue label May 24, 2024

melvin-bot bot removed the Overdue label May 27, 2024

miljakljajic closed this as completed May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-05-22] Hold expense - Thread header for system message is different from the actual system message #41202

[HOLD for payment 2024-05-22] Hold expense - Thread header for system message is different from the actual system message #41202

izarutskaya commented Apr 29, 2024 •

edited by melvin-bot bot

melvin-bot bot commented Apr 29, 2024

izarutskaya commented Apr 29, 2024

dragnoir commented Apr 29, 2024

bernhardoj commented Apr 30, 2024

melvin-bot bot commented May 2, 2024

melvin-bot bot commented May 2, 2024

miljakljajic commented May 2, 2024

Ollyws commented May 6, 2024

bernhardoj commented May 6, 2024

Ollyws commented May 6, 2024

bernhardoj commented May 6, 2024

Ollyws commented May 6, 2024

melvin-bot bot commented May 6, 2024

neil-marcellini commented May 8, 2024

bernhardoj commented May 9, 2024

melvin-bot bot commented May 15, 2024

melvin-bot bot commented May 15, 2024

melvin-bot bot commented May 15, 2024

melvin-bot bot commented May 22, 2024

miljakljajic commented May 22, 2024

Ollyws commented May 22, 2024

Ollyws commented May 23, 2024

neil-marcellini commented May 23, 2024

miljakljajic commented May 27, 2024

miljakljajic commented May 27, 2024

Ollyws commented May 27, 2024 •

edited

JmillsExpensify commented May 28, 2024

[HOLD for payment 2024-05-22] Hold expense - Thread header for system message is different from the actual system message #41202

[HOLD for payment 2024-05-22] Hold expense - Thread header for system message is different from the actual system message #41202

Comments

izarutskaya commented Apr 29, 2024 • edited by melvin-bot bot

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Apr 29, 2024

izarutskaya commented Apr 29, 2024

dragnoir commented Apr 29, 2024

bernhardoj commented Apr 30, 2024

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?

melvin-bot bot commented May 2, 2024

melvin-bot bot commented May 2, 2024

miljakljajic commented May 2, 2024

Ollyws commented May 6, 2024

bernhardoj commented May 6, 2024

Ollyws commented May 6, 2024

bernhardoj commented May 6, 2024

Ollyws commented May 6, 2024

melvin-bot bot commented May 6, 2024

neil-marcellini commented May 8, 2024

bernhardoj commented May 9, 2024

melvin-bot bot commented May 15, 2024

melvin-bot bot commented May 15, 2024

melvin-bot bot commented May 15, 2024

melvin-bot bot commented May 22, 2024

Payment Summary

BugZero Checklist (@miljakljajic)

miljakljajic commented May 22, 2024

Ollyws commented May 22, 2024

Ollyws commented May 23, 2024

neil-marcellini commented May 23, 2024

miljakljajic commented May 27, 2024

miljakljajic commented May 27, 2024

Ollyws commented May 27, 2024 • edited

JmillsExpensify commented May 28, 2024

izarutskaya commented Apr 29, 2024 •

edited by melvin-bot bot

Ollyws commented May 27, 2024 •

edited