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.

Sign up for GitHub

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

Jump to bottom

fix: handle trailing slashes in block explorer URLs #8592

Merged
merged 1 commit into from
May 14, 2020
Merged

fix: handle trailing slashes in block explorer URLs #8592

merged 1 commit into from
May 14, 2020
+86 −2

Conversation

bguiz
Copy link
Contributor

@bguiz bguiz commented May 14, 2020

What

  • modify ui/app/helpers/utils/transactions.util.js and
    ui/lib/account-link.js to strip trailing slashes
    if they are present.
  • added relevant tests not just for the new scenario,
    but also the general scenarios for these functions,
    as there previously was no test coverage for these
    two functions.

Why

  • Current behaviour, when user enters a block explorer URL
    when configuring a custom RPC, and that block explorer URL
    contains a trailing /.
    • e.g. https://block.explorer/
    • this results in a double-slash (//) in the transaction
      and account URLs generated by MetaMask.
    • e.g. https://block.explorer/tx/0xabcd...,
      https://block.explorer/account/0xabcd...
    • This needs to be handled using a router redirect
      on the server of the block explorer,
      and this changes would avoid that requirement.

@bguiz bguiz requested a review from a team as a code owner May 14, 2020 02:19
@bguiz bguiz force-pushed the fix/handle-trailing-slash-in-block-explorer-url branch from 041170d to ecce617 Compare May 14, 2020 02:42
rekmarks
rekmarks requested changes May 14, 2020
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Neat work, thanks!

Please resolve the lint failures and push again. yarn lint:fix or yarn lint:changed:fix (quicker, if you do a soft reset), should do the trick.

@bguiz bguiz force-pushed the fix/handle-trailing-slash-in-block-explorer-url branch from ecce617 to d19b46b Compare May 14, 2020 08:22
@bguiz
Copy link
Contributor Author

bguiz commented May 14, 2020

Neat work, thanks!

Please resolve the lint failures and push again. yarn lint:fix or yarn lint:changed:fix (quicker, if you do a soft reset), should do the trick.

@rekmarks done, looks like the various CI lint tasks have all passed now. Earlier on ci/circleci: test-e2e-firefox appears to have failed as well, but a forced-push without any changes appears to have resolved it, so perhaps that test is fragile/ non-deterministic.

https://circleci.com/gh/MetaMask/metamask-extension/164993

[2020-05-14 02:31:56.620][e2e]   1) MetaMask
[2020-05-14 02:31:56.620][e2e]        Going through the first time flow
[2020-05-14 02:31:56.620][e2e]          clicks the continue button on the welcome screen:
[2020-05-14 02:31:56.620][e2e]      ElementClickInterceptedError: Element <button class="button btn-primary first-time-flow__button"> is not clickable at point (154,509) because another element <div class="loading-overlay"> obscures it

@bguiz
fix: handle trailing / in block explorer URLs
96929d9 
What

- modify `ui/app/helpers/utils/transactions.util.js` and
  `ui/lib/account-link.js` to strip trailing slashes
  if they are present.
- added relevant tests not just for the new scenario,
  but also the general scenarios for these functions,
  as there previously was no test coverage for these
  two functions.

Why

- Current behaviour, when user enters a block explorer URL
  when configuring a custom RPC, and that block explorer URL
  contains a trailing `/`.
  - e.g. `https://block.explorer/`
  - this results in a double-slash (`//`) in the transaction
    and account URLs generated by MetaMask.
  - e.g. `https://block.explorer/tx/0xabcd...`,
    `https://block.explorer/account/0xabcd...`
  - This needs to be handled using a router redirect
    on the server of the block explorer,
    and this changes would avoid that requirement.
@bguiz bguiz force-pushed the fix/handle-trailing-slash-in-block-explorer-url branch from d19b46b to 96929d9 Compare May 14, 2020 09:15
@whymarrh
Copy link
Contributor

Yeah, our e2e tests aren’t especially reliable, currently 🙃

@bguiz
Copy link
Contributor Author

bguiz commented May 14, 2020

Yeah, our e2e tests aren’t especially reliable, currently

@whymarrh yeah, well one more failed test-e2e-Firefox, same error, but I manually triggered the build by doing another forced push. All checks are passing now!

rekmarks
rekmarks approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@bguiz, this is a lovely example of an external PR. I really appreciate the care you put into it.

@rekmarks rekmarks merged commit d0fcf66 into MetaMask:develop May 14, 2020
@bguiz
Copy link
Contributor Author

bguiz commented May 15, 2020

LGTM!

@bguiz, this is a lovely example of an external PR. I really appreciate the care you put into it.

You're welcome @rekmarks - and thanks for the comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rekmarks rekmarks rekmarks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bguiz @whymarrh @rekmarks