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

Add template methods #26

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

Add template methods #26

wants to merge 2 commits into from

Conversation

flori
Copy link

@flori flori commented Mar 17, 2020

No description provided.

@flori
Test on more recent rubies
6bc9679
@flori
Add template methods for logging etc.
a13981d 
This patch adds one template method `pwned_after_password_attempt` that
is called after every attempt to sign in allowing to evaluate the
results and log them. It also adds a method `pwned_after_error` that is
called after a Pwned::Error (including Pwned::TimeoutError) has occured
to be notified if there are problems related to the HaveIBeenPwned API.
@TylerRick
Copy link
Contributor

This looks good. Just need to figure out why CI is failing for 2.6:
https://github.com/michaelbanfield/devise-pwned_password/pull/26/checks?check_run_id=513421333

@TylerRick
Copy link
Contributor

Also, may want to call them "callback methods" rather than "template methods"...

@TylerRick TylerRick mentioned this pull request Apr 27, 2020
Open
@TylerRick
Copy link
Contributor

I pushed the exact some code up as #34 and it passed CI, which seems to confirm that it's just an intermittent failure. Looks like that also caused the status of this to be updated since they were both associated with the same commit ref (26 and 34).

@fwolfst
Copy link

fwolfst commented Aug 2, 2022 •
edited
Loading

Also, may want to call them "callback methods" rather than "template methods"...

There is no reference to "template" in the code. We currently run a fork of devise-pwned_password with pretty much this change in it. @TylerRick @michaelbanfield Is there any chance that we get it merged?

  • What I see missing is some mention in the README (although not critical since its a niche feature), I can do that.
  • Also, maybe somebody prefers after_pwned_error instead of pwned_after... (and same for ...attempt...).

@fwolfst
Copy link

fwolfst commented Sep 16, 2022

Sorry for being pingy, but I'd really like to get this upsteam (and close our fork). Anything I can do to get it back alive?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@flori @TylerRick @fwolfst