Skip to content
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

[TS migration] Migrate '[Remaining Group 2]' lib to TypeScript #32164

Merged
merged 43 commits into from
Jan 9, 2024

Conversation

VickyStash
Copy link
Contributor

@VickyStash VickyStash commented Nov 29, 2023

Details

This PR migrates next lib files to TS:

  • TransactionEdit.js
  • TeachersUnite.js
  • OnyxUpdateManager.js
  • DemoActions.js
  • Card.js
  • CanvasSize.js
  • MemoryOnlyKeys folder

Fixed Issues

$ #31922
PROPOSAL: N/A

Tests

  • Verify that no errors appear in the JS console
TransactionEdit
  1. Create a distance request.
  2. Navigate to the workspace chat which you created the distance request in. Click on the distance request preview to open the distance request.
  3. Select the "Distance" menu item => Close it / Go Back => Open it again => Input new waypoints.
  4. Select the "Distance" menu item again => Input new waypoints.
  5. Click "Save". Verify that a loading spinner appears on the Save button and the modal closes after the transaction is saved.
  6. After a bit (this could take a few seconds) verify the distance request amount and the title in the "Distance" menu item are updated to reflect your new waypoints.
  7. Try to update the description/date fields, and make sure it works.
TeachersUnite
  1. Click on FAB
  2. Click on Save the World
  3. There should be 2 options (I know a teacher and I am a teacher)
  4. Click on "I know a teacher"
  5. Provide information for the user you're referring and confirm.
  6. Make sure you're redirected to a public room with a whisper message indicating you referred them.
  7. Click on "I am a teacher" and sign providing the principal's information.
  8. Verify a policy expense chat has been created.
OnyxUpdateManager

Confirm that the app continues to work as expected on normal flows:

  • sending messages
  • receiving messages
  • creating workspaces

Besides that, you can use Onyx.set('onyxUpdatesFromServer', {INVALID_FORMAT_FROM_LIST_BELOW}); in the console to set that property to an invalid format and confirm it will log [OnyxUpdateManager] Invalid format found for updates, cleaning and unpausing the queue
you can test the following wrong formats:

  • Onyx.set('onyxUpdatesFromServer', {'type': ''});
  • Onyx.set('onyxUpdatesFromServer', {'type': 'https'});
  • Onyx.set('onyxUpdatesFromServer', {'type': 'https', 'request': {}});
  • Onyx.set('onyxUpdatesFromServer', {'type': 'pusher', 'request': {}, 'response': {}});
DemoActions
  1. Open the app and sign out
  2. Manually navigate to /money2020
  3. Sign in with a brand new account
  4. Verify you're automatically navigated to a new chat report, between your account & money2020@expensify.com
Card
  1. add useEffect with test data to the WalletPage.js
useEffect(() => {
        if (cardList[1] || cardList[2]) {
            return;
        }
        // eslint-disable-next-line rulesdir/prefer-actions-set-data
        Onyx.merge('cardList', {
            1: {
                availableSpend: 1000000,
                bank: 'Expensify Card',
                cardID: 1,
                cardholderFirstName: 'FirstName',
                cardholderLastName: 'LastName',
                domainName: 'expensify.com',
                fraud: 'none',
                isVirtual: false,
                lastFourPAN: '1234',
                state: 3,
            },
            2: {
                availableSpend: 1000000,
                bank: 'Expensify Card',
                cardID: 2,
                cardholderFirstName: 'FirstName',
                cardholderLastName: 'LastName',
                domainName: 'expensify.com',
                fraud: 'none',
                isVirtual: true,
                lastFourPAN: '9876',
                state: 3,
            },
        });
    }, [cardList]);
  1. Go to the Settings/Wallet -> you will see the Expensify card in the assigned cards
  2. Click on it, look through the card page, make sure it looks ok.
  3. Click on Reveal Details
  4. An error should appear under this menu item, as the card is fake
CanvasSize
  1. Go to any report
  2. Click on add attachment and select a pdf with big pages, for example this one
  3. Verify that the pdf is displayed correctly in the preview (it can take some time for it to preload).

On Mobile Web it gives next error (same on the main branch):
image

MemoryOnlyKeys
  1. Sign the app
  2. Run enableMemoryOnlyKeys() from the JS console
  3. Verify the logs say [MemoryOnlyKeys] enabled
  4. Run disableMemoryOnlyKeys() from the JS console
  5. Verify the logs say [MemoryOnlyKeys] disabled

Offline tests

N/A

QA Steps

  • Verify that no errors appear in the JS console

Same as in Tests section

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

- TransactionEdit.js

android_transaction1.mp4
android_transaction2.mp4

- TeachersUnite.js
android_teacher3

android_teacher2.mp4
android_teacher1.mp4

- OnyxUpdateManager.js

android_onyxUpdateManager.mp4

- Card.js

android_card.mp4

- CanvasSize.js

android_canvas.mp4
Android: mWeb Chrome

- TransactionEdit.js

android_web_transaction.mp4

- TeachersUnite.js
android_web_teacher3

android_web_teacher2.mp4
android_web_teacher1.mp4

- OnyxUpdateManager.js

android_web_onyx.mp4

- DemoActions.js
android_web_money2020

- Card.js

android_web_card.mp4
iOS: Native

- TransactionEdit.js

ios_transaction.mp4

- TeachersUnite.js
ios_teacher3

ios_teacher2.mp4
ios_teacher1.mp4

- OnyxUpdateManager.js

ios_OnyxManager.mp4

- Card.js

ios_card.mp4

- CanvasSize.js

ios_canvas.mp4
iOS: mWeb Safari

- TransactionEdit.js

ios_web_transaction.mp4

- TeachersUnite.js
ios_web_teacher3

ios_web_teacher2.mp4
ios_web_teacher1.mp4

- OnyxUpdateManager.js

ios_onyxManager.mp4

- DemoActions.js
ios_web_money2020

- Card.js

ios_web_card.mp4
MacOS: Chrome / Safari

- TransactionEdit.js

web_transaction_edit.mp4

- TeachersUnite.js
web_teacher3

web_teacher2.mp4
web_teacher1.mp4

- OnyxUpdateManager.js
web_onyxManager1

web_onyxManager.mp4

- DemoActions.js
web_money2020

- Card.js

web_card.mp4

- CanvasSize.js

web_Canvas.mp4

- MemoryOnlyKeys folder
web_memory_keys

MacOS: Desktop

- TransactionEdit.js

desktop_transactionEdit.mp4

- TeachersUnite.js
desktop_teacher3

desktop_teacher2.mp4
desktop_teacher1.mp4

- OnyxUpdateManager.js
desktop_onyxManager2

desktop_onyxManager1.mp4

- Card.js

desktop_card.mp4

- CanvasSize.js

desktop_canvas.mp4

- MemoryOnlyKeys folder

desktop_memoryKeys

src/libs/actions/Card.ts Outdated Show resolved Hide resolved
src/libs/actions/OnyxUpdateManager.ts Outdated Show resolved Hide resolved
src/types/modules/canvas-size.d.ts Outdated Show resolved Hide resolved
src/types/onyx/PersonalDetails.ts Outdated Show resolved Hide resolved
src/libs/actions/DemoActions.ts Outdated Show resolved Hide resolved
src/libs/actions/OnyxUpdateManager.ts Outdated Show resolved Hide resolved
src/libs/actions/OnyxUpdateManager.ts Outdated Show resolved Hide resolved
const loggedInEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(sessionEmail);
const reportCreationData: ReportCreationData = {};

const expenseChatData = ReportUtils.buildOptimisticChatReport([sessionAccountID], '', CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT, policyID, sessionAccountID, true, policyName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you can add type to this const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubabutkiewicz it's typed automatically, do you think it makes sense to type it additionally?
image


const expenseChatData = ReportUtils.buildOptimisticChatReport([sessionAccountID], '', CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT, policyID, sessionAccountID, true, policyName);
const expenseChatReportID = expenseChatData.reportID;
const expenseReportCreatedAction = ReportUtils.buildOptimisticCreatedReportAction(sessionEmail);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubabutkiewicz it's typed automatically, see my comment

# Conflicts:
#	src/types/onyx/ReportAction.ts
src/libs/actions/Card.ts Outdated Show resolved Hide resolved
src/libs/actions/Card.ts Outdated Show resolved Hide resolved
src/libs/actions/Card.ts Outdated Show resolved Hide resolved
src/libs/actions/TeachersUnite.ts Outdated Show resolved Hide resolved
@VickyStash
Copy link
Contributor Author

@BartoszGrajdek Thank you, updated 🙂

@VickyStash
Copy link
Contributor Author

@BartoszGrajdek kind bump 🙂

src/types/modules/window.d.ts Outdated Show resolved Hide resolved
@VickyStash
Copy link
Contributor Author

@grgia kind bump 🙂

grgia
grgia previously approved these changes Jan 5, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we push these in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because @types/canvas-size was added in this PR 😄

@cubuspl42
Copy link
Contributor

What's the "Updates from main branch" commit? I don't want to be picky, but messed-up merges have been a source of regressions in the past. Shouldn't this be just a single merge commit?

@VickyStash
Copy link
Contributor Author

@cubuspl42 The original JS file, which had updates in the main branch, was removed in favor of TS one during migration. So another commit applies updates from the JS file to the TS file so as not to miss updated functionality

# Conflicts:
#	src/libs/PolicyUtils.ts
#	src/types/onyx/Policy.ts
@VickyStash
Copy link
Contributor Author

@grgia kind bump 🙂

@cubuspl42
Copy link
Contributor

@cubuspl42 The original JS file, which had updates in the main branch, was removed in favor of TS one during migration. So another commit applies updates from the JS file to the TS file so as not to miss updated functionality

Oh, I get it. Great that you caught those changes.

@grgia grgia merged commit e372de1 into Expensify:main Jan 9, 2024
15 of 17 checks passed
@grgia
Copy link
Contributor

grgia commented Jan 9, 2024

Merging with license tests failing which is expected with package-lock changes: https://github.com/Expensify/App/pull/32164/files/4de7a5ed23c231251f1bee824c46bac35892efa1#r1442385933

@OSBotify
Copy link
Contributor

OSBotify commented Jan 9, 2024

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/grgia in version: 1.4.24-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@kavimuru
Copy link

@VickyStash We need some help in validating this PR:

  1. Should we test TeachersUnite section since it deals with real users?
  2. Need help to test the following section.

Besides that, you can use Onyx.set('onyxUpdatesFromServer', {INVALID_FORMAT_FROM_LIST_BELOW}); in the console to set that property to an invalid format and confirm it will log [OnyxUpdateManager] Invalid format found for updates, cleaning and unpausing the queue
you can test the following wrong formats:

Onyx.set('onyxUpdatesFromServer', {'type': ''});
Onyx.set('onyxUpdatesFromServer', {'type': 'https'});
Onyx.set('onyxUpdatesFromServer', {'type': 'https', 'request': {}});
Onyx.set('onyxUpdatesFromServer', {'type': 'pusher', 'request': {}, 'response': {}});

  1. MemoryOnlyKeys:
    How do we verify the logs in this section?

@VickyStash
Copy link
Contributor Author

@kavimuru Small note: in screenshots/recordings section, you can find material for every feature per platform (if it's possible to test it on the platform) split by feature name. It looks like
- TransactionEdit.js
material for it
- TeachersUnite.js
material for it

  1. I think so since this PR was implemented a couple of days ago
  2. I think this screenshot should help with it.
    You can pass to the console onyxUpdatesFromServer event with invalid format data, and you should see the error

image

Event which you can try to pass to the console:

Onyx.set('onyxUpdatesFromServer', {'type': ''});
Onyx.set('onyxUpdatesFromServer', {'type': 'https'});
Onyx.set('onyxUpdatesFromServer', {'type': 'https', 'request': {}});
Onyx.set('onyxUpdatesFromServer', {'type': 'pusher', 'request': {}, 'response': {}});
  1. The logs should be displayed in the console
    image

Please, let me know if something is still not clear, I'll try to help!

@kavimuru
Copy link

@VickyStash Thanks for the explanation. We will try with your instructions. But still I want to make sure if we can use dummy data for "teachers unite" section.

@VickyStash
Copy link
Contributor Author

@kavimuru I think you can use dummy data on non production environments, since after this PR was merged it shouldn't affect real users.
See how TU chat looks for me right now on dev, a lot of test messages
image
Since @marcochavezf is the author of the PR I mention, maybe he can also confirm

@kavimuru
Copy link

@VickyStash Thanks for confirming.

@thienlnam
Copy link
Contributor

thienlnam commented Jan 11, 2024

This appears to be causing type errors on main

Error: src/libs/actions/TeachersUnite.ts(93,9): error TS2322: Type '{ onyxMethod: "set"; key: `policy_${string}`; value: { id: string; isPolicyExpenseChatEnabled: true; type: "corporate"; name: "Expensify.org / Teachers Unite!"; role: "user"; owner: string; outputCurrency: string; pendingAction: "add"; }; }' is not assignable to type 'OnyxUpdate'.
  Types of property 'value' are incompatible.
    Property 'areChatRoomsEnabled' is missing in type '{ id: string; isPolicyExpenseChatEnabled: true; type: "corporate"; name: "Expensify.org / Teachers Unite!"; role: "user"; owner: string; outputCurrency: string; pendingAction: "add"; }' but required in type 'Policy'.
Error: Process completed with exit code 2.

EDIT: This may actually be because of a change I am reverting nvm

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.24-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.