ankane / ahoy_email

First-party email analytics for Rails
MIT License
1.11k stars 137 forks source link

Instance model #142

Closed JensDebergh closed 3 years ago

JensDebergh commented 4 years ago

Hey there!

I'm tracking opened mails for particular mails inside my application. I stumbled on this hard to track bug because of how ahoy_email works

Yesterday I received an email from a user where he complained that he received an email that was destined for another user. Luckily it didn't contain any sensitive information.

module AhoyEmail
  class MailOpenedSubscriber < Base
    MAILER = "LedgerItemMailer#my_mail".freeze

    def open(event)
      super(event)

      if mailer === MAILER
        # Get workspace owner
        user_id = workspace.owner.id

          MyMailer.SendMail(user_id).deliver_later
      end
    end

    def workspace
      @workspace ||= Workspace.find(workspace_id)
    end

    def workspace_id
      extra_data["workspace_id"]
    end
  end
end

Since the subscribers are instantiated in the initializer, ahoy_email always use the same instance until garbage collected I guess.

I had following scenario. 2 opened events were received from ahoyemail. First run through everything works as intended.

Second run through it uses all the correct event_data but it is still using the old @workspace instance variable since I'm doing this:

    def workspace
      @workspace ||= Workspace.find(workspace_id)
    end

Since this is a very common pattern in ruby it is easy to create a bug like this with devastating effects.

Is it possible to to create "functional" subscribers which are instantiated by giving a class on the fly instead of using an object?

Instead of:

AhoyEmail.subscribers = [AhoyEmail::LedgerItemMailOpenedSubscriber.new] 

We would do this:

AhoyEmail.subscribers = [AhoyEmail::LedgerItemMailOpenedSubscriber]

Ahoy mail instantiates the subscriber when the event arrives. Every run through is a fresh instance object where faults like the one I mentioned above would never be able to happen.

Thoughts on this?

Kind regards Jens

ankane commented 3 years ago

Hey @JensDebergh, thanks for the detailed report, and sorry for the delay. This makes sense to me. I've added support for classes, and will update the docs to make this the preferred method when the next release goes out.

JensDebergh commented 3 years ago

@ankane No worries! I really appreciate your hard work on this library. It helps a ton!

Happy new year!

ankane commented 3 years ago

Thanks @JensDebergh, happy new year to you, too!