[TS migration] Migrate 'Policy.js' lib to TypeScript #33691

cdOut · 2023-12-28T06:55:08Z

Details

Migration of Policy.js to TypeScript.

Fixed Issues

Tests

Policy.js that is being migrated here is responsible for everything related to workspaces, so please click through all flows related with it as a test check.

Log in and create a new workspace.
Go into the workspace, click through and test every flow that is related to the workspace.
In settings change the name, unit and rate, you can also test updating the avatar and then clearing it.
Invite new members with a specific message, check that it sent the correct message to user.
Try to connect a bank account, make sure it works properly.
After trying every flow related to the workspace, remove it and confirm that it was deleted successfully.

Verify that no errors appear in the JS console

Offline tests

QA Steps

Policy.js that is being migrated here is responsible for everything related to workspaces, so please click through all flows related with it as a test check.

Log in and create a new workspace.
Go into the workspace, click through and test every flow that is related to the workspace.
In settings change the name, unit and rate, you can also test updating the avatar and then clearing it.
Invite new members with a specific message, check that it sent the correct message to user.
Try to connect a bank account, make sure it works properly.
After trying every flow related to the workspace, remove it and confirm that it was deleted successfully.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

ANDROID-NATIVE.mov

Android: mWeb Chrome

ANDROID-CHROME.mov

iOS: Native

IOS-NATIVE.mov

iOS: mWeb Safari

IOS-SAFARI.mov

MacOS: Chrome / Safari

CHROME.mov

MacOS: Desktop

DESKTOP.mov

…ction-lib

assets/emojis/common.ts

src/libs/PersonalDetailsUtils.js

src/libs/actions/Policy.ts

src/libs/PersonalDetailsUtils.ts

src/types/onyx/Policy.ts

src/libs/actions/Policy.ts

blazejkustra · 2024-01-05T07:53:35Z

@cdOut Conflicts 😅 Let us know once it's ready for re-review

kubabutkiewicz

minor comments, rest looks good 👍

src/libs/PolicyUtils.ts

src/types/onyx/Policy.ts

melvin-bot · 2024-01-10T10:48:28Z

@alitoshmatov 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]

