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

When a port is specified for localhost, it should still be accepted as valid #214

Merged
merged 3 commits into from
Apr 26, 2018

Conversation

MariagraziaAlastra
Copy link
Member

@MariagraziaAlastra MariagraziaAlastra commented Apr 26, 2018

Reviewer: @dharb

CC: @andrey-p

Description:

Following up from #199, this also allows to whitelist the localhost when a port is specified (currently, strings like 'localhost:8080' are not considered valid).

Steps to test this PR:

  1. Build and reload extension
  2. Go to the options page
  3. Try whitelisting 'localhost', 'localhost:8080' (or use any port) - both should be valid
  4. Things like 'localhost:asdlajshdkja' should still be invalid
  5. Try whitelisting other domains: they should get validated as usual
  6. After whitelisting a domain, navigate to it and make sure it's actually whitelisted
  7. Play around with adding and removing domains using the toggle and the options page to make sure functionality is intact

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

Copy link
Contributor

@dharb dharb left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this, looks great!

@dharb dharb merged commit f7d78e6 into develop Apr 26, 2018
@dharb dharb deleted the maria/ports branch April 26, 2018 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants