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

Aum/wall 228/crypto cashier changes for third party processor coinspaid #8343

Conversation

aum-deriv
Copy link
Contributor

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

  • Bug fix
  • New feature
  • Update feature
  • Refactor code
  • Translation to code
  • Translation to crowdin
  • Script configuration
  • Improve performance
  • Style only
  • Dependency update
  • Documentation update
  • Release

@vercel
Copy link

vercel bot commented Apr 25, 2023

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

Name Status Preview Comments Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2023 9:04am

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

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/8343](https://github.com/binary-com/deriv-app/pull/8343)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-aum-deriv-aum-wall-228crypto-cashier-8fd026.binary.sx?qa_server=red.binaryws.com&app_id=24053
    - **Original**: https://deriv-app-git-fork-aum-deriv-aum-wall-228crypto-cashier-8fd026.binary.sx
- **App ID**: `24053`

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 21
🟧 Accessibility 75
🟢 Best practices 92
🟧 SEO 85
🟢 PWA 90

Lighthouse ran with https://deriv-app-git-fork-aum-deriv-aum-wall-228crypto-cashier-8fd026.binary.sx/

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2023

Codecov Report

Merging #8343 (4dbc748) into master (e7df8d2) will increase coverage by 0.02%.
The diff coverage is 80.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #8343      +/-   ##
==========================================
+ Coverage   20.81%   20.83%   +0.02%     
==========================================
  Files        1561     1562       +1     
  Lines       36118    36129      +11     
  Branches     7091     7094       +3     
==========================================
+ Hits         7518     7528      +10     
- Misses      27858    27860       +2     
+ Partials      742      741       -1     
Impacted Files Coverage Δ
...ansactions-history/crypto-transactions-history.tsx 72.00% <25.00%> (-5.28%) ⬇️
...shier/src/components/alert-banner/alert-banner.tsx 100.00% <100.00%> (ø)
...nsactions-history/crypto-transactions-renderer.tsx 85.36% <100.00%> (+3.78%) ⬆️
...rc/pages/deposit/crypto-deposit/crypto-deposit.tsx 91.78% <100.00%> (+0.35%) ⬆️

Copy link
Contributor

@farzin-deriv farzin-deriv left a comment

Choose a reason for hiding this comment

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

Please update your branch with the latest master 🙏🏻

heorhi-deriv
heorhi-deriv previously approved these changes May 3, 2023
import React from 'react';
import classNames from 'classnames';
import { Icon, Text } from '@deriv/components';
import { Localize } from '@deriv/translations';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { Localize } from '@deriv/translations';

type TAlertBanner = {
className?: string;
icon: string;
message: JSX.Element | string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message: JSX.Element | string;
message: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heorhi-deriv suggested me to use <Localize /> component instead of the function as he pointed out that the function variant is used only for simple strings and not for dynamic data throughout the project.

);

expect(crypto_transactions_history_table_tooltip_mobile).toBeInTheDocument();
fireEvent.click(crypto_transactions_history_table_tooltip_mobile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fireEvent.click(crypto_transactions_history_table_tooltip_mobile);
userEvent.click(crypto_transactions_history_table_tooltip_mobile);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@henry-deriv can you please provide a reason for using userEvent instead of fireEvent?

Copy link
Contributor

Choose a reason for hiding this comment

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

userEvent reflects user interaction better than fireEvent. Mimics a more realistic user interaction than fireEvent. Also, by convention we have stopped using fireEvent in our repo.

@@ -1,5 +1,6 @@
import React from 'react';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { render, screen, waitFor } from '@testing-library/react';

…L-228/crypto-cashier-changes-for-third-party-processor-coinspaid
…L-228/crypto-cashier-changes-for-third-party-processor-coinspaid
…anner.spec.tsx

Co-authored-by: Farzin Mirzaie <72082844+farzin-deriv@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.0% 2.0% Duplication

@sandeep-deriv sandeep-deriv merged commit 28ad069 into binary-com:master May 10, 2023
nijil-deriv pushed a commit to nijil-deriv/deriv-app that referenced this pull request May 24, 2023
…id (binary-com#8343)

* feat: Tooltip for crypto transactions processed by third-party payement processors

* feat: Added popup note for mobile view for third-party payment tooltip

* feat: added alert banner component in cashier package

* feat: added IcAlertWarningDark icon

* chore: working on test cases 1

* chore: reseting testcase to upstream/master

* chore: added test case for tooltip in desktop mode

* feat: AlertBanner for cryptoc with minimum deposit using third-party processor (CoinsPaid)

* feat: changed styles for crypto-deposit.tsx

* chore: added data-testid for tooltip in mobile view

* chore: added test cases for crypto-deposit.tsx

* chore: added test cases for crypto-deposit.tsx

* fix: fixed the test cases for crypto-deposit.tsx

* chore: made changes based on comments in PR

* chore: fixing changes suggested by @heorhi-deriv

* chore: updated alert-banner.tsx

* refactor: changed the way localization is handled by alert-banner.tsx

* chore: minor fixes

* chore: removed unused classnames

* chore: add test cases and made changes suggested by @nijil-deriv

* chore: made changes for the comments in PR

* Update packages/cashier/src/components/alert-banner/__tests__/alert-banner.spec.tsx

Co-authored-by: Farzin Mirzaie <72082844+farzin-deriv@users.noreply.github.com>

---------

Co-authored-by: Farzin Mirzaie <72082844+farzin-deriv@users.noreply.github.com>
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