[HOLD for payment 2024-07-24] [$250] Search - RHP closes or returns to transaction thread depending on which field is edited

[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.85-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4641773
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• Enable all the features in the workspace so that all possible fields are present in transaction thread
• Billable option is enabled on Old Dot

1. Go to staging.new.expensify.com
2. Go to workspace chat
3. Submit an expense
4. Go to Search
5. Open the transaction from Step 3
6. Edit all the fields (Amount, description, merchant, category, tag, tax rate, tax amount, billable toggle)

Expected Result:

After editing the fields, there should be consistency whether the RHP should close and return to previous page. The RHP should not close after editing any field

Actual Result:

After editing Amount, Tag and Billable field, RHP closes.
While for the rest of the fields, RHP returns to transaction thread after editing the fields

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

Bug6517753_1718743470259.20240619_042105__1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d87627b980e14f86
• Upwork Job ID: 1803575274326292159
• Last Price Increase: 2024-06-19
• Automatic offers:
• ishpaul777 | Reviewer | 102865693
• Krishna2323 | Contributor | 102865694

Issue Owner

Current Issue Owner: @ishpaul777

[melvin-bot]

Triggered auto assignment to @MitchExpensify (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]

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

Search - RHP closes or returns to transaction thread depending on which field is edited

What is the root cause of that problem?

There is inconsistency between edit request components, some uses Navigation.dismissModal() and some Navigation.goBack(backTo);.

App/src/pages/iou/request/step/IOURequestStepTag.tsx

Lines 98 to 104 in 362953d

	if (isEditing) {
	IOU.updateMoneyRequestTag(transactionID, report?.reportID ?? '-1', updatedTag, policy, policyTags, policyCategories);
	Navigation.dismissModal();
	return;
	}
	IOU.setMoneyRequestTag(transactionID, updatedTag);
	navigateBack();

App/src/pages/iou/request/step/IOURequestStepDate.tsx

Lines 106 to 110 in 362953d

	if (isEditing) {
	IOU.updateMoneyRequestDate(transaction?.transactionID ?? '-1', reportID, newCreated, policy, policyTags, policyCategories);
	}
	navigateBack();

App/src/pages/iou/request/step/IOURequestStepMerchant.tsx

Lines 86 to 107 in 362953d

	const updateMerchant = (value: FormOnyxValues<typeof ONYXKEYS.FORMS.MONEY_REQUEST_MERCHANT_FORM>) => {
	const newMerchant = value.moneyRequestMerchant?.trim();
	// In the split flow, when editing we use SPLIT_TRANSACTION_DRAFT to save draft value
	if (isEditingSplitBill) {
	IOU.setDraftSplitTransaction(transactionID, {merchant: newMerchant});
	navigateBack();
	return;
	}
	// In case the merchant hasn't been changed, do not make the API request.
	// In case the merchant has been set to empty string while current merchant is partial, do nothing too.
	if (newMerchant === merchant || (newMerchant === '' && merchant === CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT)) {
	navigateBack();
	return;
	}
	// When creating/editing an expense, newMerchant can be blank so we fall back on PARTIAL_TRANSACTION_MERCHANT
	IOU.setMoneyRequestMerchant(transactionID, newMerchant || CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT, !isEditing);
	if (isEditing) {
	IOU.updateMoneyRequestMerchant(transactionID, reportID, newMerchant || CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT, policy, policyTags, policyCategories);
	}
	navigateBack();

App/src/pages/iou/request/step/IOURequestStepCurrency.tsx

Lines 44 to 56 in 362953d

	if (pageIndex === 'confirm') {
	const routeToAmountPageWithConfirmationAsBackTo = getUrlWithBackToParam(
	backTo as string,
	`/${ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(CONST.IOU.ACTION.CREATE, iouType, transactionID, reportID)}`,
	);
	if (selectedCurrencyValue) {
	Navigation.navigate(`${routeToAmountPageWithConfirmationAsBackTo}&currency=${selectedCurrencyValue}` as Route);
	} else {
	Navigation.goBack(routeToAmountPageWithConfirmationAsBackTo as Route);
	}
	return;
	}
	Navigation.goBack(backTo);

App/src/pages/iou/request/step/IOURequestStepTaxRatePage.tsx

Lines 86 to 98 in 362953d

	if (isEditing) {
	const newTaxCode = taxes.code;
	IOU.updateMoneyRequestTaxRate({
	transactionID: currentTransaction?.transactionID ?? '-1',
	optimisticReportActionID: report?.reportID ?? '-1',
	taxCode: newTaxCode,
	taxAmount: CurrencyUtils.convertToBackendAmount(taxAmount ?? 0),
	policy,
	policyTagList: policyTags,
	policyCategories,
	});
	navigateBack();
	return;

App/src/pages/iou/request/step/IOURequestStepAmount.tsx

Lines 305 to 306 in 362953d

	IOU.updateMoneyRequestAmountAndCurrency({transactionID, transactionThreadReportID: reportID, currency, amount: newAmount, taxAmount, policy, taxCode});
	Navigation.dismissModal();

App/src/pages/iou/request/step/IOURequestStepDescription.tsx

Lines 128 to 141 in 362953d

	if (isEditingSplitBill) {
	IOU.setDraftSplitTransaction(transaction?.transactionID ?? '-1', {comment: newComment});
	navigateBack();
	return;
	}
	const isTransactionDraft = action === CONST.IOU.ACTION.CREATE || IOUUtils.isMovingTransactionFromTrackExpense(action);
	IOU.setMoneyRequestDescription(transaction?.transactionID ?? '-1', newComment, isTransactionDraft);
	if (action === CONST.IOU.ACTION.EDIT) {
	IOU.updateMoneyRequestDescription(transaction?.transactionID ?? '-1', reportID, newComment, policy, policyTags, policyCategories);
	}
	navigateBack();

App/src/components/ReportActionItem/MoneyRequestView.tsx

Lines 206 to 216 in 362953d

	const saveBillable = useCallback(
	(newBillable: boolean) => {
	// If the value hasn't changed, don't request to save changes on the server and just close the modal
	if (newBillable === TransactionUtils.getBillable(transaction)) {
	Navigation.dismissModal();
	return;
	}
	IOU.updateMoneyRequestBillable(transaction?.transactionID ?? '-1', report?.reportID, newBillable, policy, policyTagList, policyCategories);
	Navigation.dismissModal();
	},
	[transaction, report, policy, policyTagList, policyCategories],

Note: There are more components that are being used for editing IOU request, we should check all components which are used in the IOU request edit flow.

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

We should update all components to navigate back instead of dismissing the modals. If we don't want to navigate back in all cases, we can just navigate back when editing, all components has isEditing variable to check for that.

For MoneyRequestView billable edit action we can use useIsReportOpenInRHP hook and won't call Navigation.dismissModal(); in saveBillable function.

What alternative solutions did you explore? (Optional)

Result

iou_step_inconsistency.mp4

[Krishna2323]

Proposal Updated

• Added few references and updated main solution.

[MitchExpensify]

I agree this is weird; the RHP should not close after editing any field IMO. If we close the RHP we're assuming the user is finished editing that expense which may not be the case. We should drop them back in the view they were previously on which is the RHP open.

[melvin-bot]

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

[melvin-bot]

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

[ishpaul777]

We can go with @Krishna2323 Proposal!

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[NikkiWines]

Agreed, @Krishna2323's proposal looks fine for this issue

[melvin-bot]

📣 @ishpaul777 🎉 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]

Will raise PR for this tomorrow.

[ishpaul777]

Update ^

[MitchExpensify]

Reassigning while I'm on leave (PR is on staging) 🙇

[melvin-bot]

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

[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 9.0.7-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Search - RHP closes or returns to transaction thread depending on which field is edited. #44662

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

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

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

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

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

[sonialiap]

Payment summary:

• @Krishna2323 $250 - offer sent via upwork
• @ishpaul777 $250 - offer sent via upwork - please complete the checklist - paid ✔️

https://www.upwork.com/ab/applicants/1816055594606816091/job-details

[Krishna2323]

Offer accepted. Thanks

Sorry, two offers was sent, I accepted both. Canceled one.

[sonialiap]

Sorry Krishna! I thought I copied Ishpaul's name so that I can search for him on Upwork but apparently I didn't and sent a duplicate offer to you 😅

[ishpaul777]

[@ishpaul777] The PR that introduced the bug has been identified. Link to the PR: This was inconsistency across different edit pages i was not able to track to a specific PR using git blame history.
[@ishpaul777] 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: N/A
[@ishpaul777] 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 required
[@ishpaul777] Determine if we should create a regression test for this bug.- Yes
[@ishpaul777] 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:

Precondition:

• Enable all the features in the workspace so that all possible fields are present in transaction thread
• Billable option is enabled on Old Dot

1. Go to new.expensify.com
2. Go to workspace chat
3. Submit an expense
4. Go to Search
5. Open the transaction from Step 3
6. Edit all the fields (Amount, description, merchant, category, tag, tax rate, tax amount, billable toggle)

Expected Result:

After editing the fields, there should be consistency and user should return to previous page.

Do we agree 👍 or 👎

[ishpaul777]

Hello @sonialiap Please hold my payment for few days (Expected end of this month), i'll bump once i am ready to accept payment, Feel free to move this to weekly and make me issue owner ★

[sonialiap]

@ishpaul777 checking if I can issue payment for you here? :)

[ishpaul777]

Thank you for holding payment! Please issue whenever you get the chance

[sonialiap]

Excellent, payment issued ✔️