-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add delete option to deletable report fields #36039
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2024-03-28_11.46.32.mp4Android: mWeb Chromeandroid-chrome-2024-03-28_12.49.31.mp4iOS: Nativeios-app-2024-03-28_12.35.14.mp4iOS: mWeb Safariios-safari-2024-03-28_12.43.48.mp4MacOS: Chrome / Safaridesktop-chrome-2024-03-28_11.24.49.mp4MacOS: Desktopdesktop-app-2024-03-28_11.34.26.mp4 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@allroundexperts Any thoughts on the bugs above? |
@allroundexperts Friendly bump! 🙇 |
Hi @jjcoffee! |
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 seems to be on the backend as well. @thienlnam can you verify? I'm sending the field like shown here |
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
I can take a look at this one - created an issue to look into it further https://github.com/Expensify/Expensify/issues/375261 |
Hmm now the delete option never shows, I think because the |
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) |
We might want to move forward with the FE changes for #36170 before doing this PR |
I think it makes sense to add the change regarding the option list migration here. At the time of writing this feature, the option list was written in JS and that is now causing lint issues. Also, I feel like if we go via the route of creating new issue for this, things would get dragged even further. As such, lets try to handle these minor last pieces here even if that means an increase in the compensation. As Expensify says, Let's get the shit done once and for all 😄 |
@@ -1891,6 +1891,10 @@ export default { | |||
subtitle: 'Sync your chart of accounts and more.', | |||
}, | |||
}, | |||
reportFields: { | |||
delete: 'Delete field', | |||
deleteConfirmation: 'Are you sure that you want to delete this field?', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allroundexperts Just double checking is this copy & translation all approved?
@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 desktop-chrome-newfield-2024-04-04_15.15.03.mp4 |
Hmm, if the data is stored correctly in the policy onyx key - then I would imagine this would be a FE issue |
Added the check icon @jjcoffee |
@thienlnam Just double-checked it's also happening on main. It looks like there's no pusher event to update the report's |
@allroundexperts Sorry, there are conflicts now, can you fix? 😄 |
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!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Damn just noticed the perf test is failing. Can you merge main @allroundexperts? |
Conflicting PR won't block us, we're good to merge this once the perf test passes @thienlnam 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.61-0 🚀
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.4.61-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.61-8 🚀
|
fieldName={Str.UCFirst(reportField.name)} | ||
fieldKey={fieldKey} | ||
fieldValue={fieldValue} | ||
isRequired={!reportField.deletable} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have made sure that the report title field is always required. #42525
@@ -73,44 +84,78 @@ function EditReportFieldPage({route, policy, report}: EditReportFieldPageProps) | |||
Navigation.dismissModal(report?.reportID); | |||
}; | |||
|
|||
const handleReportFieldDelete = () => { | |||
ReportActions.deleteReportField(report.reportID, reportField); | |||
Navigation.dismissModal(report?.reportID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we call Navigation.dismissModal here, we should set isDeleteModalVisible to false to prevent the delay in closing the delete modal #43496
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
Reports
option in the LHN and create some report fields.policyReportFields
beta turned on in the code.Offline tests
N/A
QA Steps
Reports
option in the LHN and create some report fields.policyReportFields
beta turned on in the code.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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