alitoshmatov · 2024-01-10T15:58:28Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible 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 checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (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
- 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 verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
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
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
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(theme.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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

policy-android.mov

Android: mWeb Chrome

policy-mweb.mov

iOS: Native

policy-ios.mp4

iOS: mWeb Safari

policy-safari.mp4

MacOS: Chrome / Safari

policy-web.mov

MacOS: Desktop

policy-desktop.mov

alitoshmatov

LGTM!

roryabraham

Try to re-review this PR yourself and check for a few things:

Use finallyData to DRY up any overlaps between successData and failureData
Make sure any types have descriptive names that capture what they actually are
question whether optional fields really should be optional

src/libs/PersonalDetailsUtils.ts

src/libs/actions/Policy.ts

roryabraham · 2024-01-10T20:06:52Z

src/types/onyx/Policy.ts

+    unit: Unit;
+};
+
+type NewCustomUnit = {


We should not need separate types for CustomUnit and NewCustomUnit.

Furthermore, It's pretty sus that all the fields of both would be optional.

I did check which fields should be required and updated it accordingly. I have a very limited leeway when handing types here due to how values are passed in the updateWorkspaceCustomUnitAndRate and saveUnitAndRate. I did manage to remove the other type, it still isn't perfect but that's the best we can achieve without changing the logic and value passed to the functions mentioned above. Please check whether current implementation is okay.

merge main into ts-migration/policy-action-lib

cdOut · 2024-01-11T15:49:23Z

cc: @roryabraham

I re-reviewed and applied your suggestions, please check it again.

src/libs/actions/Report.ts

src/types/onyx/Policy.ts

roryabraham · 2024-01-15T19:23:09Z

src/types/onyx/Policy.ts

+    makeMeAdmin?: boolean;
+
+    /** Pending fields for the policy */
+    pendingFields?: Record<string, unknown>;


We should instead use OnyxCommon.PendingFields, ideally including only the fields that can be pending, for example like pendingFields?: OnyxCommon.PendingFields<'avatar' | 'employeeList>

We should make this change here, but I also brought this topic up for further discussion here

In this case to get ts passing I had to change this to:

pendingFields?: OnyxCommon.PendingFields<'addWorkspaceRoom' & 'avatar' & 'generalSettings'>

@roryabraham 'addWorkspaceRoom' & 'avatar' & 'generalSettings' type equals never, so it's not ideal. I agree that we should define pendingFields better, moving to the slack discussion.

We still could have changed this to be just OnyxCommon.PendingFields, but I'll treat this as NAB since we have a follow-up issue in place

roryabraham · 2024-01-15T23:11:49Z

Also we have conflicts here

src/libs/actions/Policy.ts

blazejkustra · 2024-01-17T17:20:10Z

Back to you @roryabraham!

roryabraham

LGTM 👍🏼

roryabraham · 2024-01-17T21:04:51Z

src/types/onyx/Policy.ts

+    makeMeAdmin?: boolean;
+
+    /** Pending fields for the policy */
+    pendingFields?: Record<string, unknown>;


We still could have changed this to be just OnyxCommon.PendingFields, but I'll treat this as NAB since we have a follow-up issue in place

OSBotify · 2024-01-17T21:13:11Z

✋ 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 · 2024-01-18T03:25:24Z

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.27-0 🚀

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

OSBotify · 2024-01-19T09:45:10Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.27-1 🚀

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

BartoszGrajdek and others added 7 commits November 28, 2023 16:07

Rename Policy file

c037865

TS migration

ddaf7b9

Merge remote-tracking branch 'origin/main' into ts-migration/policy-a…

66fbaf9

…ction-lib

TS migration of Policy.js

a5ea5d1

Resolve merge conflicts

bc82922

Add changes to ReportUtils & merge main

ecb7dfe

add remaining typescript typing to the policy lib

a793486

blazejkustra reviewed Dec 28, 2023

View reviewed changes

assets/emojis/common.ts Outdated Show resolved Hide resolved

blazejkustra suggested changes Dec 28, 2023

View reviewed changes

cdOut and others added 10 commits January 2, 2024 03:45

Merge branch 'main' into ts-migration/policy-action-lib

ab6b95f

add review suggested changes

1097bc1

Fix typecheck and lint

fa05286

Merge branch 'main' into ts-migration/policy-action-lib

f9ec088

Final Policy adjustements

18a8891

Add missing return types and change Record to stricter types

55648a3

Merge branch 'main' into ts-migration/policy-action-lib

87610b2

Make isHarvestingEnabled optional

fa42c08

Merge branch 'main' into ts-migration/policy-action-lib

11c7a4c

Merge branch 'main' into ts-migration/policy-action-lib

46e4bba

blazejkustra approved these changes Jan 3, 2024

View reviewed changes

BartoszGrajdek reviewed Jan 3, 2024

View reviewed changes

src/libs/actions/Policy.ts Outdated Show resolved Hide resolved

src/libs/actions/Policy.ts Outdated Show resolved Hide resolved

src/libs/actions/Policy.ts Outdated Show resolved Hide resolved

src/libs/actions/Policy.ts Outdated Show resolved Hide resolved

VickyStash suggested changes Jan 3, 2024

View reviewed changes

cdOut added 3 commits January 4, 2024 15:59

add suggested review changes and refactor

4f1adec

Merge branch 'main' into ts-migration/policy-action-lib

d0e0cbb

fix correct type imports

bd95ad8

kubabutkiewicz reviewed Jan 8, 2024

View reviewed changes

src/libs/PolicyUtils.ts Outdated Show resolved Hide resolved

src/types/onyx/Policy.ts Outdated Show resolved Hide resolved

Merge branch 'main' into ts-migration/policy-action-lib

81d9208

roryabraham mentioned this pull request Jan 9, 2024

[HOLD for payment 2024-01-26] [TS migration] Migrate 'Policy.js' lib to TypeScript #24918

Closed

implement fix and review suggestions

b0a9ed9

melvin-bot bot requested a review from alitoshmatov January 10, 2024 10:48

alitoshmatov approved these changes Jan 10, 2024

View reviewed changes

melvin-bot bot requested a review from roryabraham January 10, 2024 15:58

roryabraham reviewed Jan 10, 2024

View reviewed changes

cdOut added 6 commits January 11, 2024 11:36

Merge branch 'main' into ts-migration/policy-action-lib

7e534e9

merge main into ts-migration/policy-action-lib

implement finallyData and fix merge errors

ef4c055

include required areChatRoomsEnabled in policy creation

5ba57d1

change properties from optional to required for CustomUnit

58ac053

remove NewCustomUnit type and refactor rest accordingly

ce0205d

refactor two api writes for finallyData

2d67506

roryabraham requested changes Jan 15, 2024

View reviewed changes

blazejkustra reviewed Jan 16, 2024

View reviewed changes

src/libs/actions/Policy.ts Outdated Show resolved Hide resolved

cdOut added 5 commits January 17, 2024 02:17

refactor return type of getNewPersonalDetailsOnyxData

cdc8486

fix CustomUnit rates typing

59556b2

remove usage of isNotEmptyObject for in PR related files

6c72ef8

Merge branch 'main' into ts-migration/policy-action-lib

b1e2aeb

fix double negation lint error

b9f4551

roryabraham approved these changes Jan 17, 2024

View reviewed changes

roryabraham merged commit 7dfbddc into Expensify:main Jan 17, 2024
16 checks passed

github-actions bot mentioned this pull request Jan 18, 2024

Deploy Checklist: New Expensify 2024-01-18 #34701

Closed

31 tasks

bernhardoj mentioned this pull request Jan 30, 2024

[$500] Connect BA modal is not displayed when trying to pay with Expensify in DM chat #35366

Closed

6 tasks

sobitneupane mentioned this pull request Apr 11, 2024

[Awaiting checklist completion] [$500] Workspace – Workspace chat name and avatar changes to the default after deleting WS #37759

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TS migration] Migrate 'Policy.js' lib to TypeScript #33691

[TS migration] Migrate 'Policy.js' lib to TypeScript #33691

cdOut commented Dec 28, 2023 •

edited

Loading

blazejkustra commented Jan 5, 2024

kubabutkiewicz left a comment

melvin-bot bot commented Jan 10, 2024

alitoshmatov commented Jan 10, 2024

alitoshmatov left a comment

roryabraham left a comment

roryabraham Jan 10, 2024

cdOut Jan 11, 2024

cdOut commented Jan 11, 2024

roryabraham Jan 15, 2024

roryabraham Jan 15, 2024

roryabraham Jan 15, 2024

blazejkustra Jan 16, 2024

roryabraham Jan 17, 2024

roryabraham commented Jan 15, 2024

blazejkustra commented Jan 17, 2024

roryabraham left a comment

roryabraham Jan 17, 2024

OSBotify commented Jan 17, 2024

OSBotify commented Jan 18, 2024

OSBotify commented Jan 19, 2024

[TS migration] Migrate 'Policy.js' lib to TypeScript #33691

[TS migration] Migrate 'Policy.js' lib to TypeScript #33691

Conversation

cdOut commented Dec 28, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

blazejkustra commented Jan 5, 2024

kubabutkiewicz left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Jan 10, 2024

alitoshmatov commented Jan 10, 2024

Reviewer Checklist

Screenshots/Videos

alitoshmatov left a comment

Choose a reason for hiding this comment

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdOut commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented Jan 15, 2024

blazejkustra commented Jan 17, 2024

roryabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jan 17, 2024

OSBotify commented Jan 18, 2024

OSBotify commented Jan 19, 2024

cdOut commented Dec 28, 2023 •

edited

Loading