Undo defaults values for onyx ids PureReportActionItem#55536
Conversation
…to undo-defaults-values-purereportactionitem
…to undo-defaults-values-purereportactionitem
…to undo-defaults-values-purereportactionitem
…to undo-defaults-values-purereportactionitem
…to undo-defaults-values-purereportactionitem
fabioh8010
left a comment
fabioh8010
left a comment
There was a problem hiding this comment.
You have some lint errors
| } | ||
|
|
||
| const renderItem = ({item}: {item: TripReservationUtils.ReservationData}) => <ReservationView reservation={item.reservation} />; | ||
| const renderItem = ({item}: {item: ReservationData}) => <ReservationView reservation={item.reservation} />; |
There was a problem hiding this comment.
| const renderItem = ({item}: {item: ReservationData}) => <ReservationView reservation={item.reservation} />; | |
| const renderItem = ({item}: ListRenderItemInfo<ReservationData>) => <ReservationView reservation={item.reservation} />; |
There was a problem hiding this comment.
This doesnt look right
The type from FlatList looks like that
And when I am changing to whcih you suggested seems to be completely different shape so it will not work well
| () => ({ | ||
| anchor: popoverAnchorRef.current, | ||
| report: {...report, reportID: report?.reportID ?? ''}, | ||
| report: report?.reportID ? {...report, reportID: report?.reportID} : undefined, |
There was a problem hiding this comment.
Let's make sure about this, looks like a dangerous change 👀
There was a problem hiding this comment.
hmm actually shouldn't we just do report: {...report} ? since if there is a report there's also reportID right? there is no way that we have a report without reportID @fabioh8010
| report: OnyxTypes.Report; | ||
| action: OnyxTypes.ReportAction; | ||
| }; | ||
| contextValue: ShowContextMenuContextProps; |
There was a problem hiding this comment.
Because of the change above https://github.com/Expensify/App/pull/55536/files/84d6eb5c1c2932b042ad1b3cde8ffbf5bd0d73ec#diff-a7f41a67ae98f78eb397d452128d1a83b5b142976e61efe2302e52853604cf3a
I also checked it passed just undefined to PureReportActionItem and worked fine will do couple more tests to make sure 100%
There was a problem hiding this comment.
But can we not make this change? It looks like a regression magnet to me.
There was a problem hiding this comment.
I think we still should do it I trace how report is passed here and its always comes from Onyx so its possible that report here will be undefined as we removed defaults in PureReportActionItem so previous type was wrong
…to undo-defaults-values-purereportactionitem
kubabutkiewicz
changed the title
melvin-bot
Bot
requested review from
paultsimura
and removed request for
a team
|
@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] |
|
@kubabutkiewicz Let's remove |
kubabutkiewicz
changed the title
|
@kubabutkiewicz please resolve conflicts 🙏 |
…to undo-defaults-values-purereportactionitem
…to undo-defaults-values-purereportactionitem
|
|
||
| const currentReportID = useCurrentReportID(); | ||
| const currentReportIDValue = currentReportIDContext || currentReportID?.currentReportID; | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing |
There was a problem hiding this comment.
Looks like this can be removed
There was a problem hiding this comment.
I think we should leave it if currentReportIDContext would be '' the ?? operator wont work and will assign to currentReportIDValue '' but if we use || it will correctly assign currentReportID?.currentReportID. And by default in MentionReportContext currentReportID is ''
There was a problem hiding this comment.
I meant the // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing only, because my linter didn't throw any warning with it removed.
Looks like it was just slow, let's keep it then.
|
|
||
| /** Saves a new message for a comment. Marks the comment as edited, which will be reflected in the UI. */ | ||
| function editReportComment(reportID: string, originalReportAction: OnyxEntry<ReportAction>, textForNewComment: string, videoAttributeCache?: Record<string, string>) { | ||
| function editReportComment(reportID: string | undefined, originalReportAction: OnyxEntry<ReportAction>, textForNewComment: string, videoAttributeCache?: Record<string, string>) { |
There was a problem hiding this comment.
Why not return early if !reportID here and in the functions below?
There was a problem hiding this comment.
When I do this the the TS will complain about this
const parameters: UpdateCommentParams = {
reportID: originalReportID,
reportComment: htmlForNewComment,
reportActionID,
};
as originalReportID is of type string | undefined and UpdateCommentParams expect reportID will be of type string but when I do this
if (!originalReportID || !originalReportAction) {
return;
}
everything looks good
| report: OnyxTypes.Report; | ||
| action: OnyxTypes.ReportAction; | ||
| }; | ||
| contextValue: ShowContextMenuContextProps; |
There was a problem hiding this comment.
But can we not make this change? It looks like a regression magnet to me.
|
Ok @paultsimura I think all comments are resolved/answered |
|
Thanks, giving it another 👀 soon! |
…to undo-defaults-values-purereportactionitem
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.webmAndroid: mWeb ChromeChrome.webmiOS: NativeiOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-24.at.13.38.40.mp4MacOS: Chrome / SafariScreen.Recording.2025-01-24.at.13.32.58.movMacOS: DesktopScreen.Recording.2025-01-24.at.13.35.56.mov |
|
@kubabutkiewicz let's fix the conflicts one last time and we're g2g ✔️ |
…to undo-defaults-values-purereportactionitem
|
@paultsimura conflicts resolved |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for all the hard work. I have some requests.
| description={translate('task.description')} | ||
| title={report.description ?? ''} | ||
| onPress={() => Navigation.navigate(ROUTES.REPORT_DESCRIPTION.getRoute(report.reportID, Navigation.getReportRHPActiveRoute()))} | ||
| title={report?.description ?? ''} |
There was a problem hiding this comment.
This should not be defaulted
There was a problem hiding this comment.
Are we sure about that? the new rule is only for IDs
There was a problem hiding this comment.
Ah right good point
…to undo-defaults-values-purereportactionitem
|
@neil-marcellini I resolved comments from you, you can recheck! |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Good to go. Thanks for the updates.
|
✋ 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.90-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.90-6 🚀
|
| import {getDisplayNameForParticipant, getDisplayNamesWithTooltips, isCompletedTaskReport, isOpenTaskReport} from '@libs/ReportUtils'; | ||
| import {isActiveTaskEditRoute} from '@libs/TaskUtils'; | ||
| import {checkIfActionIsAllowed} from '@userActions/Session'; | ||
| import {canActionTask as canActionTaskUtil, canModifyTask as canModifyTaskUtil, clearTaskErrors, completeTask, reopenTask, setTaskReport} from '@userActions/Task'; |
There was a problem hiding this comment.
@kubabutkiewicz, in this PR, I see a lot of imports are being renamed like <nameOfImport>Util. Can you please explain to me why this was done? I don't think it's necessary and it makes search for code references very difficult. If it's not necessary, could you please create a PR to cleanup and remove all the aliases that were added?
There was a problem hiding this comment.
@tgolen, sure, it was done because in the files we are importing methods, there are naming conflicts so for example there is already a variable/function created with the same name, thats why we needed to add an alias to resolve those naming conflicts for example in this file
App/src/components/ReportActionItem/TaskView.tsx
Lines 60 to 61 in 8870ad7
There was a problem hiding this comment.
In those instances, renaming the variable would be more preferred than aliasing the import. For example, these variables should be named isTaskModifiable and isTaskActionable
6 participants
Explanation of Change
Fixed Issues
$ #55745
PROPOSAL:
Tests
Login to app
Go to some chat
Send a message
Open context menu
Add emoji
Open context menu
Create a thread
Open context menu
Delete message
Open context menu
Edit message
Login to app
Go to some workspace chat
Submit an manual expense
Go again to some workspace chat
Send a Distance expense
Go again to workspace chat
Send a scan
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
