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

Maryia/Fix: Hide URL query params before sending to 3rd parties #5666

Merged
merged 4 commits into from
Jun 16, 2022

Conversation

maryia-deriv
Copy link
Contributor

Changes:

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

  • Fix: Hide URL query params before sending to 3rd parties

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 Jun 8, 2022

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

Name Status Preview Updated
deriv-app ✅ Ready (Inspect) Visit Preview Jun 16, 2022 at 7:32AM (UTC)

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

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/5666](https://github.com/binary-com/deriv-app/pull/5666)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-maryia-binary-fixqueryparamhide.binary.sx?qa_server=frontend.binaryws.com&app_id=31496
    - **Original**: https://deriv-app-git-fork-maryia-binary-fixqueryparamhide.binary.sx
- **App ID**: `31496`

@codecov-commenter
Copy link

Codecov Report

Merging #5666 (16d04c0) into master (f3718f1) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 16d04c0 differs from pull request most recent head 06f4413. Consider uploading reports for the commit 06f4413 to get more accurate results

@@            Coverage Diff             @@
##           master    #5666      +/-   ##
==========================================
- Coverage   20.38%   20.37%   -0.01%     
==========================================
  Files        1290     1290              
  Lines       29290    29299       +9     
  Branches     4944     4945       +1     
==========================================
  Hits         5970     5970              
- Misses      23003    23012       +9     
  Partials      317      317              
Impacted Files Coverage Δ
...ages/core/src/App/Containers/Redirect/redirect.jsx 2.56% <0.00%> (-0.07%) ⬇️
packages/core/src/Stores/client-store.js 9.66% <0.00%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3718f1...06f4413. Read the comment docs.

@maryia-deriv maryia-deriv changed the title Maryia/Fix: Hide URL query params before sending to 3rd parties [WIP] Maryia/Fix: Hide URL query params before sending to 3rd parties Jun 9, 2022
setTimeout(() => {
// timeout is needed to get the token (code) from the URL before we hide it from the URL
// and from LiveChat that gets the URL from Window, particularly when initialized via HTML script on mobile
history.replaceState(null, null, window.location.search.replace(/&?code=[^&]*/i, ''));
Copy link
Contributor

Choose a reason for hiding this comment

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

since query params is seperated by &, the & before code is always existed, so why do we need to make it optional?

Suggested change
history.replaceState(null, null, window.location.search.replace(/&?code=[^&]*/i, ''));
history.replaceState(null, null, window.location.search.replace(/&code=[^&]*/i, ''));

Copy link
Contributor Author

@maryia-deriv maryia-deriv Jun 9, 2022

Choose a reason for hiding this comment

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

@mahdiyeh-fs because we need to include a case when the code query would be the first one on the list of query params, i.e. standing right after the '?' in the url 🙂

Copy link
Contributor

@matin-deriv matin-deriv left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2022

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
0.0% 0.0% Duplication

@carolsachdeva carolsachdeva merged commit 76dd373 into deriv-com:master Jun 16, 2022
vinu-deriv pushed a commit to vinu-deriv/deriv-app that referenced this pull request Jun 17, 2022
…istory state (deriv-com#5666)

Co-authored-by: Carol Sachdeva <58209918+carol-binary@users.noreply.github.com>
vinu-deriv pushed a commit to vinu-deriv/deriv-app that referenced this pull request Jun 21, 2022
…istory state (deriv-com#5666)

Co-authored-by: Carol Sachdeva <58209918+carol-binary@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants