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

fix #150: remove ALL matching allowedHosts items #191

Closed

Conversation

groovecoder
Copy link
Member

@groovecoder groovecoder commented Oct 11, 2016

I couldn't actually reproduce the original state of #150. I.e., I think it was caused by a previous bug in the add-on that added multiple items of the same host into the allowedHosts array. That bug has since been closed, but those of us (like @pdehaan) who had the add-on running before it was closed likely got into a state where we had multiple items of the same host in our allowedHosts array.

To fix, I wrapped the allowedHosts.splice call in a do...while loop to clear out ALL matching items from the allowedHosts array.

To test:

  1. Run the add-on and disable protection for a few different sites.
  2. Go back to at least one of the sites where you disabled protection, and re-enable it.

Verify:

Refresh each of the sites and verify that their protection state is what you expect. I.e., the disabled sites are still disabled, and the re-enabled site(s) is enabled.

@groovecoder groovecoder force-pushed the re-enable-clear-any-allowsHosts-items-150 branch from 775c1d2 to 0ea7d7e Compare October 11, 2016 15:51
@groovecoder groovecoder force-pushed the re-enable-clear-any-allowsHosts-items-150 branch from 0ea7d7e to 06f7691 Compare October 11, 2016 15:53
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 45.94% when pulling 06f7691 on re-enable-clear-any-allowsHosts-items-150 into 5d74775 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 45.842% when pulling 06f7691 on re-enable-clear-any-allowsHosts-items-150 into 5d74775 on master.

@pdehaan
Copy link
Collaborator

pdehaan commented Oct 11, 2016

👍
LGTM. I don't think I've encountered the error again, since reporting this. So it's hard to verify locally.

@groovecoder
Copy link
Member Author

In that case, I think I'd actually like to close this PR without merging to avoid any potential knock-on effects of this change; but leave the branch on GitHub in case it comes up again.

@groovecoder groovecoder deleted the re-enable-clear-any-allowsHosts-items-150 branch December 12, 2016 16:10
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

3 participants