[No QA] Remove default values for IDs to comply with eslint rules #54306
+70
−58
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -296,7 +296,7 @@ | |
| REPORT: 'r', | ||
| REPORT_WITH_ID: { | ||
| route: 'r/:reportID?/:reportActionID?', | ||
| getRoute: (reportID = '', reportActionID?: string, referrer?: string) => { | ||
| const baseRoute = reportActionID ? (`r/${reportID}/${reportActionID}` as const) : (`r/${reportID}` as const); | ||
| const referrerParam = referrer ? `?referrer=${encodeURIComponent(referrer)}` : ''; | ||
| return `${baseRoute}${referrerParam}` as const; | ||
|
|
@@ -698,7 +698,7 @@ | |
| WORKSPACE_NEW_ROOM: 'workspace/new-room', | ||
| WORKSPACE_INITIAL: { | ||
| route: 'settings/workspaces/:policyID', | ||
| getRoute: (policyID = '', backTo?: string) => `${getUrlWithBackToParam(`settings/workspaces/${policyID}`, backTo)}` as const, | ||
| }, | ||
| WORKSPACE_INVITE: { | ||
| route: 'settings/workspaces/:policyID/invite', | ||
|
|
@@ -923,7 +923,7 @@ | |
| }, | ||
| WORKSPACE_MEMBERS: { | ||
| route: 'settings/workspaces/:policyID/members', | ||
| getRoute: (policyID = '') => `settings/workspaces/${policyID}/members` as const, | ||
| }, | ||
| WORKSPACE_MEMBERS_IMPORT: { | ||
| route: 'settings/workspaces/:policyID/members/import', | ||
|
|
@@ -935,7 +935,7 @@ | |
| }, | ||
| POLICY_ACCOUNTING: { | ||
| route: 'settings/workspaces/:policyID/accounting', | ||
| getRoute: (policyID = '', newConnectionName?: ConnectionName, integrationToDisconnect?: ConnectionName, shouldDisconnectIntegrationBeforeConnecting?: boolean) => { | ||
| let queryParams = ''; | ||
| if (newConnectionName) { | ||
| queryParams += `?newConnectionName=${newConnectionName}`; | ||
|
|
@@ -972,7 +972,7 @@ | |
| }, | ||
| WORKSPACE_CATEGORIES: { | ||
| route: 'settings/workspaces/:policyID/categories', | ||
| getRoute: (policyID = '') => `settings/workspaces/${policyID}/categories` as const, | ||
| }, | ||
| WORKSPACE_CATEGORY_SETTINGS: { | ||
| route: 'settings/workspaces/:policyID/category/:categoryName', | ||
|
|
@@ -1037,7 +1037,7 @@ | |
| }, | ||
| WORKSPACE_MORE_FEATURES: { | ||
| route: 'settings/workspaces/:policyID/more-features', | ||
| getRoute: (policyID = '') => `settings/workspaces/${policyID}/more-features` as const, | ||
| }, | ||
| WORKSPACE_TAGS: { | ||
| route: 'settings/workspaces/:policyID/tags', | ||
|
|
@@ -1162,16 +1162,16 @@ | |
| }, | ||
| WORKSPACE_REPORT_FIELDS_LIST_VALUES: { | ||
| route: 'settings/workspaces/:policyID/reportFields/listValues/:reportFieldID?', | ||
| getRoute: (policyID: string, reportFieldID?: string) => `settings/workspaces/${policyID}/reportFields/listValues/${encodeURIComponent(reportFieldID ?? '')}` as const, | ||
|
Check failure on line 1165 in src/ROUTES.ts
|
||
| }, | ||
| WORKSPACE_REPORT_FIELDS_ADD_VALUE: { | ||
| route: 'settings/workspaces/:policyID/reportFields/addValue/:reportFieldID?', | ||
| getRoute: (policyID: string, reportFieldID?: string) => `settings/workspaces/${policyID}/reportFields/addValue/${encodeURIComponent(reportFieldID ?? '')}` as const, | ||
|
Check failure on line 1169 in src/ROUTES.ts
|
||
| }, | ||
| WORKSPACE_REPORT_FIELDS_VALUE_SETTINGS: { | ||
| route: 'settings/workspaces/:policyID/reportFields/:valueIndex/:reportFieldID?', | ||
| getRoute: (policyID: string, valueIndex: number, reportFieldID?: string) => | ||
| `settings/workspaces/${policyID}/reportFields/${valueIndex}/${encodeURIComponent(reportFieldID ?? '')}` as const, | ||
|
Check failure on line 1174 in src/ROUTES.ts
|
||
| }, | ||
| WORKSPACE_REPORT_FIELDS_EDIT_VALUE: { | ||
| route: 'settings/workspaces/:policyID/reportFields/new/:valueIndex/edit', | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -804,11 +804,11 @@ function clearAvatarErrors(reportID: string) { | |
| * @param participantAccountIDList The list of accountIDs that are included in a new chat, not including the user creating it | ||
| */ | ||
| function openReport( | ||
| reportID = '', | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| reportActionID?: string, | ||
| participantLoginList: string[] = [], | ||
| newReportObject?: ReportUtils.OptimisticChatReport, | ||
| parentReportActionID = '', | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| isFromDeepLink = false, | ||
| participantAccountIDList: number[] = [], | ||
| avatar?: File | CustomRNImageManipulatorResult, | ||
|
|
@@ -1065,7 +1065,7 @@ function openReport( | |
| failureData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${newReportObject.parentReportID}`, | ||
| value: {[parentReportActionID]: {childReportID: '', childType: ''}}, | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
| } | ||
| } | ||
|
|
@@ -1142,12 +1142,12 @@ function navigateToAndOpenReport( | |
| const report = isEmptyObject(chat) ? newChat : chat; | ||
|
|
||
| // We want to pass newChat here because if anything is passed in that param (even an existing chat), we will try to create a chat on the server | ||
| openReport(report?.reportID, '', userLogins, newChat, undefined, undefined, undefined, avatarFile); | ||
| if (shouldDismissModal) { | ||
| Navigation.dismissModalWithReport(report); | ||
| } else { | ||
| Navigation.navigateWithSwitchPolicyID({route: ROUTES.HOME}); | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report?.reportID), actionType); | ||
|
There was a problem hiding this comment. Make sure it's not called if there is no There was a problem hiding this comment. But if I prevent navigation from happening it will disrupt normal user flow 🤔 Shouldn't we warn the user that there was some problem, or something? There was a problem hiding this comment. Let's try to test it as I mentioned here. What happens if you navigate user to the screen with There was a problem hiding this comment. @VickyStash if the id is invalid, the page will show a Not found page but it seems that all reports in Onyx are deleted |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -1165,7 +1165,7 @@ function navigateToAndOpenReportWithAccountIDs(participantAccountIDs: number[]) | |
| const report = chat ?? newChat; | ||
|
|
||
| // We want to pass newChat here because if anything is passed in that param (even an existing chat), we will try to create a chat on the server | ||
| openReport(report?.reportID, '', [], newChat, '0', false, participantAccountIDs); | ||
| Navigation.dismissModalWithReport(report); | ||
| } | ||
|
|
||
|
|
@@ -1176,8 +1176,8 @@ function navigateToAndOpenReportWithAccountIDs(participantAccountIDs: number[]) | |
| * @param parentReportAction the parent comment of a thread | ||
| * @param parentReportID The reportID of the parent | ||
| */ | ||
| function navigateToAndOpenChildReport(childReportID = '', parentReportAction: Partial<ReportAction> = {}, parentReportID = '0') { | ||
| if (childReportID !== '' && childReportID !== '0') { | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(childReportID)); | ||
| } else { | ||
| const participantAccountIDs = [...new Set([currentUserAccountID, Number(parentReportAction.actorAccountID)])]; | ||
|
|
@@ -1357,7 +1357,7 @@ function readNewestAction(reportID: string, shouldResetUnreadMarker = false) { | |
| * Sets the last read time on a report | ||
| */ | ||
| function markCommentAsUnread(reportID: string, reportActionCreated: string) { | ||
| if (reportID === '') { | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Log.warn('7339cd6c-3263-4f89-98e5-730f0be15784 Invalid report passed to MarkCommentAsUnread. Not calling the API because it wil fail.'); | ||
| return; | ||
| } | ||
|
|
@@ -1482,7 +1482,7 @@ function handleReportChanged(report: OnyxEntry<Report>) { | |
| const currCallback = callback; | ||
| callback = () => { | ||
| currCallback(); | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.preexistingReportID), CONST.NAVIGATION.TYPE.UP); | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| // The report screen will listen to this event and transfer the draft comment to the existing report | ||
|
|
@@ -1503,7 +1503,7 @@ function handleReportChanged(report: OnyxEntry<Report>) { | |
| return; | ||
| } | ||
|
|
||
| saveReportDraftComment(report.preexistingReportID, draftReportComment, callback); | ||
|
|
||
| return; | ||
| } | ||
|
|
@@ -1928,10 +1928,13 @@ function updateRoomVisibility(reportID: string, previousValue: RoomVisibility | | |
| * @param parentReportID The reportID of the parent | ||
| * @param prevNotificationPreference The previous notification preference for the child report | ||
| */ | ||
| function toggleSubscribeToChildReport(childReportID = '', parentReportAction: Partial<ReportAction> = {}, parentReportID = '', prevNotificationPreference?: NotificationPreference) { | ||
| if (childReportID !== '') { | ||
| openReport(childReportID); | ||
| // updateNotificationPreference cannot default parentReportActionID to '', | ||
| // otherwise it's behaviour will change | ||
| // eslint-disable-next-line rulesdir/no-default-id-values | ||
| const parentReportActionID = parentReportAction?.reportActionID ?? ''; | ||
pac-guerreiro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (!prevNotificationPreference || prevNotificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { | ||
| updateNotificationPreference(childReportID, prevNotificationPreference, CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS, parentReportID, parentReportActionID); | ||
| } else { | ||
|
|
@@ -2889,7 +2892,7 @@ function navigateToMostRecentReport(currentReport: OnyxEntry<Report>) { | |
| const lastAccessedReportID = ReportUtils.findLastAccessedReport(false, false, undefined, currentReport?.reportID)?.reportID; | ||
|
|
||
| if (lastAccessedReportID) { | ||
| const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID); | ||
| Navigation.goBack(lastAccessedReportRoute); | ||
| } else { | ||
| const isChatThread = ReportUtils.isChatThread(currentReport); | ||
|
|
@@ -3587,14 +3590,14 @@ function prepareOnboardingOptimisticData( | |
| const taskDescription = | ||
| typeof task.description === 'function' | ||
| ? task.description({ | ||
| adminsRoomLink: `${environmentURL}/${ROUTES.REPORT_WITH_ID.getRoute(adminsChatReportID)}`, | ||
| workspaceCategoriesLink: `${environmentURL}/${ROUTES.WORKSPACE_CATEGORIES.getRoute(onboardingPolicyID)}`, | ||
| workspaceMembersLink: `${environmentURL}/${ROUTES.WORKSPACE_MEMBERS.getRoute(onboardingPolicyID)}`, | ||
| workspaceMoreFeaturesLink: `${environmentURL}/${ROUTES.WORKSPACE_MORE_FEATURES.getRoute(onboardingPolicyID)}`, | ||
| navatticURL: getNavatticURL(environment, engagementChoice), | ||
| integrationName, | ||
| workspaceAccountingLink: `${environmentURL}/${ROUTES.POLICY_ACCOUNTING.getRoute(onboardingPolicyID)}`, | ||
| workspaceSettingsLink: `${environmentURL}/${ROUTES.WORKSPACE_INITIAL.getRoute(onboardingPolicyID)}`, | ||
| }) | ||
| : task.description; | ||
| const taskTitle = | ||
|
|
@@ -3644,7 +3647,9 @@ function prepareOnboardingOptimisticData( | |
| type: 'task', | ||
| task: task.type, | ||
| taskReportID: currentTask.reportID, | ||
| // API expects a string, that's why policyID must default to a string | ||
| // eslint-disable-next-line rulesdir/no-default-id-values | ||
| parentReportID: currentTask.parentReportID ?? '', | ||
|
There was a problem hiding this comment.
Are you sure? How was it before TS migration? Is it be possible to call this API call without There was a problem hiding this comment. @VickyStash I'm not sure what the API expects or how to test it, but currently we send '-1' in case There was a problem hiding this comment. Answered here: #54306 (comment) |
||
| parentReportActionID: taskReportAction.reportAction.reportActionID, | ||
| assigneeChatReportID: '', | ||
| createdTaskReportActionID: taskCreatedAction.reportActionID, | ||
|
|
@@ -4087,7 +4092,9 @@ function searchForReports(searchInput: string, policyID?: string) { | |
| }, | ||
| ]; | ||
|
|
||
| // API expects a string, that's why policyID must default to a string | ||
| // eslint-disable-next-line rulesdir/no-default-id-values | ||
| const searchForRoomToMentionParams: SearchForRoomsToMentionParams = {query: searchInput, policyID: policyID ?? ''}; | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. @VickyStash I'm not sure what the API expects or how to test it, but currently we send '-1' in case There was a problem hiding this comment. Answered here: #54306 (comment) |
||
| const searchForReportsParams: SearchForReportsParams = {searchInput, canCancel: true}; | ||
|
|
||
| // We want to cancel all pending SearchForReports API calls before making another one | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think the initial idea of this ESlint change was to make sure the real values are passed where needed. And the missing data to be correctly handled where the function is being called.
For instance, we shouldn't change these
getRoutefunctions, but throw something when we don't have thepolicyIDwhen callingWORKSPACE_INITIAL.getRoute().@iwiznia please correct me if I'm wrong. And what should we do when we don't have a policy ID but need to navigate to this page?
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.
I don't think any of these routes should be used by passing an empty policyID
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.
Yeah, I think it makes more sense to add the condition on level above to make sure
WORKSPACE_INITIAL.getRoute(...)not called without policy id. Same for other cases here.@pac-guerreiro let's try to do it so
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.
@iwiznia @paultsimura @VickyStash
getRoute(...)is mostly used inNavigation.navigate(...)and I'm worried that if we prevent it from running we'll disrupt normal app flowThere 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.
Can't we just do it like this
ROUTES.REPORT_WITH_ID.getRoute(`${report.preexistingReportID}`)?This way we still navigate the user to the new page but we pass undefined as string
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.
Have you tried to navigate to the page with invalid id? What results do you get
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.
@VickyStash if the id is invalid, the page will show a
Not foundpage but it seems that all reports in Onyx are deleted