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

Add global enabled option so we can toggle it on and off for different tests #31

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TylerRick
Copy link
Contributor

@TylerRick TylerRick commented Apr 25, 2020

Resolves #30

(Also resolves/supersedes #27)

Currently based on #29 but could be rebased.

  • Add tests
  • Update docs

@TylerRick TylerRick force-pushed the add_enabled_option branch 3 times, most recently from cf0aab8 to e445c2c Compare April 27, 2020 16:40
@TylerRick
Copy link
Contributor Author

It would be nice if we could simply call it Devise::PwnedPassword.enabled, but using top-level Devise config, Devise.pwned_password_check_enabled, seemed to be the most consistent with what this gem is already doing.

@TylerRick TylerRick changed the title WIP: Add global enabled option so we can toggle it on and off for different tests Add global enabled option so we can toggle it on and off for different tests Apr 27, 2020
@ndbroadbent
Copy link

ndbroadbent commented May 8, 2020

Amazing, I was just looking for this!

I have some specific feature specs and VCR recordings that test that devise-pwned_password is working properly, but I only want to selectively enable it for these tests. The with_pwned_password_check test helper is also super awesome, so thanks for including that!

I will use your fork for now and try it out.

UPDATE: This works perfectly! Thanks for your work!

@ndbroadbent
Copy link

@michaelbanfield Sorry to bother you! But I'm just going through my Gemfile and looking at different forks I'm using, and I'm still using @TylerRick's branch for this PR. I was wondering if it might be possible to merge this and release a new version of the gem?

If you are no longer maintaining the gem, then I'd be happy to take over maintenance and releasing new versions, since my company is using it (https://github.com/docspring).

@ndbroadbent
Copy link

Update: We're still using this fork and would be keen to see support for this in the official release. But no worries if not!

@erikbrannstrom
Copy link

We also have cases where we are currently doing some less-than-elegant workarounds. For example, when seeding data, we skip all validations when creating users to avoid errors (since seed passwords tend to be pretty basic), and in our test suite we are using webmock to stub requests to the API, since we can't disable it completely due to some tests that verify the password validation process.

Let me know if there's anything I can do to help get this improvement released! We really appreciate your work on this gem @michaelbanfield! 🙏

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.

Add global enabled option so we can toggle it on and off for different tests
3 participants