[TS migration] Migrate MoneyTemporaryForRefactorRequestConfirmationList.js component to TypeScript
#37181
Conversation
|
Still working on the PR will update once it is ready for review |
Signed-off-by: Rohan Sasne <rohansasne30@gmail.com>
src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx
Outdated
Show resolved
Hide resolved
|
Is this ready for review? @GandalfGwaihir |
|
will be ready by monday, have to resolve the merge conflicts now 😅 |
|
@hoangzinh 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] |
|
PR is ready for review @hoangzinh @blazejkustra |
|
@hoangzinh @blazejkustra , i can't seem to figure out the exact type error below: When i type it as number it says number not assignable to string and when i type it as a string it says string not assignable to number, a little help over here please :) |
src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx
Outdated
Show resolved
Hide resolved
src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx
Outdated
Show resolved
Hide resolved
src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx
Outdated
Show resolved
Hide resolved
|
@GandalfGwaihir Bumping this 😄 |
|
sorry for not updating, actually this PR is similar to #37716, this was approved today and will get merged, should we wait to see how this PR performs in staging? the components used are exactly same, so maybe we want to wait but interested to hear what you think, Also i will comment on the review of yours bt today EOD and make necessary changes |
|
Ah these merge conflicts I tell you, umm I see a discussion going into this android built, Can't we get an internal engineer to approve this making an exception ? These changes didn't touch any native components, and as we tested on all other platforms I guess it would be okay to skip (If the android built didn't get fixed)! , what do you say @hoangzinh ? |
|
Also typescript fail is unrelated, it is failing in main |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #36130 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@GandalfGwaihir I can build it via VS so no worry. Reg TS, can you report it in the slack? |
|
@GandalfGwaihir @hoangzinh Looks like TS checks is passing on |
|
merged main, lets hope for the best 😅 |
|
@GandalfGwaihir I've checked your PR and here is my suggestion to fix the TS issues: diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts
index 85f5c414db..ad4632f96f 100644
--- a/src/libs/ReportUtils.ts
+++ b/src/libs/ReportUtils.ts
@@ -418,7 +418,7 @@ type OptionData = {
notificationPreference?: NotificationPreference | null;
isDisabled?: boolean | null;
name?: string | null;
- isSelfDM?: boolean | null;
+ isSelfDM?: boolean;
reportID?: string;
enabled?: boolean;
data?: Partial<TaxRate>;
diff --git a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
index 897af48f05..da12e30a00 100644
--- a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
+++ b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx
@@ -2,7 +2,6 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
-import type {ValueOf} from 'type-fest';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import * as Expensicons from '@components/Icon/Expensicons';
@@ -30,11 +29,12 @@ import ROUTES from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
import type {Policy, PolicyCategories, PolicyTagList} from '@src/types/onyx';
import type {Participant} from '@src/types/onyx/IOU';
+import type {PaymentMethodType} from '@src/types/onyx/OriginalMessage';
import type {Receipt} from '@src/types/onyx/Transaction';
import type {WithFullTransactionOrNotFoundProps} from './withFullTransactionOrNotFound';
import withFullTransactionOrNotFound from './withFullTransactionOrNotFound';
-import withWritableReportOrNotFound from './withWritableReportOrNotFound';
import type {WithWritableReportOrNotFoundProps} from './withWritableReportOrNotFound';
+import withWritableReportOrNotFound from './withWritableReportOrNotFound';
type IOURequestStepConfirmationOnyxProps = {
/** The policy of the report */
@@ -97,7 +97,7 @@ function IOURequestStepConfirmation({
transaction?.participants?.map((participant) => {
const participantAccountID = participant.accountID ?? 0;
return participantAccountID ? OptionsListUtils.getParticipantsOption(participant, personalDetails) : OptionsListUtils.getReportOption(participant);
- }),
+ }) ?? [],
[transaction?.participants, personalDetails],
);
const isPolicyExpenseChat = useMemo(() => ReportUtils.isPolicyExpenseChat(ReportUtils.getRootParentReport(report)), [report]);
@@ -422,11 +422,9 @@ function IOURequestStepConfirmation({
/**
* Checks if user has a GOLD wallet then creates a paid IOU report on the fly
- *
- * @param {String} paymentMethodType
*/
const sendMoney = useCallback(
- (paymentMethodType: ValueOf<typeof CONST.IOU.PAYMENT_TYPE>) => {
+ (paymentMethod: PaymentMethodType | undefined) => {
const currency = transaction?.currency;
const trimmedComment = transaction?.comment?.comment ? transaction.comment.comment.trim() : '';
@@ -437,12 +435,12 @@ function IOURequestStepConfirmation({
return;
}
- if (paymentMethodType === CONST.IOU.PAYMENT_TYPE.ELSEWHERE) {
+ if (paymentMethod === CONST.IOU.PAYMENT_TYPE.ELSEWHERE) {
IOU.sendMoneyElsewhere(report, transaction.amount, currency, trimmedComment, currentUserPersonalDetails.accountID, participant);
return;
}
- if (paymentMethodType === CONST.IOU.PAYMENT_TYPE.EXPENSIFY) {
+ if (paymentMethod === CONST.IOU.PAYMENT_TYPE.EXPENSIFY) {
IOU.sendMoneyWithWallet(report, transaction.amount, currency, trimmedComment, currentUserPersonalDetails.accountID, participant);
}
},
@@ -490,12 +488,11 @@ function IOURequestStepConfirmation({
/>
{isLoading && <FullScreenLoadingIndicator />}
<View style={[styles.flex1, isLoading && styles.opacity0]}>
- {/* @ts-expect-error TODO: Remove this once MoneyRequestConfirmationList (https://github.com/Expensify/App/issues/36130) is migrated to TypeScript. */}
<MoneyRequestConfirmationList
transaction={transaction}
hasMultipleParticipants={iouType === CONST.IOU.TYPE.SPLIT}
selectedParticipants={participants}
- iouAmount={transaction?.amount}
+ iouAmount={transaction?.amount ?? 0}
iouComment={transaction?.comment.comment ?? ''}
iouCurrencyCode={transaction?.currency}
iouIsBillable={transaction?.billable}
|
|
let me fix them ASAP, thanks for the help @fabioh8010 |
|
Fixed the type check fail, let's get this one merged ASAP, @youssef-lr 🚀 |
| <MoneyRequestConfirmationList | ||
| transaction={transaction} | ||
| hasMultipleParticipants={iouType === CONST.IOU.TYPE.SPLIT} | ||
| selectedParticipants={participants} | ||
| iouAmount={transaction?.amount} | ||
| iouAmount={transaction?.amount ?? 0} |
There was a problem hiding this comment.
I don't think this is necessary, unless it was causing a TS error?
| iouAmount={transaction?.amount ?? 0} | |
| iouAmount={transaction?.amount} |
There was a problem hiding this comment.
It is causing an TS error @youssef-lr, I checked and it is a required prop
There was a problem hiding this comment.
okay cool! Testing now locally, then I'll merge if nothing comes up
|
✋ 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/youssef-lr in version: 1.4.63-0 🚀
|
|
I think this PR may be causing the deploy blocker: #40476 |
|
@aldo-expensify , this is a known issue which was caused by another PR, we have a slack discussion on this as well, let me attach that here |
|
here is the slack convo: https://expensify.slack.com/archives/C01GTK53T8Q/p1713382047884229 Long story short, this was caused by #38709 And is already being fixed here #40385 |
|
Yeah, sorry I just noticed that too. Thanks for all the info! |
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀
|
Details
This is a typescript migration issue, the file was migrated from
jstotsxFixed Issues
$ #36130
PROPOSAL: #36130 (comment)
Tests
Same as QA step
Offline tests
Same as QA step
QA Steps
+icon(This is a migration PR, so test out all options available on the final confirmation page)
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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
MacOS: Chrome / Safari
simplescreenrecorder-2024-04-04_05.04.47.mp4
MacOS: Desktop
simplescreenrecorder-2024-04-04_05.08.08.mp4
iOS: mWeb Safari
WhatsApp.Video.2024-04-04.at.5.17.22.AM.1.mp4
Android: mWeb Chrome
simplescreenrecorder-2024-04-04_05.24.57.mp4
iOS: Native
WhatsApp.Video.2024-04-04.at.5.17.22.AM.mp4
Android: Native
simplescreenrecorder-2024-04-04_05.27.33.mp4