Create report flow follow ups #59386
Conversation
| statusNum, | ||
| total: 0, | ||
| nonReimbursableTotal: 0, | ||
| participants: {}, |
There was a problem hiding this comment.
Maybe declare it earlier like this:
const participants = accountId ?? {
[accountID]: {
notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
},
} : {};
optimisticEmptyReport.ownerAccountID = accountID;
is declared twice, so we can omit it in the if (accountID), and as a result, the entire if can be removed.
SzymczakJ
force-pushed
the
@szymczak/create-report-flow-follow-up
branch
from
758fbb3 to
4010be4
Compare
|
@allgandalf 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] |
|
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
| @@ -3929,558 +3915,6 @@ function getReportPrivateNote(reportID: string | undefined) { | |||
| API.read(READ_COMMANDS.GET_REPORT_PRIVATE_NOTE, parameters, {optimisticData, successData, failureData}); | |||
| } | |||
|
|
|||
| function prepareOnboardingOnyxData( | |||
There was a problem hiding this comment.
why are we moving this out of the the actions folder ?
There was a problem hiding this comment.
How it happened:
- I had to add tests, but I wasn't able to make them pass, because of wrong imports across the app, so I had to fix these imports.
- wrong imports:
prepareOnboardingOnyxDatafunction was located inReport.ts-> it was imported inReportUtils.ts-> it was reexported fromReportUtils.tsand used inPolicy.ts, so that we don't get a linter warning "you cannot use action in another action"(for linter action is a function exported from actions folder).
This combination of import was for some reason making jest fail my tests, when I was trying to mockReportUtils.tsinReportTest.ts
So now it is fixed: prepareOnboardingOnyxData is no longer in Report.ts, but its in ReportUtils.ts. That way we don't have the import + export that was breaking jest tests.
Does this make sense? 😄 If not I'll try to explain it more plainly.
There was a problem hiding this comment.
understood, we need to document these changes for sure though!, lets look at it in a followup NAB
src/libs/actions/Report.ts
Outdated
| /** | ||
| * Removes report, it's related report actions and next step from Onyx. | ||
| */ | ||
| function removeReport(reportID: string | undefined) { | ||
| Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, null); | ||
| Onyx.set(`${ONYXKEYS.COLLECTION.NEXT_STEP}${reportID}`, null); | ||
| Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, null); | ||
| } |
There was a problem hiding this comment.
where do we call the API?
There was a problem hiding this comment.
This is deletion of optimistic report that failed to create, so it's not even present on BE.
I'll edit function comment: Removes the report after failure to create. Also removes it's related report actions and next step from Onyx. WDYT?
There was a problem hiding this comment.
or rename the function to removeFailedReport
There was a problem hiding this comment.
or rename the function to
removeFailedReport
I like this idea, the current name confused me at first glance
| // eslint-disable-next-line react-compiler/react-compiler | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [hideQABSubtitle, personalDetails, quickAction?.action, quickActionPolicy?.name, quickActionReport, translate]); |
There was a problem hiding this comment.
whyyy do we need to suppress these ones ?
There was a problem hiding this comment.
This fixes this bug #59425.
Some of the functions getting QA subtitle(getReportName) are using OnyxConnect and therefore do not get updated when user changes display name. To fix it we need to also add personalDetails as a dependency of this useMemo.
Linter is unhappy, because it doesn't see any usage of personalDetails in the hook, but actually personalDetails are used under the hood and we need to recompute useMemo, when they change.
That's why I suppress the warnings. Previously it was working, because we didn't use any useMemo, but I think it's better to have it here, even if we suppress these warnings.
There was a problem hiding this comment.
@SzymczakJ NAB but could you add a comment about the personal details being used under the hood as it might not obvious immediately and this will also help explain why we have the es linter skips
|
You might want to read the Expanation of changes section in PR description @allgandalf. Left some notes there, so it's easier to review this PR. |
Screen.Recording.2025-04-04.at.9.55.56.PM.mov
|
|
Will be fixed in http://github.com/Expensify/App/issues/59502 |
| } | ||
| type SkipViewTourOnboardingChoices = 'newDotSubmit' | 'newDotSplitChat' | 'newDotPersonalSpend' | 'newDotEmployer'; | ||
| if ( | ||
| task.type === 'viewTour' && |
There was a problem hiding this comment.
We should use a const here
There was a problem hiding this comment.
@luacmartins that code was just moved from the actions file, so we decided not to make any changes related to it
There was a problem hiding this comment.
Yea, I agree that's cleaner that way. Just noting that we could clean this up later.
| const completedTaskReportAction = task.autoCompleted | ||
| ? buildOptimisticTaskReportAction(currentTask.reportID, CONST.REPORT.ACTIONS.TYPE.TASK_COMPLETED, 'marked as complete', actorAccountID, 2) | ||
| : null; | ||
| if (task.type === 'createWorkspace') { |
| // eslint-disable-next-line react-compiler/react-compiler | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [hideQABSubtitle, personalDetails, quickAction?.action, quickActionPolicy?.name, quickActionReport, translate]); |
There was a problem hiding this comment.
@SzymczakJ NAB but could you add a comment about the personal details being used under the hood as it might not obvious immediately and this will also help explain why we have the es linter skips
| @@ -75,6 +75,35 @@ describe('libs/NextStepUtils', () => { | |||
| Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, policy).then(waitForBatchedUpdates); | |||
| }); | |||
|
|
|||
| describe('it generates and optimistic nextStep once a report has been created', () => { | |||
There was a problem hiding this comment.
| describe('it generates and optimistic nextStep once a report has been created', () => { | |
| describe('it generates an optimistic nextStep once a report has been created', () => { |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.24-2 🚀
|
|
@luacmartins @DylanDylann @mountiny How do we enable "tableReportView" to run this PR? |
|
cc @allgandalf |
It's "tableReportView" beta, from what I know every tester has all the permissions, so you don't need to worry about that. If you see a Create report button in your FAB for account that has a paid workspace, then you know you have it. |
|
@mvtglobally any applause tester on your domain has it enabled - public domains do not have the beta |
|
@SzymczakJ This PR is failing because of issue #59853 bug.mp4 |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${currentTask.reportID}`, | ||
| value: { | ||
| [taskCreatedAction.reportActionID]: {pendingAction: null}, |
There was a problem hiding this comment.
We forgot to set isOptimisticAction to null for tasks, textComment, and welcomeSignOffCommentAction in the success data. This caused this issue #60833 where, after creating tasks and going offline, the tasks appeared grayed out. Context: #60833 (comment)
| @@ -2637,14 +2604,24 @@ function buildNewReportOptimisticData(policy: OnyxEntry<Policy>, reportID: strin | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
| onyxMethod: Onyx.METHOD.MERGE, | |||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReport?.reportID}`, | |||
There was a problem hiding this comment.
this caused this regression: #61500
Explanation of Change
Expanation of changes:
FloatingActionButtonAndPopover.AttachmentPickerWithMenuItemsto enable creating reports from eligible workspace chats.OfflineWithFeedbackcomponent toMoneyRequestReportViewto handle offline state and failure state(along with fixing report optimistic data, some error utils functions and dismiss error function)prepareOnboardingOnyxDatafrom Report.ts to ReportUtils.ts to fix linter, after all I also think this is a helper function and it belongs there.MoneyRequestReportViewto handle handle specific case when we are creating a report -> selecting a workspace for it -> dismissing creation error -> we need to get navigated to the previous page.Fixed Issues
$ #59295
$ #59425
$ #59339
$ #59459
PROPOSAL:
Tests
Have all beta permissions and no workspaces on the account, open up FAB and make sure there is no create report option.
Have a workspace that is a paid group workspace and has chat enabled, open up a workspace chat and try to create report with plus buttonnext to the composer.
Offline tests
Point 10. of tests.
QA Steps
Note: to test all of these fixes you have to have tableReportView permission
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
andweb.mov
iOS: Native
io1.mov
ios2.mov
iOS: mWeb Safari
iosweb.mov
iosweb2.mov
MacOS: Chrome / Safari
webp1.mov
web2.mov
MacOS: Desktop
desktop.mov
desktop2.mov