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

allow only one confirmation #1001

Merged

Conversation

MaicolBen
Copy link
Collaborator

Reopened #519 because it has conflicts and the original author can't rebase it

Thanks @rmvenancio !

Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thank you for bringing this to a mergeable PR.

@@ -10,6 +10,10 @@ def show
token_hash = BCrypt::Password.create(token)
expiry = (Time.now + @resource.token_lifespan).to_i

if @resource.sign_in_count >0
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a space after the operator?

@MaicolBen MaicolBen force-pushed the allow_one_confirmation branch from 8b9dfcd to 89fccba Compare October 30, 2017 13:48
@MaicolBen MaicolBen force-pushed the allow_one_confirmation branch from 89fccba to 929e889 Compare October 30, 2017 13:54
@zachfeldman zachfeldman merged commit dc9456d into lynndylanhurley:master Oct 30, 2017
@hansy
Copy link

hansy commented Dec 15, 2017

This change means sign_in_count is a required attribute for the confirmable module? Doesn't Devise add errors to the instance if it's already been confirmed? If so, wouldn't it make more sense for the confirmations controller to check for errors on the resource before proceeding to generate headers and/or sign it in?

@zachfeldman
Copy link
Contributor

@hansy if this is concerning for you please file a new issue since this was already merged! Thanks

@MaicolBen
Copy link
Collaborator Author

@hansy It was fixed in #1064


test 'user already confirmed' do
assert @resource.sign_in_count > 0 do
assert expiry == (Time.now + Time.now + 1.second).to_i
Copy link
Collaborator

Choose a reason for hiding this comment

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

The #assert method from minitest does not yield the block that is given here!
(See RDoc for Minitest::Assertions#assert)

To further demonstrate this, even with an exception raised from within the block, the test still passes!

test 'user already confirmed' do
  assert @resource.sign_in_count > 0 do
    assert expiry == (Time.now + Time.now + 1.second).to_i
    raise "Chunky Bacon!"
  end
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

n/m... I commented here waaay late, and this has been resolved by #1113.

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.

5 participants