Farzin/76562/Remove legacy `connect` method from `cashier` #7238

farzin-deriv · 2022-12-27T07:17:20Z

Changes:

Please include a summary of the change and which issue is fixed below:

...

When you need to add unit test

If this change disrupt current flow
If this change is adding new flow

When you need to add integration test

If components from external libraries are being used to define the flow, e.g. @deriv/components
If it relies on a very specific set of props with no default behavior for the current component.

Test coverage checklist (for reviewer)

Ensure utility / function has a test case
Ensure all the tests are passing

Type of change

… components

vercel · 2022-12-27T07:17:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
deriv-app	✅ Ready (Inspect)	Visit Preview	Dec 30, 2022 at 3:43PM (UTC)

github-actions · 2022-12-27T07:28:29Z

A production App ID was automatically generated for this PR. (log)

PR: Farzin/76562/Remove legacy connect method from cashier #7238
URLs:
- w/ App ID + Server: https://deriv-app-git-fork-farzin-deriv-farzin-76562removelegacy-ebe5a6.binary.sx?qa_server=frontend.binaryws.com&app_id=31483
- Original: https://deriv-app-git-fork-farzin-deriv-farzin-76562removelegacy-ebe5a6.binary.sx
App ID: 31483

Click here to copy & paste above information.

- **PR**: [https://github.com/binary-com/deriv-app/pull/7238](https://github.com/binary-com/deriv-app/pull/7238)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-farzin-deriv-farzin-76562removelegacy-ebe5a6.binary.sx?qa_server=frontend.binaryws.com&app_id=31483
    - **Original**: https://deriv-app-git-fork-farzin-deriv-farzin-76562removelegacy-ebe5a6.binary.sx
- **App ID**: `31483`

…_from_cashier

codecov-commenter · 2022-12-27T07:40:00Z

Codecov Report

Merging #7238 (4449d6a) into develop (c743761) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #7238      +/-   ##
===========================================
+ Coverage    19.40%   19.50%   +0.09%     
===========================================
  Files         1478     1477       -1     
  Lines        34569    34587      +18     
  Branches      6366     6366              
===========================================
+ Hits          6709     6745      +36     
+ Misses       27349    27329      -20     
- Partials       511      513       +2

Impacted Files	Coverage Δ
packages/cashier/src/app.jsx	`0.00% <ø> (ø)`
...tainers/routes/error-component/error-component.tsx	`80.00% <ø> (ø)`
...er/src/containers/routes/route-with-sub-routes.tsx	`14.28% <ø> (ø)`
...ransfer-confirm/payment-agent-transfer-confirm.jsx	`100.00% <ø> (ø)`
...gent-transfer-form/payment-agent-transfer-form.jsx	`77.77% <ø> (ø)`
...ransfer-receipt/payment-agent-transfer-receipt.jsx	`100.00% <ø> (ø)`
.../payment-agent-transfer/payment-agent-transfer.jsx	`100.00% <ø> (ø)`
...ayment-agent-container/payment-agent-container.jsx	`86.11% <ø> (ø)`
...thdraw-form/payment-agent-listed-withdraw-form.jsx	`88.09% <ø> (ø)`
...nt/payment-agent-receipt/payment-agent-receipt.jsx	`96.00% <ø> (ø)`
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

farzin-deriv · 2022-12-27T08:46:16Z

packages/cashier/package.json

@@ -48,7 +48,6 @@
        "loadjs": "^4.2.0",
        "lodash.debounce": "^4.0.8",
        "mobx": "^6.6.1",
-        "mobx-react": "^7.5.1",


Should’ve been mobx-react-lite, But seems like we have missed it in merge conflicts and didn’t notice it.

farzin-deriv · 2022-12-27T08:47:50Z

packages/cashier/src/app.jsx

-                <Routes />
-            </StoreProvider>
-        </MobxContentProvider>
+        <StoreProvider store={root_store}>


Suggested change

<StoreProvider store={root_store}>

<StoreProvider>

Later when we could move all stores to the @deriv/stores we won’t need to pass anything to the provider anymore as the stores already live in the @deriv/stores package.

farzin-deriv · 2022-12-27T08:51:36Z

packages/cashier/src/containers/routes/routes.tsx

-// need to wrap withRouter around connect
-// to prevent updates on <BinaryRoutes /> from being blocked


Not sure if this comment is still valid or not, If it is then I guess we need to wrap BinaryRoutes with observer but I don’t see any use of stores data in BinaryRoutes 🤔

farzin-deriv · 2022-12-27T08:55:49Z

packages/cashier/src/types/stores/index.ts

@@ -1,6 +1,3 @@
 import { useStore } from '@deriv/stores';

 export type TRootStore = ReturnType<typeof useStore>;


We still need to keep TRootStore in the @deriv/cashier package as we are overriding the return type of useStore hook with cashier stores, Later once we managed to replace cashier stores with hooks or move them to @deriv/stores we should be able to remove this as well.

farzin-deriv · 2022-12-27T08:56:46Z

packages/stores/src/stores/CounterStore.ts

@@ -21,4 +22,8 @@ export default class CounterStore {
    decrement() {
        this.count = --this.count;
    }
+
+    unmount() {
+        stopPersisting(this);


We should call stopPersisting here to solve the warning we get while running tests.

heorhi-deriv · 2022-12-27T09:19:36Z

...ges/cashier/src/pages/on-ramp/on-ramp-provider-card/__tests__/on-ramp-provider-card.spec.tsx

    };

    it('should show proper messages and button', () => {
-        render(<OnRampProviderCard {...props} />);
+        const mockRootStore: DeepPartial<TRootStore> = {


@farzin-deriv could we refactor mockRootStore to avoid duplication? For example create mockRootStore in beforeEach function and in test cases just change the required field. (e.g is_dark_mode_on in 'should show proper icons in dark_mode' test case)?

heorhi-deriv · 2022-12-27T09:23:46Z

packages/cashier/src/pages/p2p-cashier/__tests__/p2p-cashier.spec.js

@@ -22,32 +17,148 @@ describe('<P2PCashier />', () => {
    const history = createBrowserHistory();

    it('should render <Loading /> component', () => {
+        const mockRootStore = {


@farzin-deriv could we refactor mockRootStore to avoid duplication? For example create mockRootStore in beforeEach function and in test cases just change the required field. (because e.g in 'should render P2P component' test case we only change is_logging_in field and in 'should redirect to "/cashier/p2p" page with "?order=1" query parameter' test case as i can see there is no any difference with original mockRootStore variable 🤔)?

@george-usynin-binary Yes you are right, We should have a mocked version of the RootStore so we won’t need to create it each time for each test case, I've already created a card for it and added my proposed API for it, We can discuss it and come up with better ideas 🙇🏻

…_from_cashier

sonarcloud · 2022-12-30T15:39:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

No Coverage information
66.0% Duplication

Farzin Mirzaie added 6 commits December 23, 2022 12:04

refactor(cashier): 🔥 remove mobx-react-lite from cashier

7babd01

refactor(cashier): ♻️ replace connect with useStore for remaining…

32b4c73

… components

refactor(cashier): ♻️ fix test files to use StoreProvider

73c5b25

refactor(cashier): 🔥 remove connect from cashier

d33218b

refactor(cashier): 🔥 remove extra store related types from cashier

df76fc2

fix(stores): 🐛 stop persisting the stores when store is unmount

02f8cfe

farzin-deriv changed the base branch from master to develop December 27, 2022 07:17

vercel bot deployed to Preview December 27, 2022 07:28 View deployment

Merge branch 'develop' into farzin/76562/remove_legacy_connect_method…

00a5fbd

…_from_cashier

vercel bot deployed to Preview December 27, 2022 07:47 View deployment

farzin-deriv commented Dec 27, 2022

View reviewed changes

farzin-deriv marked this pull request as ready for review December 27, 2022 08:57

farzin-deriv requested review from nijil-deriv, matin-deriv and balakrishna-deriv as code owners December 27, 2022 08:57

heorhi-deriv reviewed Dec 27, 2022

View reviewed changes

heorhi-deriv approved these changes Dec 27, 2022

View reviewed changes

Merge branch 'develop' into farzin/76562/remove_legacy_connect_method…

631cc10

…_from_cashier

vercel bot deployed to Preview December 28, 2022 08:59 View deployment

Merge branch 'develop' into farzin/76562/remove_legacy_connect_method…

745f900

…_from_cashier

vercel bot deployed to Preview December 29, 2022 01:49 View deployment

vinu-deriv approved these changes Dec 29, 2022

View reviewed changes

rostislav-deriv approved these changes Dec 29, 2022

View reviewed changes

Merge branch 'develop' into farzin/76562/remove_legacy_connect_method…

bc95ae0

…_from_cashier

vercel bot deployed to Preview December 30, 2022 07:19 View deployment

Merge branch 'develop' into farzin/76562/remove_legacy_connect_method…

0548085

…_from_cashier

vercel bot deployed to Preview December 30, 2022 07:44 View deployment

Merge branch 'develop' into farzin/76562/remove_legacy_connect_method…

4449d6a

…_from_cashier

vercel bot deployed to Preview December 30, 2022 15:43 View deployment

carolsachdeva merged commit ad1dc0b into binary-com:develop Jan 3, 2023

farzin-deriv deleted the farzin/76562/remove_legacy_connect_method_from_cashier branch January 3, 2023 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Farzin/76562/Remove legacy `connect` method from `cashier` #7238

Farzin/76562/Remove legacy `connect` method from `cashier` #7238

farzin-deriv commented Dec 27, 2022

vercel bot commented Dec 27, 2022 •

edited

Loading

github-actions bot commented Dec 27, 2022 •

edited

Loading

codecov-commenter commented Dec 27, 2022 •

edited

Loading

farzin-deriv Dec 27, 2022

farzin-deriv Dec 27, 2022

farzin-deriv Dec 27, 2022

farzin-deriv Dec 27, 2022

farzin-deriv Dec 27, 2022

heorhi-deriv Dec 27, 2022

heorhi-deriv Dec 27, 2022

farzin-deriv Dec 27, 2022 •

edited

Loading

sonarcloud bot commented Dec 30, 2022

		// need to wrap withRouter around connect
		// to prevent updates on <BinaryRoutes /> from being blocked

		@@ -1,6 +1,3 @@
		import { useStore } from '@deriv/stores';

		export type TRootStore = ReturnType<typeof useStore>;

Farzin/76562/Remove legacy connect method from cashier #7238

Farzin/76562/Remove legacy connect method from cashier #7238

Conversation

farzin-deriv commented Dec 27, 2022

Changes:

When you need to add unit test

When you need to add integration test

Test coverage checklist (for reviewer)

Type of change

vercel bot commented Dec 27, 2022 • edited Loading

github-actions bot commented Dec 27, 2022 • edited Loading

codecov-commenter commented Dec 27, 2022 • edited Loading

Codecov Report

farzin-deriv Dec 27, 2022

Choose a reason for hiding this comment

farzin-deriv Dec 27, 2022

Choose a reason for hiding this comment

farzin-deriv Dec 27, 2022

Choose a reason for hiding this comment

farzin-deriv Dec 27, 2022

Choose a reason for hiding this comment

farzin-deriv Dec 27, 2022

Choose a reason for hiding this comment

heorhi-deriv Dec 27, 2022

Choose a reason for hiding this comment

heorhi-deriv Dec 27, 2022

Choose a reason for hiding this comment

farzin-deriv Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Dec 30, 2022

Farzin/76562/Remove legacy `connect` method from `cashier` #7238

Farzin/76562/Remove legacy `connect` method from `cashier` #7238

vercel bot commented Dec 27, 2022 •

edited

Loading

github-actions bot commented Dec 27, 2022 •

edited

Loading

codecov-commenter commented Dec 27, 2022 •

edited

Loading

farzin-deriv Dec 27, 2022 •

edited

Loading