[hold for payment 2024-04-29] [$500] [Held requests] System message for hold request shows a comma as reason

[kbecciv]

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.54-1
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com.
2. Request money from any user.
3. Navigate to request details page.
4. Click 3-dot menu > Hold request.
5. Add a reason and save it.

Expected Result:

The system message for hold request will show the entered reason.

Actual Result:

The system message for hold request briefly shows the entered reason, then it changes to a comma.

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

Bug6419227_1710853518973.bandicam_2024-03-19_21-02-32-738.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b056a4fad087d390
• Upwork Job ID: 1770105583965515776
• Last Price Increase: 2024-03-19
• Automatic offers:
• mkhutornyi | Reviewer | 0

[melvin-bot]

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

[kbecciv]

@joekaufmanexpensify 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.

[kbecciv]

We think that this bug might be related to #wave-collect - Release 1

[shahinyan11]

Proposal

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

Hold request - System message for hold request shows a comma as reason

What is the root cause of that problem?

When we create optimistic hold report action we set the comment as 1 index of message but when we receive data from server the comment becomes 3 index in message array. Since we always take 1 index of the message array here, it is displayed right after crating and disappears when the data is updated based on the response from the server.

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

Take the index 3 from action.message instead of 1 here

translate('iou.heldRequest', {comment: action.message?.[3]?.text ?? ''})

What alternative solutions did you explore? (Optional)

Since implementing above changes causes a delay in displaying the comment because the index 3 does not exist in the optimistic data and is added after the data is received from the server, I suggest using the below opinion which will display index 3 if it exists otherwise index 1

translate('iou.heldRequest', {comment: action.message?.[3]?.text ?? action.message?.[1]?.text ?? ''})

[allgandalf]

Proposal

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

System message for hold request shows a comma as reason

What is the root cause of that problem?

When we submit the Hold reason first time, we store it at the 1nd index, but when we fetch it again when we call Open Report, this time, the BE returns that the value is stored at the 4th index, this causes the inconsistency:

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

Update the condition to check if the 3rd index exits and if not then display the text at the 1st index:

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

Lines 519 to 520 in 34fa987

	} else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) {
	children = <ReportActionItemBasicMessage message={translate('iou.heldRequest', {comment: action.message?.[1].text ?? ''})} />;

} else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) {
            console.log('action.message:', action.message);
            children = <ReportActionItemBasicMessage message={translate('iou.heldRequest', {comment: action.message?.[3]?.text ?? action.message?.[1].text})} />;
        } 

What alternative solutions did you explore? (Optional)

N/A

[joekaufmanexpensify]

Def seems like a bug.

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

The system message for hold request briefly shows the entered reason, then it changes to a comma.

What is the root cause of that problem?

The optimistic data of hold action is different from the data that is returned by server

App/src/libs/ReportUtils.ts

