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

[P2PS] / Ameerul / P2PS-2375 Remove P2P API from DTrader page #13488

Conversation

ameerul-deriv
Copy link
Contributor

Changes:

Please provide a summary of the change.

  • Removed any p2p calls outside of p2p, only call them in p2p
  • Added logic for is_p2p_user tracking p2p_status from get_account_status
  • Prevent subscribing if the user is not a p2p user for p2p_advertiser_info and p2p_order_list inside of p2p
  • Moved useP2PCompletedOrderNotifications into app in p2p
  • Added new is_p2p_user return value in useGetAccountStatus hook

Screenshots:

Please provide some screenshots of the change.

remove.p2p.calls.outside.of.p2p.mov

Copy link

vercel bot commented Feb 9, 2024

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

Name Status Preview Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview Feb 19, 2024 9:28am

!general_store.poi_status &&
!general_store.should_show_dp2p_blocked
) {
if (general_store.is_p2p_user === null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we no longer call the endpoint, the previous checks will forever load the page, needed to check if get_account_status returns p2p_status instead. Resolves loading issue

@@ -44,6 +44,7 @@ export default class GeneralStore extends BaseStore {
is_listed = false;
is_loading = false;
is_p2p_blocked_for_pa = false;
is_p2p_user = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to add another observable for this, cause we can't use hooks inside the store 😢 but for v2 all of this will be gone 😄

},
[this.setP2pOrderList]
[this.updateAdvertiserInfo, response => sendbird_store.handleP2pAdvertiserInfo(response)],
this.is_p2p_user
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only calls if the user has 'active' or 'temp_ban' statuses, else no need to call to reduce load for BE

Comment on lines -524 to -531
order_list_subscription: subscribeWS(
{
p2p_order_list: 1,
subscribe: 1,
offset: 0,
limit: this.list_item_limit,
},
[this.setP2pOrderList]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no longer need this since the useP2PCompletedOrderNotifications hook is enough

…m:niloofar-deriv/deriv-app into P2PS-2375-remove-p2p-api-calls-from-dtrader-page
Copy link
Contributor

github-actions bot commented Feb 9, 2024

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

Click here to copy & paste above information.
- **PR**: [https://github.com/binary-com/deriv-app/pull/13488](https://github.com/binary-com/deriv-app/pull/13488)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-ameerul-deriv-p2ps-2375-remove-p2p-ap-7ed306.binary.sx?qa_server=red.derivws.com&app_id=32933
    - **Original**: https://deriv-app-git-fork-ameerul-deriv-p2ps-2375-remove-p2p-ap-7ed306.binary.sx
- **App ID**: `32933`

Copy link
Contributor

github-actions bot commented Feb 9, 2024

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 28
🟧 Accessibility 89
🟢 Best practices 92
🟧 SEO 85
🟧 PWA 78

Lighthouse ran with https://deriv-app-git-fork-ameerul-deriv-p2ps-2375-remove-p2p-ap-7ed306.binary.sx/

@@ -15,7 +15,7 @@
"@deriv/api": "^1.0.0",
"@deriv/library": "^1.0.0",
"@deriv/quill-design": "^1.3.2",
"@deriv/quill-icons": "^1.17.4",
"@deriv/quill-icons": "^1.17.25",
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we updating the version 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.

the PR was failing because of it :( so i had to update it here

Comment on lines +52 to +57
React.useEffect(() => {
if (error?.code === api_error_codes.PERMISSION_DENIED) {
general_store.setIsBlocked(true);
}
}, [error?.code]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously we checked p2p_advertiser_info response for Permission Denied error, but now we don't call it if the user is not a p2p user, so need this check

Comment on lines +479 to +487
if (status.includes('cashier_locked')) {
this.setIsBlocked(true);
this.hideModal();
} else {
this.setP2pPoaRequired(p2p_poa_required);
this.setPoaStatus(document.status);
this.setPoiStatus(identity.status);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we don't call p2p_advertiser_info if the user is not a p2p_user, better to move it in onMount, which also fixes issue where p2p_poa_required is not set

farrah-deriv
farrah-deriv previously approved these changes Feb 15, 2024
Copy link

sonarcloud bot commented Feb 19, 2024

Quality Gate Passed Quality Gate passed

Issues
6 New issues

Measures
0 Security Hotspots
No data about Coverage
6.2% Duplication on New Code

See analysis details on SonarCloud

@farrah-deriv farrah-deriv merged commit f7261b1 into deriv-com:master Feb 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants