-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add QAB for Create report flow #59167
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
Changes from all commits
422ec4d
3ae66bd
92a5a89
d78e97c
80dc09e
a3f8e99
7876485
575f1d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,13 @@ | ||
| import Navigation from '@libs/Navigation/Navigation'; | ||
| import {generateReportID} from '@libs/ReportUtils'; | ||
| import CONST from '@src/CONST'; | ||
| import ROUTES from '@src/ROUTES'; | ||
| import type {PersonalDetails} from '@src/types/onyx'; | ||
| import type {QuickActionName} from '@src/types/onyx/QuickAction'; | ||
| import type QuickAction from '@src/types/onyx/QuickAction'; | ||
| import type {IOURequestType} from './IOU'; | ||
| import {startMoneyRequest} from './IOU'; | ||
| import {createNewReport} from './Report'; | ||
| import {startOutCreateTaskQuickAction} from './Task'; | ||
|
|
||
| function getQuickActionRequestType(action: QuickActionName | undefined): IOURequestType | undefined { | ||
|
|
@@ -25,8 +29,14 @@ function getQuickActionRequestType(action: QuickActionName | undefined): IOURequ | |
| return requestType; | ||
| } | ||
|
|
||
| function navigateToQuickAction(isValidReport: boolean, quickActionReportID: string, quickAction: QuickAction, selectOption: (onSelected: () => void, shouldRestrictAction: boolean) => void) { | ||
| const reportID = isValidReport ? quickActionReportID : generateReportID(); | ||
| function navigateToQuickAction( | ||
| isValidReport: boolean, | ||
| quickAction: QuickAction, | ||
| currentUserPersonalDetails: PersonalDetails, | ||
| policyID: string | undefined, | ||
| selectOption: (onSelected: () => void, shouldRestrictAction: boolean) => void, | ||
| ) { | ||
| const reportID = isValidReport && quickAction?.chatReportID ? quickAction?.chatReportID : generateReportID(); | ||
| const requestType = getQuickActionRequestType(quickAction?.action); | ||
|
|
||
| switch (quickAction?.action) { | ||
|
|
@@ -35,15 +45,15 @@ function navigateToQuickAction(isValidReport: boolean, quickActionReportID: stri | |
| case CONST.QUICK_ACTIONS.REQUEST_DISTANCE: | ||
| case CONST.QUICK_ACTIONS.PER_DIEM: | ||
| selectOption(() => startMoneyRequest(CONST.IOU.TYPE.SUBMIT, reportID, requestType, true), true); | ||
| return; | ||
| break; | ||
| case CONST.QUICK_ACTIONS.SPLIT_MANUAL: | ||
| case CONST.QUICK_ACTIONS.SPLIT_SCAN: | ||
| case CONST.QUICK_ACTIONS.SPLIT_DISTANCE: | ||
| selectOption(() => startMoneyRequest(CONST.IOU.TYPE.SPLIT, reportID, requestType, true), true); | ||
| return; | ||
| break; | ||
|
mountiny marked this conversation as resolved.
|
||
| case CONST.QUICK_ACTIONS.SEND_MONEY: | ||
| selectOption(() => startMoneyRequest(CONST.IOU.TYPE.PAY, reportID, undefined, true), false); | ||
| return; | ||
| break; | ||
| case CONST.QUICK_ACTIONS.ASSIGN_TASK: | ||
| selectOption(() => startOutCreateTaskQuickAction(isValidReport ? reportID : '', quickAction.targetAccountID ?? CONST.DEFAULT_NUMBER_ID), false); | ||
| break; | ||
|
|
@@ -52,6 +62,14 @@ function navigateToQuickAction(isValidReport: boolean, quickActionReportID: stri | |
| case CONST.QUICK_ACTIONS.TRACK_DISTANCE: | ||
| selectOption(() => startMoneyRequest(CONST.IOU.TYPE.TRACK, reportID, requestType, true), false); | ||
| break; | ||
| case CONST.QUICK_ACTIONS.CREATE_REPORT: | ||
| selectOption(() => { | ||
| const optimisticReportID = createNewReport(currentUserPersonalDetails, policyID); | ||
| Navigation.setNavigationActionToMicrotaskQueue(() => { | ||
| Navigation.navigate(ROUTES.SEARCH_MONEY_REQUEST_REPORT.getRoute({reportID: optimisticReportID, backTo: Navigation.getActiveRoute()})); | ||
| }); | ||
| }, true); | ||
| break; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be changed to 'return' here to maintain consistency.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a linter rule prohibiting it for some reason :(. But I will change everything to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. works!, but what's the issue that it throws?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| default: | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2546,6 +2546,15 @@ function buildNewReportOptimisticData(policy: OnyxEntry<Policy>, reportID: strin | |||||
| key: `${ONYXKEYS.COLLECTION.NEXT_STEP}${reportID}`, | ||||||
| value: optimisticNextStep, | ||||||
| }, | ||||||
| { | ||||||
| onyxMethod: Onyx.METHOD.SET, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we use SET in all of these, I think we should just use Merge
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @allgandalf can you think of a reason to use Set for these instead of Merge?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found some comments from the previous implementation: App/src/libs/actions/Report.ts Lines 1012 to 1013 in 92a5a89
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some quick docs from the upstream repo:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It says:
I say
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every onyx data update(there's about 18 of them) of
new Quick Action is something different, that the old one and no stale data should be left, so using SET seems reasonable to me.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but at the same time we only use the same keys so nothing stale would be left behind. NAB |
||||||
| key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE, | ||||||
| value: { | ||||||
| action: CONST.QUICK_ACTIONS.CREATE_REPORT, | ||||||
| chatReportID: parentReport?.reportID, | ||||||
| isFirstQuickAction: isEmptyObject(quickAction), | ||||||
| }, | ||||||
| }, | ||||||
| ]; | ||||||
|
|
||||||
| const failureData: OnyxUpdate[] = [ | ||||||
|
|
@@ -2559,6 +2568,11 @@ function buildNewReportOptimisticData(policy: OnyxEntry<Policy>, reportID: strin | |||||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, | ||||||
| value: {[reportActionID]: {errorFields: {create: getMicroSecondOnyxErrorWithTranslationKey('report.genericCreateReportFailureMessage')}}}, | ||||||
| }, | ||||||
| { | ||||||
| onyxMethod: Onyx.METHOD.SET, | ||||||
| key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE, | ||||||
| value: quickAction ?? null, | ||||||
| }, | ||||||
| ]; | ||||||
|
|
||||||
| const successData: OnyxUpdate[] = [ | ||||||
|
|
@@ -2606,7 +2620,7 @@ function createNewReport(creatorPersonalDetails: PersonalDetails, policyID?: str | |||||
|
|
||||||
| API.write( | ||||||
| WRITE_COMMANDS.CREATE_APP_REPORT, | ||||||
| {reportName: optimisticReportName, type: CONST.REPORT.TYPE.EXPENSE, policyID, reportID: optimisticReportID, reportActionID, reportPreviewReportActionID}, | ||||||
| {reportName: optimisticReportName, type: CONST.REPORT.TYPE.EXPENSE, policyID, reportID: optimisticReportID, reportActionID, reportPreviewReportActionID, shouldUpdateQAB: true}, | ||||||
| {optimisticData, successData, failureData}, | ||||||
| ); | ||||||
| return optimisticReportID; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1485,4 +1485,37 @@ describe('actions/Report', () => { | |
|
|
||
| expect(report?.lastMentionedTime).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should create new report and "create report" quick action, when createNewReport gets called', async () => { | ||
| const reportID = Report.createNewReport({accountID: 1234}, '5678'); | ||
| await new Promise<void>((resolve) => { | ||
| const connection = Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT, | ||
| waitForCollectionCallback: true, | ||
| callback: (reports) => { | ||
| Onyx.disconnect(connection); | ||
| const createdReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]; | ||
|
|
||
| // assert correctness of crucial onyx data | ||
| expect(createdReport?.reportID).toBe(reportID); | ||
| expect(createdReport?.total).toBe(0); | ||
|
|
||
| resolve(); | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| const connection = Onyx.connect({ | ||
| key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE, | ||
| callback: (quickAction) => { | ||
| Onyx.disconnect(connection); | ||
|
|
||
| // Then the quickAction.action should be set to CREATE_REPORT | ||
| expect(quickAction?.action).toBe(CONST.QUICK_ACTIONS.CREATE_REPORT); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For future pr could you also assert the report id |
||
| resolve(); | ||
| }, | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add a test for the expected onyx data when the createReport action is executed? same when it fails
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Automated tests are the new normal 😌 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,19 @@ | ||
| import {startMoneyRequest} from '@libs/actions/IOU'; | ||
| import {navigateToQuickAction} from '@libs/actions/QuickActionNavigation'; | ||
| import {createNewReport} from '@libs/actions/Report'; | ||
| import {startOutCreateTaskQuickAction} from '@libs/actions/Task'; | ||
| import {generateReportID} from '@libs/ReportUtils'; | ||
| import CONST from '@src/CONST'; | ||
|
|
||
| jest.mock('@libs/actions/IOU', () => ({ | ||
| startMoneyRequest: jest.fn(), | ||
| })); | ||
| jest.mock('@libs/actions/Report', () => ({ | ||
| createNewReport: jest.fn(), | ||
| })); | ||
| jest.mock('@libs/actions/Task', () => ({ | ||
| startOutCreateTaskQuickAction: jest.fn(), | ||
| })); | ||
|
|
||
| describe('IOU Utils', () => { | ||
| // Given navigateToQuickAction is called with quick action argument when clicking on quick action button from Global create menu | ||
|
|
@@ -14,7 +22,7 @@ describe('IOU Utils', () => { | |
|
|
||
| it('should be navigated to Manual Submit Expense', () => { | ||
| // When the quick action is REQUEST_MANUAL | ||
| navigateToQuickAction(true, reportID, {action: CONST.QUICK_ACTIONS.REQUEST_MANUAL}, (onSelected: () => void) => { | ||
| navigateToQuickAction(true, {action: CONST.QUICK_ACTIONS.REQUEST_MANUAL, chatReportID: reportID}, {accountID: 1234}, undefined, (onSelected: () => void) => { | ||
| onSelected(); | ||
| }); | ||
| // Then we should start manual submit request flow | ||
|
|
@@ -23,7 +31,7 @@ describe('IOU Utils', () => { | |
|
|
||
| it('should be navigated to Scan receipt Split Expense', () => { | ||
| // When the quick action is SPLIT_SCAN | ||
| navigateToQuickAction(true, reportID, {action: CONST.QUICK_ACTIONS.SPLIT_SCAN}, (onSelected: () => void) => { | ||
| navigateToQuickAction(true, {action: CONST.QUICK_ACTIONS.SPLIT_SCAN, chatReportID: reportID}, {accountID: 1234}, undefined, (onSelected: () => void) => { | ||
| onSelected(); | ||
| }); | ||
| // Then we should start scan split request flow | ||
|
|
@@ -32,7 +40,7 @@ describe('IOU Utils', () => { | |
|
|
||
| it('should be navigated to Track distance Expense', () => { | ||
| // When the quick action is TRACK_DISTANCE | ||
| navigateToQuickAction(true, reportID, {action: CONST.QUICK_ACTIONS.TRACK_DISTANCE}, (onSelected: () => void) => { | ||
| navigateToQuickAction(true, {action: CONST.QUICK_ACTIONS.TRACK_DISTANCE, chatReportID: reportID}, {accountID: 1234}, undefined, (onSelected: () => void) => { | ||
| onSelected(); | ||
| }); | ||
| // Then we should start distance track request flow | ||
|
|
@@ -41,11 +49,30 @@ describe('IOU Utils', () => { | |
|
|
||
| it('should be navigated to Per Diem Expense', () => { | ||
| // When the quick action is PER_DIEM | ||
| navigateToQuickAction(true, reportID, {action: CONST.QUICK_ACTIONS.PER_DIEM}, (onSelected: () => void) => { | ||
| navigateToQuickAction(true, {action: CONST.QUICK_ACTIONS.PER_DIEM, chatReportID: reportID}, {accountID: 1234}, undefined, (onSelected: () => void) => { | ||
| onSelected(); | ||
| }); | ||
| // Then we should start per diem request flow | ||
| expect(startMoneyRequest).toHaveBeenCalledWith(CONST.IOU.TYPE.SUBMIT, reportID, CONST.IOU.REQUEST_TYPE.PER_DIEM, true); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Non IOU quickActions test:', () => { | ||
| const reportID = generateReportID(); | ||
|
|
||
| describe('navigateToQuickAction', () => { | ||
| it('creates new report for "createReport" quick action', () => { | ||
| navigateToQuickAction(true, {action: CONST.QUICK_ACTIONS.CREATE_REPORT, chatReportID: reportID}, {accountID: 1234}, undefined, (onSelected: () => void) => { | ||
| onSelected(); | ||
| }); | ||
| expect(createNewReport).toHaveBeenCalled(); | ||
| }); | ||
| it('starts create task flow for "assignTask" quick action', () => { | ||
| navigateToQuickAction(true, {action: CONST.QUICK_ACTIONS.ASSIGN_TASK, targetAccountID: 123}, {accountID: 1234}, undefined, (onSelected: () => void) => { | ||
| onSelected(); | ||
| }); | ||
| expect(startOutCreateTaskQuickAction).toHaveBeenCalled(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should add a test where we ensure the QAB data was correctly set in onyx
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree ^, but not blocking my review for this |
||
| }); | ||
| }); | ||
| }); | ||

Uh oh!
There was an error while loading. Please reload this page.