Lines 3690 to 3695 in 014557c

	actorAccountID: currentUserAccountID,
	message: [
	{
	type: CONST.REPORT.MESSAGE.TYPE.TEXT,
	style: 'normal',
	text: Localize.translateLocal('iou.heldRequest', {comment}),

And we're displaying the comment base on optimistic data

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

Lines 519 to 520 in 4ac5d6c

	} else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) {
	children = <ReportActionItemBasicMessage message={translate('iou.heldRequest', {comment: action.message?.[1].text ?? ''})} />;

So after the hold API is complete, the comment is replaced by ,

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

We should sync the data that in optimisticData with the data that is returned by server like this

message: [
    {
        type: CONST.REPORT.MESSAGE.TYPE.TEXT,
        style: 'normal',
        text: Localize.translateLocal('iou.heldRequest', {comment}),
    },
    {
        type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
        text: `, `,
    },
    {
        type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
        text: `saying`,
    },
    {
        type: CONST.REPORT.MESSAGE.TYPE.COMMENT,
        text: comment,
    },
],

App/src/libs/ReportUtils.ts

Lines 3690 to 3695 in 014557c

	actorAccountID: currentUserAccountID,
	message: [
	{
	type: CONST.REPORT.MESSAGE.TYPE.TEXT,
	style: 'normal',
	text: Localize.translateLocal('iou.heldRequest', {comment}),

And then here we will get the comment of hold action through the fourth element of message array action.message?.[3].text ?? ''

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

Lines 519 to 520 in 4ac5d6c

	} else if (action.actionName === CONST.REPORT.ACTIONS.TYPE.HOLD) {
	children = <ReportActionItemBasicMessage message={translate('iou.heldRequest', {comment: action.message?.[1].text ?? ''})} />;

Also update the same here.

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Line 380 in 4ac5d6c

Clipboard.setString(Localize.translateLocal('iou.heldRequest', {comment: reportAction.message?.[1]?.text ?? ''}));

OPTIONAL: To only need to update in one place when the format of hold action changes, we can create a util like getHoldReason that will return the comment of hold action and then we can use this in all related places.

What alternative solutions did you explore? (Optional)

NA

[brandonhenry]

Proposal

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

The system message for hold request briefly shows the entered reason, then it changes to a comma. This is due to a mismatch between the optimistic data and the data returned by the server.

What is the root cause of that problem?

The root cause is that the optimistic data and the server response have different formats for the message array. The optimistic data assumes the comment is at index 1, while the server returns the comment at index 3.

This one is a regression from #33897

1. Why the message originally shows correctly:

   • When the user submits the hold request, the buildOptimisticHoldReportAction function is called to create an optimistic version of the report action.
   • In this optimistic version, the comment is placed at index 1 of the message array.
   • The UI immediately displays the optimistic data, showing the correct comment.

2. Why the message changes to a comma:

   • After the optimistic data is displayed, the app sends the hold request to the server.
   • The server processes the request and sends back a response.
   • In the server response, the comment is placed at index 3 of the message array, following a specific format.
   • When the app receives the server response, it updates the report action data in the store.
   • However, the code that renders the comment in the UI is still looking for the comment at index 1 (action.message?.[1].text).
   • Since index 1 in the server response contains a comma (,), the UI displays the comma instead of the actual comment.

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

This one is a bit deep.

If we modify the optimistic data structure to have the comment at index 3, the UI would initially display an empty or incorrect comment until the server response arrives. This is because the code that renders the comment would be looking for it at index 3, which doesn't exist in the optimistic data.

Safest approach seems:

1. Keep the buildOptimisticHoldReportAction function as is, with the comment placed at index 1 in the optimistic data.

2. Update the code that renders the comment to first check for the presence of the comment at index 3 (server response format), and if it doesn't exist, fallback to index 1 (optimistic data format). Here's an example:

   const comment = action.message?.[3]?.text ?? action.message?.[1]?.text ?? '';

   This way, the UI will initially display the comment from index 1 (optimistic data) and then seamlessly switch to the comment from index 3 once the server response arrives, without any visual glitch.

What alternative solutions did you explore? (Optional)

We can consider updating the server response format to match the optimistic data structure, placing the comment at index 1. This would eliminate the need for the fallback logic in the UI and ensure consistency between the optimistic data and the server response.

Personally believe this is the correct approach. As you can see in this picture, when we send the info to the server, we have the type of the comment as CONST.REPORT.MESSAGE.TYPE.COMMENT but the server is responding with each item as have type text. If the actual comment had type comment in the response, we would be able to select the comment, no matter what structure the server responds with.

So BE change could be either:

1. Hold response from server should have extra entry with type comment and the comment
2. Hold response from server should match the same structure as FE

[joekaufmanexpensify]

@mkhutornyi thoughts on proposals?

[joekaufmanexpensify]

bump @mkhutornyi when you have a sec

[mkhutornyi]

I will provide update today

[mkhutornyi]

I agree with @nkdengineer's proposal to make optimistic data and server data always consistent.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[neil-marcellini]

I agree with @nkdengineer's proposal to make optimistic data and server data always consistent. 🎀 👀 🎀 C+ reviewed

I also agree. After making the data consistent I think that we should also implement the alternate solution when getting the hold comment, so that it's more robust against breaking changes in the future.

[melvin-bot]

📣 @mkhutornyi 🎉 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

[joekaufmanexpensify]

PR on staging

[joekaufmanexpensify]

Automation was not working, but this is now ready for payment!

[joekaufmanexpensify]

Discussing payment next steps, since original C+ went OOO unexpectedly.

[joekaufmanexpensify]

We discussed in slack, and we landed on 50/50 split for both C+, since @mkhutornyi reviewed proposals, and @thesahindia reviewed the PR.

[joekaufmanexpensify]

That means we need to pay:

• @mkhutornyi - $250 for C+ proposal review via Upwork.
• @thesahindia - $250 for C+ PR review via NewDot.
• @nkdengineer - $500 for PR via Upwork.

[joekaufmanexpensify]

OG upwork job is closed, so made a new one: https://www.upwork.com/jobs/~01c5d794e2115e005d

[joekaufmanexpensify]

@nkdengineer offer sent for $500!

[joekaufmanexpensify]

@mkhutornyi offer sent for $250!

[joekaufmanexpensify]

@thesahindia please request $250 via NewDot and confirm here once complete

[joekaufmanexpensify]

Bump @nkdengineer @mkhutornyi @thesahindia , please take the next steps above so we can pay you

[nkdengineer]

@joekaufmanexpensify I accepted the offer 🙏

[joekaufmanexpensify]

Great. TY!

[joekaufmanexpensify]

@nkdengineer $500 sent and contract ended!

[joekaufmanexpensify]

@mkhutornyi still waiting for you to accept our offer, and @thesahindia for you to request payment via NewDot. Moving to weekly for now while this is pending

[thesahindia]

Hey @joekaufmanexpensify, I have added this to my list, I will send the money request soon. You can close this after paying others!

[joekaufmanexpensify]

Sounds good. Payment summary is here. Payments outstanding still are @thesahindia's (which he needs to request).

@mkhutornyi's payment is also outstanding. I see in slack they've been out for over a month, and we're not sure when they'll be back, so going to close this for now. @mkhutornyi once you're back, please comment here, and I will re-open this issue to send your payment for this issue!

[JmillsExpensify]

$250 approved for @thesahindia

[mallenexpensify]

@mkhutornyi is back, I paid them $250 via Upwork, the contract was still open.