Not setting invitation_sent_at breaks skip_invitation invites #301

Closed
hlascelles opened this Issue Apr 3, 2013 · 12 comments

Projects

None yet

3 participants

@hlascelles

Between 1.1.6 and 1.1.7,

this line:

self.invitation_sent_at = Time.now.utc

became

self.invitation_sent_at = Time.now.utc unless @skip_invitation

It means that if you use the skip_invitation feature (to not send a mail, since you want to send it yourself in a different way), then the user can never accept the invite.

This is because invitation_period_valid? always returns false (actually nil), since the first part of it is always nil:

invitation_sent_at && (self.class.invite_for.to_i.zero? || invitation_sent_at.utc >= self.class.invite_for.ago)

One symptom of this bug, incidentally, is seeing "Invalid invitation token" when choosing a password, which is the most confusing part.

The resolution is to just revert invitation_sent_at to always be set, even if @skip_invitation is true. I hesitate to send a pull request as I'm not sure what the best resolution would be.

Thanks!

@seanabrahams

Hit by the same bug.

@scambra
Owner

If you send the email you must set invitation_sent_at, devise_invitable cannot ensure email is sent so invitation_sent_at cannot be set

@scambra scambra closed this Apr 5, 2013
@hlascelles

OK... Maybe worth putting something in the docs? Especially since the symptom is misleading: "Invalid invitation token" - and it was a breaking change for a minor point release.

I'll submit a pull request.

I suppose the ultimate solution would be handling the difference between a "created" invite, (which matters), and a record of when it was sent (which doesn't, really)...

@hlascelles

Done: #303

@seanabrahams

This is a tricky one because the invitation_token shouldn't necessarily be invalid just because invitation_sent_at is nil. What if you have an invite system that doesn't send emails and instead you cut and paste an invite link to someone in a chatroom or via IM?

Why does devise_invitable rely on an invitation email being sent? For 'invite_for' shouldn't it look at when the invitation was created, and not when an invite email is sent, since people may not be sending invite emails?

@scambra
Owner

@seanabrahams is a good point, what do you think about adding invitation_created_at and using that in invite_for? or using "invitation_sent_at || invitation_created_at"?

@seanabrahams

I think invitation_created_at is definitely better for invite_for. It's what I would expect.

invitation_sent_at || invitation_created_at is tricky because if an invite email is sent out 2 days later you've just extended the invite_for period for 2 days. 99.9% of the time this probably won't matter but it's an edge case to be aware of.

My vote is to ditch invitation_sent_at in favor of invitation_created_at. What do you think @hlascelles?

@hlascelles

I think @seanabrahams has it... When invitation_created_at is the important thing, and it would be what I'd expect coming to the gem afresh.

This does mean a DB migration for current applications to add the new column, I suppose, but it is the better strategic solution. v1.2.0?

@scambra
Owner

Ok, I agree, I will add it to 1.2.0

Devise will warn existing applications of new field, because it will be added to required_fields

@scambra scambra reopened this Apr 8, 2013
@seanabrahams
@scambra scambra closed this in aaa3624 Aug 14, 2013
@scambra scambra added a commit that referenced this issue Aug 14, 2013
@scambra add invitation_created_at, closes #301
Conflicts:
	lib/devise_invitable/model.rb
e46eb32
@hlascelles

New version works great for us... Thank you!

@scambra
Owner

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment