[No QA] [TS migration] Migrate 'ReportActionsUtils.js' lib to TypeScript#28570
Conversation
rezkiy37
left a comment
There was a problem hiding this comment.
LGTM, after resolving comments.
|
FYI I will be on vacations starting tomorrow for the next two weeks and will return on Oct 23, so @kubabutkiewicz will replace me on this PR! |
src/libs/ReportActionsUtils.ts
Outdated
| import {ValueOf} from 'type-fest'; | ||
| import CONST from '../CONST'; | ||
| import ONYXKEYS from '../ONYXKEYS'; | ||
| import * as OnyxTypes from '../types/onyx'; |
There was a problem hiding this comment.
NAB: I would import types a bit different:
import ReportActions from '../../types/onyx/ReportActions';
src/libs/ReportActionsUtils.ts
Outdated
| _.has(reportAction.originalMessage, 'IOUDetails') | ||
| ); | ||
| function isSentMoneyReportAction(reportAction: OnyxEntry<OnyxTypes.ReportAction>): boolean { | ||
| return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && reportAction.originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && !!reportAction.originalMessage.IOUDetails; |
There was a problem hiding this comment.
| return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && reportAction.originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && !!reportAction.originalMessage.IOUDetails; | |
| return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && reportAction?.originalMessage?.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && !!reportAction?.originalMessage?.IOUDetails; |
| (originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.CREATE || (originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && _.has(originalMessage, 'IOUDetails'))) | ||
| parentReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU && | ||
| (parentReportAction.originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.CREATE || | ||
| (parentReportAction.originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && !!parentReportAction.originalMessage.IOUDetails)) |
There was a problem hiding this comment.
Logic was changed here on purpose? originalMessage.type -> parentReportAction.originalMessage.type
There was a problem hiding this comment.
Actually there is no logic change, here Fabio just removed const originalMessage = lodashGet(parentReportAction, 'originalMessage', {});
and just use parentReportAction.originalMessage etc
src/libs/ReportActionsUtils.ts
Outdated
| function isCreatedTaskReportAction(reportAction) { | ||
| return reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && _.has(reportAction.originalMessage, 'taskReportID'); | ||
| function isCreatedTaskReportAction(reportAction: OnyxEntry<OnyxTypes.ReportAction>): boolean { | ||
| return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && Boolean(reportAction.originalMessage?.taskReportID); |
There was a problem hiding this comment.
| return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && Boolean(reportAction.originalMessage?.taskReportID); | |
| return reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !!reportAction.originalMessage?.taskReportID; |
src/libs/ReportActionsUtils.ts
Outdated
| if (reportAction && Object.hasOwn(reportAction, 'isAttachment')) { | ||
| // The condition asserts "isAttachment" exists. | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| return reportAction.isAttachment!; | ||
| } |
There was a problem hiding this comment.
| if (reportAction && Object.hasOwn(reportAction, 'isAttachment')) { | |
| // The condition asserts "isAttachment" exists. | |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
| return reportAction.isAttachment!; | |
| } | |
| if (reportAction && 'isAttachment' in reportAction) { | |
| return reportAction.isAttachment ?? false; | |
| } |
|
@blazejkustra comments are resolved, can you recheck? |
|
@thesahindia 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] |
|
@thesahindia Hi, will you be able to check it asap? this is a blocker for few other TS migrations and would be perfect to merge it soon 😄 |
|
Hey @fabioh8010, can you please resolve the conflicts? |
…b/ReportActionsUtils
|
@thesahindia resolved |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-10-13.at.2.46.14.AM.movMobile Web - ChromeScreen.Recording.2023-10-13.at.3.09.21.AM.movMobile Web - SafariSimulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-10-13.at.23.19.45.mp4DesktopScreen.Recording.2023-10-13.at.2.56.03.AM.moviOSSimulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-10-13.at.23.16.20.mp4AndroidScreen.Recording.2023-10-13.at.10.08.54.PM.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #24910 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
neil-marcellini
left a comment
There was a problem hiding this comment.
I left some non blocking comments. It's looking good, thanks.
src/libs/ReportActionsUtils.ts
Outdated
| * | ||
| * @param {Object} reportAction report action | ||
| * @returns {Boolean} | ||
| * @param reportAction report action |
There was a problem hiding this comment.
NAB: Why leave this JSDoc? It seems kind of useless.
src/libs/ReportActionsUtils.ts
Outdated
| } | ||
|
|
||
| if (message) { | ||
| return isReportMessageAttachment(message) ?? false; |
There was a problem hiding this comment.
NAB: I don't think we need ?? false if isReportMEssageAttachment returns a boolean already.
|
@neil-marcellini Comments resolved can you check/approve again? |
|
@kubabutkiewicz good to go but we have conflicts |
…b/ReportActionsUtils
|
@neil-marcellini resolved 😄 |
|
✋ 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: 1.3.87-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.87-0 🚀
|
| // Then fallback on reportActionID as the final sorting criteria. It is a random number, | ||
| // but using this will ensure that the order of reportActions with the same created time and action type | ||
| // will be consistent across all users and devices | ||
| return (first.reportActionID < second.reportActionID ? -1 : 1) * invertedMultiplier; |
There was a problem hiding this comment.
Seems like this must be returning an object instead of an array 🤔 based on the stacktrace shared here #29965 (comment)
There was a problem hiding this comment.
@mountiny it should be vice versa. This function should return an array. reportActions should be an array of reportAction objects
There was a problem hiding this comment.
This return value affects only sort logic.
No doubt reportActions.filter(Boolean).sort always returns array.
So this function should not be related to the root cause of #29965
There was a problem hiding this comment.
Yep you are right, this should have gone to the getMostRecentIOURequestActionID method
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.87-12 🚀
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.88-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.88-11 🚀
|
|
|
||
| type OriginalMessageReimbursementQueued = { | ||
| actionName: typeof CONST.REPORT.ACTIONS.TYPE.REIMBURSEMENTQUEUED; | ||
| originalMessage: unknown; |
There was a problem hiding this comment.
@fabioh8010 Could you comment on this typing? Why is it unknown? Do we really not know the type?
There was a problem hiding this comment.
At that time we didn't know the type (same as other types previously declared here), that's why we typed as unknown.
Details
Fixed Issues
$ #24910
PROPOSAL: N/A
Tests
Report actions testing
Offline tests
QA Steps
Same as above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Screen.Recording.2023-10-01.at.21.58.21.mp4
Screen.Recording.2023-10-01.at.22.04.32.mp4
Desktop
Screen.Recording.2023-10-01.at.22.09.40.mp4
Mobile Web - Chrome
Screen.Recording.2023-10-01.at.22.15.19.mp4
Android
Screen.Recording.2023-10-01.at.22.20.47.mp4
Mobile Web - Safari
Screen.Recording.2023-10-01.at.22.28.26.mp4
iOS
Screen.Recording.2023-10-01.at.22.34.19.mp4