Skip to content
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.

Sign up for GitHub

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

[No QA] Remove default values for IDs to comply with eslint rules #54306

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+70 −58
Open

[No QA] Remove default values for IDs to comply with eslint rules #54306

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0612e06
fix: undo default values to comply with eslint rules
pac-guerreiro Dec 18, 2024
f927a37
doc: fix typo in comment
pac-guerreiro Dec 18, 2024
8d1cf91
Merge branch 'main' into pac-guerreiro/fix/50360-undo-default-values-…
pac-guerreiro Dec 19, 2024
7f97310
chore: apply suggestions and follow guidelines
pac-guerreiro Dec 19, 2024
430a195
Merge branch 'main' into pac-guerreiro/fix/50360-undo-default-values-…
pac-guerreiro Dec 19, 2024
2e07d76
Merge branch 'main' into pac-guerreiro/fix/50360-undo-default-values-…
pac-guerreiro Dec 20, 2024
4be070e
chore: resolve eslint issues
pac-guerreiro Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/ROUTES.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@
REPORT: 'r',
REPORT_WITH_ID: {
route: 'r/:reportID?/:reportActionID?',
getRoute: (reportID: string, reportActionID?: string, referrer?: string) => {
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;
Expand Down Expand Up @@ -698,7 +698,7 @@
WORKSPACE_NEW_ROOM: 'workspace/new-room',
WORKSPACE_INITIAL: {
route: 'settings/workspaces/:policyID',
getRoute: (policyID: string, backTo?: string) => `${getUrlWithBackToParam(`settings/workspaces/${policyID}`, backTo)}` as const,
getRoute: (policyID = '', backTo?: string) => `${getUrlWithBackToParam(`settings/workspaces/${policyID}`, backTo)}` as const,
Copy link
Contributor

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 getRoute functions, but throw something when we don't have the policyID when calling WORKSPACE_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?

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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 in Navigation.navigate(...) and I'm worried that if we prevent it from running we'll disrupt normal app flow

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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 found page but it seems that all reports in Onyx are deleted

},
WORKSPACE_INVITE: {
route: 'settings/workspaces/:policyID/invite',
Expand Down Expand Up @@ -923,7 +923,7 @@
},
WORKSPACE_MEMBERS: {
route: 'settings/workspaces/:policyID/members',
getRoute: (policyID: string) => `settings/workspaces/${policyID}/members` as const,
getRoute: (policyID = '') => `settings/workspaces/${policyID}/members` as const,
},
WORKSPACE_MEMBERS_IMPORT: {
route: 'settings/workspaces/:policyID/members/import',
Expand All @@ -935,7 +935,7 @@
},
POLICY_ACCOUNTING: {
route: 'settings/workspaces/:policyID/accounting',
getRoute: (policyID: string, newConnectionName?: ConnectionName, integrationToDisconnect?: ConnectionName, shouldDisconnectIntegrationBeforeConnecting?: boolean) => {
getRoute: (policyID = '', newConnectionName?: ConnectionName, integrationToDisconnect?: ConnectionName, shouldDisconnectIntegrationBeforeConnecting?: boolean) => {
let queryParams = '';
if (newConnectionName) {
queryParams += `?newConnectionName=${newConnectionName}`;
Expand Down Expand Up @@ -972,7 +972,7 @@
},
WORKSPACE_CATEGORIES: {
route: 'settings/workspaces/:policyID/categories',
getRoute: (policyID: string) => `settings/workspaces/${policyID}/categories` as const,
getRoute: (policyID = '') => `settings/workspaces/${policyID}/categories` as const,
},
WORKSPACE_CATEGORY_SETTINGS: {
route: 'settings/workspaces/:policyID/category/:categoryName',
Expand Down Expand Up @@ -1037,7 +1037,7 @@
},
WORKSPACE_MORE_FEATURES: {
route: 'settings/workspaces/:policyID/more-features',
getRoute: (policyID: string) => `settings/workspaces/${policyID}/more-features` as const,
getRoute: (policyID = '') => `settings/workspaces/${policyID}/more-features` as const,
},
WORKSPACE_TAGS: {
route: 'settings/workspaces/:policyID/tags',
Expand Down Expand Up @@ -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

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

},
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

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

},
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

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

},
WORKSPACE_REPORT_FIELDS_EDIT_VALUE: {
route: 'settings/workspaces/:policyID/reportFields/new/:valueIndex/edit',
Expand Down
53 changes: 30 additions & 23 deletions src/libs/actions/Report.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: string,
reportID = '',
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
reportActionID?: string,
participantLoginList: string[] = [],
newReportObject?: ReportUtils.OptimisticChatReport,
parentReportActionID = '-1',
parentReportActionID = '',
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
isFromDeepLink = false,
participantAccountIDList: number[] = [],
avatar?: File | CustomRNImageManipulatorResult,
Expand Down Expand Up @@ -1065,7 +1065,7 @@ function openReport(
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${newReportObject.parentReportID}`,
value: {[parentReportActionID]: {childReportID: '-1', childType: ''}},
value: {[parentReportActionID]: {childReportID: '', childType: ''}},
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
});
}
}
Expand Down Expand Up @@ -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);
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 ?? '-1'), actionType);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report?.reportID), actionType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure it's not called if there is no report?.reportID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VickyStash

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to test it as I mentioned here. What happens if you navigate user to the screen with -1 id

Copy link
Contributor Author

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 found page but it seems that all reports in Onyx are deleted

}
}

Expand All @@ -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);
openReport(report?.reportID, '', [], newChat, '0', false, participantAccountIDs);
Navigation.dismissModalWithReport(report);
}

Expand All @@ -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 = '-1', parentReportAction: Partial<ReportAction> = {}, parentReportID = '0') {
if (childReportID !== '-1' && childReportID !== '0') {
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)])];
Expand Down Expand Up @@ -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 === '-1') {
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;
}
Expand Down Expand Up @@ -1482,7 +1482,7 @@ function handleReportChanged(report: OnyxEntry<Report>) {
const currCallback = callback;
callback = () => {
currCallback();
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.preexistingReportID ?? '-1'), CONST.NAVIGATION.TYPE.UP);
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
Expand All @@ -1503,7 +1503,7 @@ function handleReportChanged(report: OnyxEntry<Report>) {
return;
}

saveReportDraftComment(report.preexistingReportID ?? '-1', draftReportComment, callback);
saveReportDraftComment(report.preexistingReportID, draftReportComment, callback);

return;
}
Expand Down Expand Up @@ -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 = '-1', parentReportAction: Partial<ReportAction> = {}, parentReportID = '-1', prevNotificationPreference?: NotificationPreference) {
if (childReportID !== '-1') {
function toggleSubscribeToChildReport(childReportID = '', parentReportAction: Partial<ReportAction> = {}, parentReportID = '', prevNotificationPreference?: NotificationPreference) {
if (childReportID !== '') {
openReport(childReportID);
const parentReportActionID = parentReportAction?.reportActionID ?? '-1';
// 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 {
Expand Down Expand Up @@ -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 ?? '-1');
const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID);
Navigation.goBack(lastAccessedReportRoute);
} else {
const isChatThread = ReportUtils.isChatThread(currentReport);
Expand Down Expand Up @@ -3587,14 +3590,14 @@ function prepareOnboardingOptimisticData(
const taskDescription =
typeof task.description === 'function'
? task.description({
adminsRoomLink: `${environmentURL}/${ROUTES.REPORT_WITH_ID.getRoute(adminsChatReportID ?? '-1')}`,
workspaceCategoriesLink: `${environmentURL}/${ROUTES.WORKSPACE_CATEGORIES.getRoute(onboardingPolicyID ?? '-1')}`,
workspaceMembersLink: `${environmentURL}/${ROUTES.WORKSPACE_MEMBERS.getRoute(onboardingPolicyID ?? '-1')}`,
workspaceMoreFeaturesLink: `${environmentURL}/${ROUTES.WORKSPACE_MORE_FEATURES.getRoute(onboardingPolicyID ?? '-1')}`,
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 ?? '-1')}`,
workspaceSettingsLink: `${environmentURL}/${ROUTES.WORKSPACE_INITIAL.getRoute(onboardingPolicyID ?? '-1')}`,
workspaceAccountingLink: `${environmentURL}/${ROUTES.POLICY_ACCOUNTING.getRoute(onboardingPolicyID)}`,
workspaceSettingsLink: `${environmentURL}/${ROUTES.WORKSPACE_INITIAL.getRoute(onboardingPolicyID)}`,
})
: task.description;
const taskTitle =
Expand Down Expand Up @@ -3644,7 +3647,9 @@ function prepareOnboardingOptimisticData(
type: 'task',
task: task.type,
taskReportID: currentTask.reportID,
parentReportID: currentTask.parentReportID ?? '-1',
// API expects a string, that's why parentReportID must default to a string
// eslint-disable-next-line rulesdir/no-default-id-values
parentReportID: currentTask.parentReportID ?? '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Should it be possible to call this request without a valid parentReportID?
  2. If yes - can we pass undefined instead of '' string?
  3. If no - add a condition to prevent it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VickyStash

I don't know that information, where can I get it?

Copy link
Contributor

@VickyStash VickyStash Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideas:

  • try to call the API call with '', -1 and with undefined as parentReportID - check if it makes any difference
  • check if the defaulting value always was there

parentReportActionID: taskReportAction.reportAction.reportActionID,
assigneeChatReportID: '',
createdTaskReportActionID: taskCreatedAction.reportActionID,
Expand Down Expand Up @@ -4087,7 +4092,9 @@ function searchForReports(searchInput: string, policyID?: string) {
},
];

const searchForRoomToMentionParams: SearchForRoomsToMentionParams = {query: searchInput, policyID: policyID ?? '-1'};
// 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 ?? ''};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Should it be possible to call this request without a valid policyID?
  2. If yes - can we pass undefined instead of '' string?
  3. If no - add a condition to prevent it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VickyStash I'm not sure what the API expects or how to test it, but currently we send '-1' in case policyID is undefined

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered here: #54306 (comment)

const searchForReportsParams: SearchForReportsParams = {searchInput, canCancel: true};

// We want to cancel all pending SearchForReports API calls before making another one
Expand Down
Loading