[HOLD for payment 2024-05-03] [$250] [Report fields] Hmm its not here page displayed when clicking on expense title

[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: 1.4.63-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. As Admin open Workspace chat with expense
2. Go to expense detail page
3. Click on Title

Expected Result:

Title edit page should open

Actual Result:

Hmm its not here error page displayed when clicking on expense title and nothing happens when click on Go back to home page button

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

Bug6453375_1713429732872.Recording__2845.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01522b79f04da5c437
• Upwork Job ID: 1780977183106654208
• Last Price Increase: 2024-04-18
• Automatic offers:
• ahmedGaber93 | Reviewer | 0
• GandalfGwaihir | Contributor | 0

[melvin-bot]

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

@trjExpensify 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 #wave-collect - Release 1

[trjExpensify]

Hm, report fields aren't supported on collect, so this seems more so related to #wave-control. CC: @thienlnam @youssef-lr @JmillsExpensify

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01522b79f04da5c437

[melvin-bot]

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

[allgandalf]

Proposal

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

Cannot access Edit title page from report money request view.

What is the root cause of that problem?

We decide whether to show the editReportFieldPage below:

App/src/pages/EditReportFieldPage.tsx

Lines 58 to 65 in a96cc01

	if (!reportField || !report || isDisabled) {
	return (
	<ScreenWrapper
	includeSafeAreaPaddingBottom={false}
	shouldEnableMaxHeight
	testID={EditReportFieldPage.displayName}
	>
	<FullPageNotFoundView

Over here we check for the value of reportField by using the condition:

App/src/pages/EditReportFieldPage.tsx

Line 53 in a96cc01

const reportField = report?.fieldList?.[fieldKey] ?? policy?.fieldList?.[fieldKey];

For other field values like date and description we don't see not here page because the fieldKey matches with the returned values of policy:

App/src/pages/EditReportFieldPage.tsx

Line 52 in a96cc01

const fieldKey = ReportUtils.getReportFieldKey(route.params.fieldID);

For Field key we use ReportUtils.getReportFieldKey, this add a extra 'expensify_text before the field key to match theBEreturned values, this is fine fordescriptionanddatebecause theBE` values match this conversion.

But for Title value, the BE returns text_title only:

But during the time of assigning fieldKey we add a extra expensify_ to it, this causes the keys to mismatch and we get a not found page

This should not happen because we do get the right data from BE, we just misinterpret how we assign the text_title key on the FE

Also, we shouldn't update the key on the BE we use the key at a lot of other places and it works smooth over there, we get a regression here only because we add extra text before the extra for text_title, functionality wise, this isn't a regression, this causes a regression because we assume the key wrong.

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

We should not call getReportFieldKey, if we are editing the title of the Report Field:

const fieldKey = route.params.fieldID === CONST.REPORT_FIELD_TITLE_FIELD_ID ? route.params.fieldID : ReportUtils.getReportFieldKey(route.params.fieldID);

Results:

Screen.Recording.2024-04-18.at.9.55.05.PM.mov

[thienlnam]

Yup that's correct - just the title is stored under text_title, all the other report field keys are stored under expensify_field_id.

[allgandalf]

I guess the proposed solution does make sense to you @thienlnam :)

[ahmedGaber93]

Reviewing now.

[ahmedGaber93]

Thanks @GandalfGwaihir for the proposal.
Your RCA and fix is correct 👍. Just small change, move the condition inside getReportFieldKey to apply this fix globally in the app.

function getReportFieldKey(reportFieldId: string) {
  if (reportFieldId === CONST.REPORT_FIELD_TITLE_FIELD_ID){
      return reportFieldId;
  }

  return `expensify_${reportFieldId}`;
}

@GandalfGwaihir's proposal LGTM.
🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[allgandalf]

Just small change, move the condition inside getReportFieldKey to apply this fix globally in the app.

Do we want to apply it globally? I’m not quite sure about it @ahmedGaber93 :) Maybe at some other place the format is different, but yeah i will check where else getReportFieldKey is being used and we will discuss that during the PR :) thanks

[ahmedGaber93]

but yeah i will check where else getReportFieldKey is being used

Yes, please do that, I just notice fieldKey it used in clearReportFieldErrors, so I assume it should fix globally in the app.

Also, I think the condition is safe as the fieldID 'text_title' is a unique fieldID, and other fields has different format field_id_FIELDNAME so if we create custom field with name title the fieldID will be "fieldID": "field_id_TITLE"

[melvin-bot]

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

📣 @GandalfGwaihir 🎉 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 📖

[thienlnam]

Yeah, agree with fixing this globally so we don't have some places trying to call expensify_text_title as opposed to text_title. It's a unique report field

[allgandalf]

PR would be up by EOD

[allgandalf]

PR ready for review @ahmedGaber93

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

• [Fix]: Fix crash on Expense Report Title due to mismatch of reportKey on the FE and BE #40704

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

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

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @GandalfGwaihir requires payment automatic offer (Contributor)

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

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR:
• [@ahmedGaber93] 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:
• [@ahmedGaber93] 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:
• [@ahmedGaber93] Determine if we should create a regression test for this bug.
• [@ahmedGaber93] 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:

[melvin-bot]

@trjExpensify, @thienlnam, @ahmedGaber93, @GandalfGwaihir Whoops! This issue is 2 days overdue. Let's get this updated quick!

[trjExpensify]

Bump on the checklist, @ahmedGaber93!

[ahmedGaber93]

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR: feat: add policyReportFields to the policy object directly #36553
• [@ahmedGaber93] 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/36553/files#r1592310890
• [@ahmedGaber93] 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
• [@ahmedGaber93] Determine if we should create a regression test for this bug.
• [@ahmedGaber93] 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:

Preconditions: your account have beta 'reportFields'

1. Open paid workspace with 'admin' role
2. Click icon (+) > Submit expense > fill fields, then submit the expense
3. Go to the expense detail page
4. Click on Title
5. Verify that edit title page is opened correctly, and no NotFoundPage displayed.

[trjExpensify]

Payment summary as follows:

• $250 to @ahmedGaber93 for the C+ review (waiting for you to accept the offer)
• $250 to @GandalfGwaihir for the fix (paid)

As for the regression test, @thienlnam @caitlinwhite1 will you guys be adding a suite of these test cases for report field flows centrally as part of the project? Seems like this one here will end up doubling up.

[thienlnam]

Good question, I don't think we've created a set of standard regression tests for this project yet.

There's probably a few piece meal QA tests added for this, but would be a good idea to get some proper ones set up in testrail. @caitlinwhite1 Would you be able to get this set up? Happy to help if needed

[caitlinwhite1]

@thienlnam Yes! On it today. Thanks Jack

[trjExpensify]

Awesome, thanks! @ahmedGaber93 still waiting for you to accept the offer to pay this one out.

[ahmedGaber93]

@trjExpensify Offer accepted.

[trjExpensify]

Thanks. Paid, closing!