[No QA] [TS migration] Migrate 'TransactionUtils.js' lib to TypeScript #28005
Conversation
# Conflicts: # src/libs/TransactionUtils.js # src/types/onyx/Transaction.ts
melvin-bot
bot
requested review from
aimane-chnaif
and removed request for
a team
|
@aimane-chnaif 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] |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #24893 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
| }; | ||
| } | ||
|
|
||
| function hasReceipt(transaction: Transaction | undefined | null): boolean { |
There was a problem hiding this comment.
| function hasReceipt(transaction: Transaction | undefined | null): boolean { | |
| function hasReceipt(transaction?: Transaction | null): boolean { |
There was a problem hiding this comment.
@blazejkustra This way, it will be possible to call TransactionUtils.hasReceipt(); without any arguments, I am not sure that it's expected
|
Reviewing... |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromechrome.movMobile Web - Safarisafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
LGTM! Make sure to check suggestions from Błażej @VickyStash |
# Conflicts: # src/libs/TransactionUtils.js # src/types/onyx/OriginalMessage.ts # src/types/onyx/Transaction.ts
|
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
|
✋ 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/cristipaval in version: 1.3.76-0 🚀
|
| * Return the merchant field from the transaction, return the modifiedMerchant if present. | ||
| */ | ||
| function getMerchant(transaction: Transaction): string { | ||
| return transaction?.modifiedMerchant ?? transaction?.merchant ?? ''; |
There was a problem hiding this comment.
This caused a deploy blocker, the ?? operator only returns the right side of the operator if transaction?.modifiedMerchant is null but the transaction?.modifiedMerchant value in this case is `` an empty string so we return an empty string by mistake
| * Return the created field from the transaction, return the modifiedCreated if present. | ||
| */ | ||
| function getCreated(transaction: Transaction): string { | ||
| const created = transaction?.modifiedCreated ?? transaction?.created ?? ''; |
There was a problem hiding this comment.
This caused a deploy blocker, the ?? operator only returns the right side of the operator if transaction?. modifiedCreated is null but the transaction?. modifiedCreated value in this case is `` an empty string so we return an empty string by mistake
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀
|
| * Filters the waypoints which are valid and returns those | ||
| */ | ||
| function getValidWaypoints(waypoints: WaypointCollection, reArrangeIndexes = false): WaypointCollection { | ||
| const sortedIndexes = Object.keys(waypoints).map(getWaypointIndex).sort(); |
There was a problem hiding this comment.
@VickyStash I think this introduced a regression or is at least part of the cause for this issue #29621.
By default Array.prototype.sort() does the following. "The default sort order is ascending, built upon converting the elements into strings, then comparing their sequences of UTF-16 code units values.". So therefore the waypoints get out of order when you have 10+ because the number indexes are sorted as strings.
I'm working on a fix, which should be easy for this piece.
There was a problem hiding this comment.
cc @cristipaval since you reviewed as well
Details
Migrate 'TransactionUtils.js' lib to TypeScript
Fixed Issues
$ #24893
PROPOSAL: N/A
Tests
Make sure the Request Money functionality works as expected
Offline tests
QA Steps
Make sure the Request Money functionality works as expected
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
Web.mov
Mobile Web - Chrome
android.google.mp4
Mobile Web - Safari
ios.safari.mp4
Desktop
Desktop.mov
iOS
ios.mov
Android
Android.mov