-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Align default IDs in Report.ts file #54306
Align default IDs in Report.ts file #54306
Conversation
@paultsimura 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] |
cc @VickyStash |
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.
Left some comments, will take more thorough look tomorrow! 👀
src/libs/actions/Report.ts
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API expects a string
Are you sure? How was it before TS migration? Is it be possible to call this API call without parentReportID
or with invalid parentReportID
?
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 I'm not sure what the API expects or how to test it, but currently we send '-1' in case currentTask.parentReportID
is undefined
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.
Answered here: #54306 (comment)
src/ROUTES.ts
Outdated
getRoute: (policyID: string, backTo?: string) => `${getUrlWithBackToParam(`settings/workspaces/${policyID}`, backTo)}` as const, | ||
getRoute: (policyID = '', backTo?: string) => `${getUrlWithBackToParam(`settings/workspaces/${policyID}`, backTo)}` as const, |
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 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?
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 in Navigation.navigate(...)
and I'm worried that if we prevent it from running we'll disrupt normal app flow
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.
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 found
page but it seems that all reports in Onyx are deleted
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); |
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.
Make sure it's not called if there is no 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.
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.
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
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 found page but it seems that all reports in Onyx are deleted
src/libs/actions/Report.ts
Outdated
// 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 ?? '', |
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.
- Should it be possible to call this request without a valid
parentReportID
? - If yes - can we pass undefined instead of '' string?
- If no - add a condition to prevent it
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 know that information, where can I get it?
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.
Ideas:
- try to call the API call with
''
,-1
and withundefined
asparentReportID
- check if it makes any difference - check if the defaulting value always was there
…on-report-actions # Conflicts: # src/libs/actions/Report.ts
…on-report-actions
I applied most of the suggestions but left some questions 😄 |
@pac-guerreiro left suggestions to some of your questions! |
Today I resolved the remaining eslint issues 😄 @VickyStash I left some replies but I wasn't able to confirm what the API expects in some situations. I guess it should be face to pass undefined for those ids. I'll be away until January 2nd but someone should take care of my work 😄 Happy holidays! 🎉 |
Hey @pac-guerreiro, welcome back 👋
Have you tried what I suggested in this comment? |
src/libs/actions/Report.ts
Outdated
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}`)}`, |
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 guess from user perspective we will have the same result if we just pass adminsChatReportID
adminsRoomLink: `${environmentURL}/${ROUTES.REPORT_WITH_ID.getRoute(`${adminsChatReportID}`)}`, | |
adminsRoomLink: `${environmentURL}/${ROUTES.REPORT_WITH_ID.getRoute(adminsChatReportID)}`, |
Could you try to navigate the route with -1
and undefined
values, will it act the same?
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 You're right but if I didn't make this change, then eslint gets upset with me for passing an undefined value 😅
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 it can be resolved by just updating the type, right?
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.
UPD: Similar case is discussed in another PR #54534 (comment)
…on-report-actions
…on-report-actions
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.
@pac-guerreiro Please don't forget to remove src/libs/actions/Report.ts
from .eslintrc.changed.js
Also @pac-guerreiro please remove |
…on-report-actions
…on-report-actions
src/libs/actions/Task.ts
Outdated
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.
Can you please resolve lint error related to import, it doesn't look complex, just 4 usages.
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 told @pac-guerreiro to do it in a follow-up PR in order to avoid further delays here
src/libs/actions/Report.ts
Outdated
if (!onboardingData) { | ||
return; | ||
} |
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.
Are you sure we should end openReport
function execution if there is no onboardingData
?
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 this was suggested by @fabioh8010. I think it makes sense to stop openReport
execution if something is not right, but I guess we could add a Log.warn 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.
Answered here.
src/libs/actions/Report.ts
Outdated
if (!targetChatReportID) { | ||
return null; | ||
} |
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'm not sure about this return.
Since it is located not in the top of the function, maybe we can add Log.warn
so it's easy to catch it up?
Or maybe we can try to handle without it at all.
Wdyt? @fabioh8010
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 this was suggested by @fabioh8010. I agree with adding a Log.warn 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.
Yeah this case is hard because there is no way to make targetChatReportID
be always defined as it comes from an Onyx value extracted from a collection, which is not guaranteed to be defined. In this case looks like making targetChatReportID
be assigned a default value could be the only option, but it defeats our very purpose here, so I suggested to stop function's execution (making it return undefined
or null
), and the places where it's called will have to handle it by either doing some other logic or stop their execution too.
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.
Also @pac-guerreiro I'm assuming you tested extensively these parts to make sure this change works as expect, right?
Co-authored-by: Viktoryia Kliushun <[email protected]>
src/libs/actions/Report.ts
Outdated
if (!targetChatReportID) { | ||
return null; | ||
} |
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 this case is hard because there is no way to make targetChatReportID
be always defined as it comes from an Onyx value extracted from a collection, which is not guaranteed to be defined. In this case looks like making targetChatReportID
be assigned a default value could be the only option, but it defeats our very purpose here, so I suggested to stop function's execution (making it return undefined
or null
), and the places where it's called will have to handle it by either doing some other logic or stop their execution too.
…on-report-actions # Conflicts: # .eslintrc.changed.js # src/ROUTES.ts
Today I resolved conflicts and applied @fabioh8010 and @VickyStash suggestions 😄 |
@iwiznia @paultsimura the PR is ready for your final review 😄 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.webmAndroid: mWeb Chromechrome.webmiOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-17.at.11.49.09.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-17.at.11.50.01.mp4MacOS: Chrome / SafariScreen.Recording.2025-01-17.at.11.46.27.movMacOS: DesktopScreen.Recording.2025-01-17.at.11.47.37.mov |
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 ✔️
Note: the lint issue will be fixed in a follow-up PR.
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.
Looks pretty solid. A couple small requests.
@neil-marcellini I just applied the changes you requested, thank you 😄 |
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.
Good to go, thank you
@neil-marcellini looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not an emergency. Lint will be fixed in a following PR. |
✋ 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/neil-marcellini in version: 9.0.89-0 🚀
|
#55676 found when running this PR |
A note for the BZ team: |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.89-8 🚀
|
Explanation of Change
Fixed Issues
$ #50360
PROPOSAL: #50360 (comment)
Tests
Reply in thread
on any commentEdit expense
from your personal reportOffline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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
Android.-.Native.mp4
Android: mWeb Chrome
Android.-.Chrome.mp4
iOS: Native
iOS.-.Native.mp4
iOS: mWeb Safari
iOS.-.Safari.mp4
MacOS: Chrome / Safari
MacOS.-.Chrome.mp4
MacOS: Desktop
MacOS.-.Desktop.mp4