feat: Integrate report fields with backend #34483

allroundexperts · 2024-01-14T18:55:02Z

Details

This PR Integrates the policy report fields with the backend.

Fixed Issues

$ #34198
PROPOSAL: N/A

Tests

Open any expense report.
Execute the following in console, replacing the policyID with the policyID of the policy the opened report is attached to:

Onyx.set('policyReportFields_47FB5109E0654E15', {
    text_title: {
        "defaultValue": "{report:type} #{report:reportID}",
        "fieldID": "text_title",
        "name": "Title",
        "orderWeight": 0,
        "type": "formula",

    },
    field_id_TEST: {
        "defaultValue": null,
        "fieldID": "field_id_TEST",
        "name": "Tax Code",
        "orderWeight": 1,
        "type": "text",
    },
    field_id_ANOTHER_ONE: {

        "defaultValue": "option A",
        "fieldID": "field_id_ANOTHER_ONE",
        "name": "Project",
        "orderWeight": 2,
        "type": "dropdown",
     	"values": ["option1", "option2", "option3"]
    },
    field_id_DATE_FIELD: {
        "defaultValue": "2023-11-19",
        "fieldID": "field_id_DATE_FIELD",
        "name": "Effective Date",
        "orderWeight": 3,
        "type": "date",
    }
})

Enable canUseReportFields beta in Permissions.ts file.
Verify that the custom fields show up.
Click on each of the field.
Enter a value and save it.
Make sure that the field gets saved successfully.

Verify that no errors appear in the JS console

Offline tests

Same as above

QA Steps

In Old dot, create a new Collect / Control policy.
Go to the settings of this workspace and execute the following in console:

p = Policy.getCurrent();
p.policy.isPolicyExpenseChatEnabled = "true";
p.save();

Go to the reports section of the settings of the newly create workspace and add some report fields.
Invite a user to be a member of this new workspace.
In NewDot, login as the invited user. Make sure canUseReportFields beta is enabled for the user.
Go to the workspace chat room and request some money.
Open the created expense report.
Verify that the custom fields show up.
Click on each of the field.
Enter a value and save it.
Make sure that the field gets saved successfully.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

Screen.Recording.2024-01-14.at.11.24.36.PM.mov

iOS: Native

Screen.Recording.2024-01-14.at.11.23.15.PM.mov

iOS: mWeb Safari

Screen.Recording.2024-01-14.at.10.57.37.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-01-14.at.10.51.35.PM.mov

MacOS: Desktop

Screen.Recording.2024-01-14.at.10.55.37.PM.mov

melvin-bot · 2024-01-14T18:55:07Z

@sobitneupane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

allroundexperts · 2024-01-22T12:51:57Z

Hi @sobitneupane!
Can you please prioritise this? This has been pending for some time now.

sobitneupane · 2024-01-22T13:41:43Z

@allroundexperts I won't be able to review it soon. So, can we get someone else assigned?

jjcoffee · 2024-01-22T14:20:11Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android-native-2024-01-22_16.53.24.mp4

Android: mWeb Chrome

android-chrome-2024-01-22_16.41.02.mp4

iOS: Native

ios-native-2024-01-22_17.01.51.mp4

iOS: mWeb Safari

ios-safari-2024-01-22_17.06.03.mp4

MacOS: Chrome / Safari

desktop-chrome-2024-01-22_17.08.48.mp4

MacOS: Desktop

desktop-app-2024-01-22_17.21.49.mp4

jjcoffee · 2024-01-22T15:52:25Z

@allroundexperts Could you add an Android native screenshot?

allroundexperts · 2024-01-22T15:55:32Z

@jjcoffee I am having issues with Android build. I think testing on web should be sufficient, no?

jjcoffee · 2024-01-22T16:27:46Z

src/pages/EditReportFieldPage.tsx

+    const reportFieldValue = report?.reportFields?.[policyReportField?.fieldID ?? ''];
+
+    // Decides whether to allow or disallow editing a money request
+    useEffect(() => {}, []);


Maybe tidy this away for now?

Doesn't really matter. It would be filled in an upcoming PR anyways. I think this would be a reminder to not miss this case!

jjcoffee · 2024-01-22T16:31:46Z

