feat: add delete option to deletable report fields by allroundexperts · Pull Request #36039 · Expensify/App

allroundexperts · 2024-02-07T15:50:52Z

Details

This PR adds the ability to delete a report field attached to a report, given it was removed from a policy.

Fixed Issues

$ #35331
PROPOSAL: N/A

Tests

Login to Old dot and create a paid policy.
Select the workspace and go to settings.
Enable workspace chat by executing the following commands in the console:

var p = Policy.getActive();
p.policy.isPolicyExpenseChatEnabled = true;
p.save();

Click on Reports option in the LHN and create some report fields.
Login to New dot using the same account.
Open the workspace chat and request some money.
Open the created expense report.
Make sure to have the policyReportFields beta turned on in the code.
Set the values for the report fields.
On Old dot, delete one of the report field.
In New dot, click on the same field in the expense report and verify that three dot menu shows up in the RHN.
Click the three dot menu and verify that an option to delete the field appears.
Delete the field and verify it no longer shows up in the expense report.

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Login to Old dot and create a paid policy.
Select the workspace and go to settings.
Enable workspace chat by executing the following commands in the console:

var p = Policy.getActive();
p.policy.isPolicyExpenseChatEnabled = true;
p.save();

Click on Reports option in the LHN and create some report fields.
Login to New dot using the same account.
Open the workspace chat and request some money.
Open the created expense report.
Make sure to have the policyReportFields beta turned on in the code.
Set the values for the report fields.
On Old dot, delete one of the report field.
In New dot, click on the same field in the expense report and verify that three dot menu shows up in the RHN.
Click the three dot menu and verify that an option to delete the field appears.
Delete the field and verify it no longer shows up in the expense report.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-02-19.at.6.39.31.AM.mov

Android: mWeb Chrome

Screen.Recording.2024-02-19.at.6.38.47.AM.mov

iOS: Native

Screen.Recording.2024-02-19.at.6.36.11.AM.mov

iOS: mWeb Safari

Screen.Recording.2024-02-19.at.6.30.21.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-02-19.at.6.23.01.AM.mov

MacOS: Desktop

Screen.Recording.2024-02-19.at.6.29.01.AM.mov

melvin-bot · 2024-02-25T17:35:06Z

@jjcoffee 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]

jjcoffee · 2024-02-27T10:26:02Z

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-app-2024-03-28_11.46.32.mp4

Android: mWeb Chrome

android-chrome-2024-03-28_12.49.31.mp4

iOS: Native

ios-app-2024-03-28_12.35.14.mp4

iOS: mWeb Safari

ios-safari-2024-03-28_12.43.48.mp4

MacOS: Chrome / Safari

desktop-chrome-2024-03-28_11.24.49.mp4

MacOS: Desktop

desktop-app-2024-03-28_11.34.26.mp4

jjcoffee · 2024-02-29T10:17:20Z

@allroundexperts Any thoughts on the bugs above?

jjcoffee · 2024-03-04T16:31:11Z

@allroundexperts Friendly bump! 🙇

allroundexperts · 2024-03-04T16:32:55Z

Hi @jjcoffee!
I'm on it now.

allroundexperts · 2024-03-04T17:02:12Z

BUG? I have to navigate away from the report and back (or refresh) in order for the three-dots menu to show up after deleting the field on OD.

This would be handled from the backend. I think pusher is not sending the delete event to the ND. @thienlnam Can you check here please?

allroundexperts · 2024-03-04T17:14:35Z

BUG: Unexpected error while updating the field, please try again later error shows when trying to delete a field. BE error is "Invalid ReportID '0' for the report store".

This seems to be on the backend as well. @thienlnam can you verify? I'm sending the field like shown here

thienlnam · 2024-03-04T18:43:05Z

This would be handled from the backend. I think pusher is not sending the delete event to the ND. @thienlnam Can you check here please?

This is known, there's another big refactor happening right now for pusher updates in the BE. This will be broken until that is finished

This seems to be on the backend as well. @thienlnam can you verify? I'm sending the field like shown here

I can take a look at this one - created an issue to look into it further https://github.com/Expensify/Expensify/issues/375261

jjcoffee · 2024-03-05T14:26:43Z

Hmm now the delete option never shows, I think because the reportFields now come through as fieldList when OpenReport is called.

thienlnam · 2024-03-05T18:14:58Z

Yeah that is one change, additionally for the API call - what are the parameters being sent?

You no longer need to prefix with expensify_ since it comes that way now from the changes listed here #36170 (comment)

thienlnam · 2024-03-05T18:15:24Z

We might want to move forward with the FE changes for #36170 before doing this PR

jjcoffee · 2024-04-04T13:16:29Z

@thienlnam Another one that seems most likely BE-related. When adding a new field in OD, it does come through via pusher on ND, but tapping on the field leads to Not found page.

desktop-chrome-newfield-2024-04-04_15.15.03.mp4

thienlnam · 2024-04-04T15:53:54Z

When adding a new field in OD, it does come through via pusher on ND, but tapping on the field leads to Not found page.

Hmm, if the data is stored correctly in the policy onyx key - then I would imagine this would be a FE issue

allroundexperts · 2024-04-04T21:29:52Z

@allroundexperts The tick next to the selected option is gone (again? 😄). Other than that testing well and code looks good!

Added the check icon @jjcoffee

jjcoffee · 2024-04-05T11:24:11Z

Hmm, if the data is stored correctly in the policy onyx key - then I would imagine this would be a FE issue

@thienlnam Just double-checked it's also happening on main. It looks like there's no pusher event to update the report's fieldList, which is why it's showing a not found page. I think we can handle it in a follow-up.

jjcoffee · 2024-04-05T11:25:32Z

@allroundexperts Sorry, there are conflicts now, can you fix? 😄

jjcoffee · 2024-04-05T13:13:20Z

Ah looks like the PR with the conflicts was reverted. New PR is in draft #39711. I have asked there if they can HOLD for our PR to get merged first (I think that makes the most sense here!).

jjcoffee

LGTM!

jjcoffee · 2024-04-05T13:16:16Z

Damn just noticed the perf test is failing. Can you merge main @allroundexperts?

jjcoffee · 2024-04-05T13:18:32Z

Conflicting PR won't block us, we're good to merge this once the perf test passes @thienlnam 🙇

thienlnam

This is unrelated to the perf tests, and it seems like we're going to merge them anyways https://expensify.slack.com/archives/C01GTK53T8Q/p1712343373389919

OSBotify · 2024-04-08T11:07:47Z

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.61-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-04-08T13:04:25Z

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.61-0 🚀

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

OSBotify · 2024-04-10T17:22:02Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.61-8 🚀

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

parasharrajat · 2024-06-14T15:53:04Z

src/pages/EditReportFieldPage.tsx

+                    fieldName={Str.UCFirst(reportField.name)}
+                    fieldKey={fieldKey}
+                    fieldValue={fieldValue}
+                    isRequired={!reportField.deletable}


We should have made sure that the report title field is always required. #42525

jayeshmangwani · 2024-06-27T13:25:57Z

src/pages/EditReportFieldPage.tsx


+    const handleReportFieldDelete = () => {
+        ReportActions.deleteReportField(report.reportID, reportField);
+        Navigation.dismissModal(report?.reportID);


Before we call Navigation.dismissModal here, we should set isDeleteModalVisible to false to prevent the delay in closing the delete modal #43496

c3024 · 2024-10-15T11:10:58Z

src/pages/EditReportFieldPage.tsx

-        );
+    const menuItems: ThreeDotsMenuItem[] = [];
+
+    const isReportFieldDeletable = reportField.deletable && !isReportFieldTitle;


isReportFieldTitle checks for formula also and the tile field can be text as well. So, we need to check if fieldID is title. More details here #49077

feat: add delete option to deleteable report fields

73d9a10

allroundexperts mentioned this pull request Feb 7, 2024

[Report Fields] Update 'deletable' fields to show as "remove field " vs. "empty" #35331

Closed

allroundexperts added 4 commits February 19, 2024 05:21

Merge branch 'main' into feat-35331

a486341

prettier

9001f45

more general improvements

ba800ad

lint fix

55443b9

allroundexperts marked this pull request as ready for review February 25, 2024 17:35

allroundexperts requested a review from a team as a code owner February 25, 2024 17:35

