Adds verified_account event

[el-mapache]

Why: So users can have a record of when their account was verified

Screenshot:

[el-mapache]

I'm not certain what the best way to test that the create_account_verified_event in the ConfirmationsController` doesn't get called more than once. I have manually verified it, but trying to to call 'get :index' multiple times in the controller spec throws an pii encryption error.

[jessieay]

enums give us a handy shorthand so you can call user.events.account_verified and it will return all of the events for that enum type. Seems like that might be clearer than looking for the integer 8

[el-mapache]

Cool, I saw in the documentation that you can call ClassName.my_enum but didn't make the leap to trying it with an instance. Thanks!

[jessieay]

yes we should def write a test for this -- controller spec would be great.

[el-mapache]

Totally agree we should test it -- my comment was that I wasn't sure how to test it. Sorry if that was unclear!

I was under the impression that we don't test private methods directly?

When I try calling get :index twice in the controller spec, I get undefined method encrypted' for nil:NilClass from the encrypt_recovery_pii method in the profile model. Since I can refresh the page in browser and not get an error, I'm thinking the way I'm trying to test a repeated action is wrong.

[monfresh]

If I'm reading the code correctly, what you want to test is that if a user has already verified their account, the account_verified event won't be created a second time. So, it's not so much testing that the controller doesn't call the method more than once, but rather that when the method is called, it does the right thing.

What I would do is create a separate class that handles the logic of event creation, and test that class separately. In the controller spec, all you would need to test is that the proper class was called.

class CreateAccountVerifiedEvent
  def initialize(user)
    @user = user
  end

  def call
    create_event_if_user_not_verified
  end

  private
  
  attr_reader :user

  def create_event_if_user_not_verified
    return if user_verified?
    Event.create(user_id: user.id, event_type: :account_verified)
  end

  def user_verified?
    user.events.account_verified.present?
  end
end

In the controller:

CreateAccountVerifiedEvent.new(current_user).call

In the controller spec:

event_creator = instance_double(CreateAccountVerifiedEvent)
allow(CreateAccountVerifiedEvent).to receive(:new).with(subject.current_user).and_return(event_creator)
expect(event_creator).to receive(:call)

[el-mapache]

Stubbing a method on a discrete class seems clearer to me, I'll go with this approach

[el-mapache]

@monfresh is it generally just accepted ruby syntax to have methods named call? Is it done so that consumers just have a super generic API call that doesn't need to be updated if logic in the class changes?

[monfresh]

It doesn't have to be named call. We've been using call, perform, and submit, but maybe we should pick one and stick to it. Although for form submissions, I like submit. One reason to use call is because that's what Procs and Lambdas respond to, so it can come in handy in some situations.

Here's a sample discussion of how people name their service classes and methods:
https://forum.upcase.com/t/naming-service-classes/2930/7

You'll find many blog posts about these debates.

[monfresh]

The User Factory already creates a password. For this test, all that matters is that the Event is tied to a user. This should be sufficient:

let(:user) do
  create(:user) do |user|
    user.events.create(event_type: :account_verified)
  end
end

let(:eventless_user) { create(:user) }

[zachmargolis]

Hey I see I'm a little late to this, but I think it's a break in a bad way to use a separate service class to create the event.

I was able to discover all the user events we create by doing a grep create_user_event and this service class does not allow for that.

I would very much like to keep the single create_user_event interface for these (or at least have one interface, and make it easy to find).

Is the reason for the service class to encapsulate the logic to decide if there is already an event? What if we made a service class for checking? Ex AccountVerifiedService.new.account_already_created? or something?

[monfresh]

Having a create_user_event method defined in ApplicationController was a bad decision IMO. I'm in the process of removing all controller calls to create_user_event and moving them to the service classes/form objects that handle the POST requests. That is where they belong, not in the controller. Controllers should be as lean as possible and ideally only deal with redirects and flash messages. They shouldn't perform any additional tasks such as sending emails, saving users, creating events, etc.

[monfresh]

You can easily grep for event creations by grepping for Event.create, which is what create_user_event called.

[zachmargolis]

I think wanting to remove controller methods and logic like that is totally reasonable, but I really like having a single chokepoint in the meantime. I think it would be great to make sure we keep the entire codebase with one style (since there are a finite number of events, we can be extra consistent). Ex a follow-up PR could quickly remove the create_user_event controller helper and move to a service class

Re: grep Event.create --- that's not complete, since in this case we could do user.build_event() or even user.events << Event.new and not having a single "chokepoint" like create_user_event gives people too much choice IMO

[monfresh]

I'm fine with creating a service class for creating events, which would be similar to our UpdateUser class. I still think the CreateVerifiedAccountEvent class is valid. Instead of calling Event.create directly, it would call this new wrapper class that you are proposing.

[zachmargolis]

Yes, I'm fine with that (new service class, interface, whatever), my thing is that since there are a manageable number of events right now, I want them to all use the same exact way right now. So I dislike that this PR would 1 event with a new way, but there are 6 events already existing using 1 other way. I don't mind if we change them all, as long as they're all the same

[monfresh]

I don't see how this is "another" way. The event is being created with Event.create, which is the exact same way create_user_event is creating the event. You could say the same thing about how we call UserMailer.signup_with_your_email(email).deliver_later in two different places. We didn't create a helper method or class to make sure we send that email the same way because that's the standard way to send emails with ActiveJob. And we have tests to make sure that email is being sent when it should be sent, just like we have tests to make sure the proper events are created at the proper time. There's no reason I can think of where we would create an event other than by calling Event.create, unless Rails changes that method or removes it.

[zachmargolis]

By "another way" I mean different syntax, which means something different to grep for

But we're getting really off course here, please feel free to disregard this and just go ahead and review & merge the PR as needed

[monfresh]

The user doesn't need to be :signed up either. This will avoid generating a recovery code, which will speed up the test.

[monfresh]

lgtm % comments