@allroundexperts Testing well! I guess the relevant changes for OpenReport aren't live yet as signing out and back in resets the changes?

allroundexperts · 2024-01-22T20:54:23Z

@allroundexperts Testing well! I guess the relevant changes for OpenReport aren't live yet as signing out and back in resets the changes?

That's correct!

thienlnam

Looking good, just one small comment

thienlnam · 2024-01-23T02:02:53Z

src/pages/EditReportFieldPage.tsx

+    };
+
+    if (policyReportField) {
+        if (policyReportField.type === 'text' || policyReportField.type === 'formula') {


Can we create constants for these report field types?

thienlnam · 2024-01-23T02:04:08Z

I will make an issue so that openReport returns the report fields - I thought it returned all report NVPS but let me double check

thienlnam · 2024-01-23T06:42:41Z

src/libs/actions/Report.ts

+        reportID,
+        reportFields: JSON.stringify({[policyField.fieldID]: {fieldID: policyField.fieldID, value: fieldValue, type: policyField.type, name: policyField.name}}),
+    };


Can we include all values from the policyField at this time? When OpenReport passes back the reports, it will have a reportField list that contains the reportFields at the time of creation.

So if the weights change later, this report should have the same report fields weights at the time of storage for historical context

thienlnam · 2024-01-23T07:03:33Z

Also while we're here, could we also update the Report.ts to include a new key called reportFields which is a json keyed by reportfieldIDs like below? I have a PR up that passes these down

allroundexperts · 2024-01-23T18:56:47Z

Also while we're here, could we also update the Report.ts to include a new key called reportFields which is a json keyed by reportfieldIDs like below? I have a PR up that passes these down

@thienlnam That exists already. One thing to note is that for the reports, we don't really need the full reportField object. That object exists in the policy already so I don't see any point in duplicating that into reports everywhere. We should just store the value of the field in the report.

allroundexperts · 2024-01-23T18:57:41Z

@allroundexperts There seems to be quite a delay for updating the field value on Android native only, compared with updating a request:

android-speed-2024-01-23_10.58.53.mp4
(It could be something to do with the size of the report)

This doesn't seem to have anything to do with the code of this PR. I think we can ignore this here.

jjcoffee · 2024-01-24T08:42:31Z

This doesn't seem to have anything to do with the code of this PR. I think we can ignore this here.

Agreed, I guess it's an Onyx issue. I retested on a smaller report and it's much faster.

thienlnam · 2024-01-24T08:51:16Z

One thing to note is that for the reports, we don't really need the full reportField object. That object exists in the policy already so I don't see any point in duplicating that into reports everywhere. We should just store the value of the field in the report.

We do this because we store the report fields at the time the report was created. For example, if you had some report fields, all expense reports would use those fields.

If you decided to change all of your report fields, you don't actually want it to change on your previous submitted reports. Those will retain the report fields that were already set. If we only had the object in the policy, we wouldn't have a way to see the old set report fields on an older report

allroundexperts · 2024-01-25T21:41:33Z

We do this because we store the report fields at the time the report was created. For example, if you had some report fields, all expense reports would use those fields.

If you decided to change all of your report fields, you don't actually want it to change on your previous submitted reports. Those will retain the report fields that were already set. If we only had the object in the policy, we wouldn't have a way to see the old set report fields on an older report

I thought we discussed this at some point and decided to use the fields stored in the policy as a single source of truth. However, if we need to change that, then the backend will need to ensure that for an open report (one that hasn't been settled), the fields attached to a report are in sync with the fields that are attached to the policy.

Given this, do you think I should do the changes in this PR or as a separate issue?

thienlnam

Nice - this looks solid! Thanks

thienlnam · 2024-01-30T23:58:45Z

@jjcoffee There's been a lot that has changed in this PR since your original review, could you please do another review / quick round of testing just to make sure none of the changes add any blockers?

thienlnam · 2024-01-31T00:00:05Z

Additionally, @allroundexperts could you please update the QA steps with the these instructions #34889 (comment) so that applause can test this when it goes to staging?

allroundexperts · 2024-01-31T00:01:16Z

Additionally, @allroundexperts could you please update the QA steps with the these instructions #34889 (comment) so that applause can test this when it goes to staging?

Sure thing. I'm done with my day so will take care of it tomorrow!

jjcoffee · 2024-01-31T10:15:58Z

@thienlnam Testing well so far! Just wondering if the OpenReport fix for setting the fields has hit production yet? None of the changes seem to persist, e.g. on navigating to another report or refreshing.

jjcoffee · 2024-01-31T10:47:41Z

Not sure if this is within scope of this PR, but trying to save the text_title field results in a You can't rename the report of a free plan 666 error from the BE, which isn't displayed anywhere on the FE.

jjcoffee · 2024-01-31T11:04:36Z

@allroundexperts Bug: Open any report (without doing any of the Onyx.set steps), enter some value for the VAT Number default field returned from BE. Note several console errors:

jjcoffee · 2024-01-31T11:12:34Z

Bug: Go offline, update some field values and save, go back online. The field values disappear. (I assume this is related to the BE changes not being live).

desktop-chrome-offline-2024-01-31_12.11.42.mp4

jjcoffee · 2024-01-31T11:16:22Z

Bug? Going in to edit the default title returned by the BE I'd expect this to be an editable formula.

thienlnam · 2024-01-31T18:57:06Z

@thienlnam Testing well so far! Just wondering if the OpenReport fix for setting the fields has hit production yet? None of the changes seem to persist, e.g. on navigating to another report or refreshing.

OpenReport should be returning the fields under report.reportFields, I'm not aware of anything being broken with it, but I also have some pusher updates changes for Report_SetFields

Not sure if this is within scope of this PR, but trying to save the text_title field results in a You can't rename the report of a free plan 666 error from the BE, which isn't displayed anywhere on the FE.

I think this because we're trying to add reportFields on a free plan for testing here - in theory, you won't be able to add these for free plans

Bug? Going in to edit the default title returned by the BE I'd expect this to be an editable formula.

You won't be able to change the formula, just the value so this works as expected

allroundexperts · 2024-01-31T20:32:31Z

I agree with @thienlnam about all these bugs.

allroundexperts · 2024-01-31T20:52:36Z

Added the correct QA steps @thienlnam. Please let me know if they look good enough.

thienlnam · 2024-01-31T23:06:44Z

Yup - the QA steps look solid

@allroundexperts Bug: Open any report (without doing any of the Onyx.set steps), enter some value for the VAT Number default field returned from BE. Note several console errors:

Have you been able to check this bug?

EDIT: Oh, I actually think this might be related to the free workspace thing. So going to merge without this since it doesn't appear to be a blocker

OSBotify · 2024-02-01T00:35:47Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-02-05T22:53:21Z

🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.36-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

c3024 · 2024-09-25T17:46:26Z

src/libs/ReportUtils.ts

+ * Given a report field, check if the field is for the report title.
+ */
+function isReportFieldOfTypeTitle(reportField: OnyxEntry<PolicyReportField>): boolean {
+    return reportField?.type === 'formula' && reportField?.fieldID === CONST.REPORT_FIELD_TITLE_FIELD_ID;


Should Custom report name field necessarily be a formula?

We have an issue here #49077. The bug here is if we set the custom report name as some text string instead of a formula, backend returns the type of the title field as text. Then this check fails and Title field shows up in the report though it should not show up here as per this code

App/src/components/ReportActionItem/MoneyReportView.tsx

Lines 108 to 109 in 1354957

if (ReportUtils.isReportFieldOfTypeTitle(reportField)) {

return null;

Can you please help here?

@thienlnam @allroundexperts @jjcoffee

allroundexperts added 2 commits January 14, 2024 22:40

feat: save the report fields in backend

0f25f75

fix: revert canUseReportField beta

e089455

allroundexperts requested a review from a team as a code owner January 14, 2024 18:55

melvin-bot bot requested review from sobitneupane and removed request for a team January 14, 2024 18:55

Merge branch 'main' into feat-34198

04f4296

allroundexperts mentioned this pull request Jan 14, 2024

[$500] Integrate the report field value selector component with the backend #34198

Closed

allroundexperts added 2 commits January 15, 2024 00:02

fix: lint errors

7532fff

Merge branch 'main' into feat-34198

c91cde9

jjcoffee reviewed Jan 22, 2024

View reviewed changes

thienlnam reviewed Jan 23, 2024

View reviewed changes

thienlnam mentioned this pull request Jan 23, 2024

Ensure OpenReport returns the set report fields #34940

Closed

thienlnam reviewed Jan 23, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

Merge branch 'main' into feat-34198

389569d

allroundexperts added 2 commits January 31, 2024 04:22

fix: minor refactors to function names

022efe3

fix: lint issues

db3738d

allroundexperts requested a review from thienlnam January 30, 2024 23:32

allroundexperts added 3 commits January 31, 2024 04:46

fix: use ucfirst str lib function and handle more lint issues

d0db92f

merge with main

a761eb2

fix: include the whole report field object in the set report field call

0c9939a

thienlnam approved these changes Jan 30, 2024

View reviewed changes

thienlnam requested review from jjcoffee and removed request for sobitneupane January 31, 2024 01:19

thienlnam merged commit c2083e3 into Expensify:main Feb 1, 2024
17 checks passed

github-actions bot mentioned this pull request Feb 2, 2024

Deploy Checklist: New Expensify 2024-02-02 #35673

Closed

deetergp mentioned this pull request Feb 2, 2024

Expense - Unable to save report title for workspace expense #35694

Closed

6 tasks

lanitochka17 mentioned this pull request Feb 4, 2024

[$500] [MEDIUM] IOU - Report fields are blank after refresh or leave from report #35772

Closed

6 tasks

s77rt mentioned this pull request Feb 13, 2024

[HOLD for payment 2024-02-20] [$500] IOU - From link in header show "IOU" instead of <User owes $XX> in request page #35803

Closed

6 tasks

m-natarajan mentioned this pull request Apr 29, 2024

App crashes after pasting a bunch of text from a gdoc into the compose box and navigating to the bottom #41227

Closed

6 tasks

c3024 reviewed Sep 25, 2024

View reviewed changes

	if (ReportUtils.isReportFieldOfTypeTitle(reportField)) {
	return null;

feat: Integrate report fields with backend #34483

feat: Integrate report fields with backend #34483

Conversation

allroundexperts commented Jan 14, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 14, 2024

allroundexperts commented Jan 22, 2024

sobitneupane commented Jan 22, 2024

jjcoffee commented Jan 22, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

jjcoffee commented Jan 22, 2024

allroundexperts commented Jan 22, 2024

jjcoffee Jan 22, 2024

Choose a reason for hiding this comment

allroundexperts Jan 22, 2024

Choose a reason for hiding this comment

jjcoffee commented Jan 22, 2024

allroundexperts commented Jan 22, 2024

thienlnam left a comment

Choose a reason for hiding this comment

thienlnam Jan 23, 2024

Choose a reason for hiding this comment

thienlnam commented Jan 23, 2024

thienlnam Jan 23, 2024

Choose a reason for hiding this comment

thienlnam commented Jan 23, 2024 • edited Loading

This comment was marked as resolved.

allroundexperts commented Jan 23, 2024

allroundexperts commented Jan 23, 2024

jjcoffee commented Jan 24, 2024

thienlnam commented Jan 24, 2024 • edited Loading

allroundexperts commented Jan 25, 2024

thienlnam left a comment

Choose a reason for hiding this comment

thienlnam commented Jan 30, 2024

thienlnam commented Jan 31, 2024

allroundexperts commented Jan 31, 2024

jjcoffee commented Jan 31, 2024

jjcoffee commented Jan 31, 2024

jjcoffee commented Jan 31, 2024

jjcoffee commented Jan 31, 2024

jjcoffee commented Jan 31, 2024

thienlnam commented Jan 31, 2024 • edited Loading

allroundexperts commented Jan 31, 2024

allroundexperts commented Jan 31, 2024

thienlnam commented Jan 31, 2024 • edited Loading

OSBotify commented Feb 1, 2024

OSBotify commented Feb 5, 2024

c3024 Sep 25, 2024

Choose a reason for hiding this comment

allroundexperts commented Jan 14, 2024 •

edited

Loading

jjcoffee commented Jan 22, 2024 •

edited

Loading

thienlnam commented Jan 23, 2024 •

edited

Loading

thienlnam commented Jan 24, 2024 •

edited

Loading

thienlnam commented Jan 31, 2024 •

edited

Loading

thienlnam commented Jan 31, 2024 •

edited

Loading