melvin-bot bot requested review from jjcoffee and removed request for a team February 25, 2024 17:35

allroundexperts added 2 commits February 25, 2024 22:52

Merge branch 'main' into feat-35331

f3129ac

fix: persist delete report fields

f2d13b9

This comment was marked as resolved.

Sign in to view

Merge branch 'main' into feat-35331

2bba4c9

allroundexperts added 4 commits March 27, 2024 00:59

merge with main

123de4f

more finetunings

7950537

Merge branch 'main' into feat-35331

cf0a78a

more refactors

9a44a1a

This comment was marked as resolved.

Sign in to view

add check for selected items

936b9dc

fix ts

237cc51

jjcoffee mentioned this pull request Apr 5, 2024

Replace OptionsSelector with SelectionList part 1 #39711

Merged

50 tasks

jjcoffee approved these changes Apr 5, 2024

View reviewed changes

melvin-bot bot requested a review from thienlnam April 5, 2024 13:14

thienlnam approved these changes Apr 5, 2024

View reviewed changes

thienlnam merged commit d3c60f6 into Expensify:main Apr 5, 2024

github-actions bot mentioned this pull request Apr 8, 2024

Deploy Checklist: New Expensify 2024-04-08 #39807

Closed

This was referenced Apr 8, 2024

[$250] Web - IOU - Report fields are not auto focused #39845

Closed

Expense - Report field is not removed when deleted #39919

Closed

This was referenced Apr 12, 2024

Deploy Checklist: New Expensify 2024-04-08 #39807 Derevyaa/Expensify-App#1

Closed

Deploy Checklist: New Expensify 2024-04-09 #39808 Derevyaa/Expensify-App#2

Closed

parasharrajat reviewed Jun 14, 2024

View reviewed changes

parasharrajat mentioned this pull request Jun 14, 2024

[HOLD for payment 2024-06-13] [$250] IOU – Previous Expense report title is returned, if save the title as empty #42525

Closed

6 tasks

jayeshmangwani reviewed Jun 27, 2024

View reviewed changes

jayeshmangwani mentioned this pull request Jun 27, 2024

[HOLD for payment 2024-06-28] [$250] Expense report - Delete modal does not disappear immediately after deleting report field #43496

Closed

6 tasks

c3024 reviewed Oct 15, 2024

View reviewed changes

c3024 mentioned this pull request Oct 15, 2024

[HOLD for payment 2024-10-14] [$250] Custom report name - Delete option is present for Title report field but unable to delete it #49077

Closed

6 tasks

Conversation

allroundexperts commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Feb 25, 2024

Uh oh!

jjcoffee commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

jjcoffee commented Feb 29, 2024

Uh oh!

jjcoffee commented Mar 4, 2024

Uh oh!

allroundexperts commented Mar 4, 2024

Uh oh!

allroundexperts commented Mar 4, 2024

Uh oh!

allroundexperts commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thienlnam commented Mar 4, 2024

Uh oh!

jjcoffee commented Mar 5, 2024

Uh oh!

thienlnam commented Mar 5, 2024

Uh oh!

thienlnam commented Mar 5, 2024

Uh oh!

jjcoffee commented Apr 4, 2024

Uh oh!

This comment was marked as resolved.

Uh oh!

thienlnam commented Apr 4, 2024

Uh oh!

allroundexperts commented Apr 4, 2024

Uh oh!

jjcoffee commented Apr 5, 2024

Uh oh!

jjcoffee commented Apr 5, 2024

Uh oh!

jjcoffee commented Apr 5, 2024

Uh oh!

jjcoffee left a comment

Choose a reason for hiding this comment

Uh oh!

jjcoffee commented Apr 5, 2024

Uh oh!

jjcoffee commented Apr 5, 2024

Uh oh!

thienlnam left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Apr 8, 2024

Uh oh!

OSBotify commented Apr 8, 2024

Uh oh!

OSBotify commented Apr 10, 2024

Uh oh!

parasharrajat Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

jayeshmangwani Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

c3024 Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

allroundexperts commented Feb 7, 2024 •

edited

Loading

jjcoffee commented Feb 27, 2024 •

edited

Loading

allroundexperts commented Mar 4, 2024 •

edited

Loading