[HOLD for Payment 2024-08-22][$250] Edit Modal Not Opening by up arrow After Holding Expense and Returning to Thread

[lanitochka17]

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: 9.0.11-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: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to Account Settings > Workspace > Create Workspace
2. From LHN, navigate to Workspace Chat
3. Click the plus sign > Submit Expense > Add Amount > Add Merchant
4. Confirm the expense, go to the thread, and go to details. Click "Hold
5. Add a reason and hold the expense. Exit the thread
6. Return to the thread and click the up arrow. Notice that the edit modal does not open

Expected Result:

The up arrow should always open the edit modal when the message is edited

Actual Result:

After holding the expense and exiting the thread, upon returning, the message appears in the composer, but the edit modal does not open when the up arrow is clicked

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

Bug6551098_1721779883250.Screen_Recording_2024-07-23_at_4.39.10_PM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016ca23291c558a731
• Upwork Job ID: 1817972655286208144
• Last Price Increase: 2024-08-05

Issue Owner

Current Issue Owner: @fedirjh

[melvin-bot]

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

[lanitochka17]

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

[lanitochka17]

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

[bernhardoj]

Proposal

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

Pressing arrow up key doesn't open the edit composer in one-transaction report if the last message is the hold reason message.

What is the root cause of that problem?

The up arrow key to edit will only work if lastReportAction (editable) exists.

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx

Lines 520 to 523 in 98d8a5a

	if (lastReportAction) {
	const message = Array.isArray(lastReportAction?.message) ? lastReportAction?.message?.at(-1) ?? null : lastReportAction?.message ?? null;
	Report.saveReportActionDraft(reportID, lastReportAction, Parser.htmlToMarkdown(message?.html ?? ''));
	}

However, in our case, it's undefined.

lastReportAction is built from the report reportActions + parentReportAction then iterated to find the first editable message. But the hold message is the transaction thread message, so it isn't included in reportActions of the one-transaction (IOU) report.

App/src/pages/home/ReportScreen.tsx

Lines 325 to 331 in 98d8a5a

	const lastReportAction: OnyxEntry<OnyxTypes.ReportAction> = useMemo(
	() =>
	reportActions.length
	? [...reportActions, parentReportAction].find((action) => ReportUtils.canEditReportAction(action) && !ReportActionsUtils.isMoneyRequestAction(action))
	: undefined,
	[reportActions, parentReportAction],
	);

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

Combine the reportActions with the transaction thread report actions like we did in ReportActionsView.

const transactionThreadReportID = ...;
const [transactionThreadReportActions = []] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadReportID}`, {
    canEvict: false,
    selector: (reportActions) => ReportActionsUtils.getSortedReportActionsForDisplay(reportActions, true), 
});
const lastReportAction: OnyxEntry<OnyxTypes.ReportAction> = useMemo(
    () =>
        reportActions.length
            ? [...ReportActionsUtils.getCombinedReportActions(reportActions, transactionThreadReportID ?? null, transactionThreadReportActions), parentReportAction].find((action) => ReportUtils.canEditReportAction(action) && !ReportActionsUtils.isMoneyRequestAction(action))
            : undefined,
    [reportActions, parentReportAction],
);

What alternative solutions did you explore? (Optional)

Create a new context and wrap it in ReportScreen. This context will hold a ref to the combined report actions. We will update the ref every time the combinedReportActions here is changed.

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

Lines 224 to 227 in 98d8a5a

	const combinedReportActions = useMemo(
	() => ReportActionsUtils.getCombinedReportActions(reportActionsToDisplay, transactionThreadReportID ?? null, transactionThreadReportActions),
	[reportActionsToDisplay, transactionThreadReportActions, transactionThreadReportID],
	);

useEffect(() => {
    // set ref
    return () => // clears the ref
}, [combinedReportActions])

Then, when the user presses the arrow up key, we will get the report actions ref and do the filter here.

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx

Lines 520 to 523 in 98d8a5a

	if (lastReportAction) {
	const message = Array.isArray(lastReportAction?.message) ? lastReportAction?.message?.at(-1) ?? null : lastReportAction?.message ?? null;
	Report.saveReportActionDraft(reportID, lastReportAction, Parser.htmlToMarkdown(message?.html ?? ''));
	}

if (key === 'UP && ...) {
    const lastAction = reportActionsContext.reportActions.find((action) => ReportUtils.canEditReportAction(action) && !ReportActionsUtils.isMoneyRequestAction(action));
    ...
}

This will prevent any unnecessary and heavy re-render.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~016ca23291c558a731

[melvin-bot]

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

[fedirjh]

I was not able to reproduce in staging v9.0.14-5

CleanShot.2024-07-31.at.18.48.47.mp4

[bernhardoj]

It's still reproducible. You need to do it in a one-transaction report.

web.mp4

[melvin-bot]

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

[puneetlath]

@fedirjh thoughts on the proposal?

[melvin-bot]

@puneetlath, @fedirjh Huh... This is 4 days overdue. Who can take care of this?

[fedirjh]

I managed to reproduce the bug with a one-transaction report.

[fedirjh]

@bernhardoj Why doesn't this bug appear in a multi-transaction report?

For the main solution of your proposal. Can you please clarify how you intend to get the value of transactionThreadReportID, considering that it is calculated later in the code?

[bernhardoj]

Why doesn't this bug appear in a multi-transaction report?

It only happens in one-transaction report because the report combines report action from the expense and transaction thread. The hold reason report action message is from the transaction thread.

Can you please clarify how you intend to get the value of transactionThreadReportID, considering that it is calculated later in the code?

We can just move up the definition.

App/src/pages/home/ReportScreen.tsx

Lines 341 to 344 in de89479

	const transactionThreadReportID = useMemo(
	() => ReportActionsUtils.getOneTransactionThreadReportID(report.reportID, reportActions ?? [], isOffline),
	[report.reportID, reportActions, isOffline],
	);

[fedirjh]

@bernhardoj Perfect, I just tested the changes, and it seems to work fine.

[fedirjh]

Let's proceed with @bernhardoj's main solution.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[bernhardoj]

PR is ready

cc: @fedirjh

[fedirjh]

Note

The production deploy automation failed: This should be on [HOLD for Payment 2024-08-22] according to #47356 prod deploy checklist, confirmed in #47133 (comment).

[iwiznia]

Just came here to say that right now I can't edit messages with up arrow at all, not sure if that means we need to hold payment of this or if something else broke it later

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

This issue has not been updated in over 15 days. @puneetlath, @fedirjh, @bernhardoj eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[puneetlath]

Wait, so what's the status of this? Does this need to be paid? Or is there a regression outstanding?

[bernhardoj]

Yes, the payment is pending. No regression.

[johncschuster]

I'd also love to get a little clarity on the status of this and how it might affect payment on the issue I was assigned.

In the issue assigned to me, @fedirjh called out a regression (comment). Is that a regression from the fix from this issue, or from something else?

[bernhardoj]

It's (#48549) a regression from another issue (#48234)

[fedirjh]

@johncschuster The issue you have been assigned to, which is #48549, is a regression from #48052 and is not related to this one.

[puneetlath]

Payment summary:

• C - $250 - @bernhardoj - to be paid via NewDot
• C+ - 250 - @fedirjh - to be paid via NewDot

Thanks everyone! Please go ahead and request payment.

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[garrettmknight]

$250 approved for @fedirjh