Add another backup option for event_decorator.message_id

[kyleschmolze]

If the webhook 'msg' attribute does not include an '_id' (this happens on my server occasionally), then we can also read the message id from the actual payload object directly. You will see in the format documentation that the '_id' field is supposed to be included in both the payload and also nested within the 'msg' field. (And yes, the '_id' refers the email sent, not some "event identifier", as clarified in the documentation.)

[lime]

Interesting. Have you reached out to Mandrill about this? If we have to work around their API inconsistencies, we could at least let them know about it. :)

[kyleschmolze]

Yeah, good call. I'll shoot them a note and let you know what they say.

[kyleschmolze]

So I heard back from Mandrill, and the guy was surprised to hear that this was occurring. I have set an alert to email me if this error occurs again, and it hasn't gone off in two weeks (with 1000s of hooks processed), so I'm gonna guess that this was an error on my part, not in the payload.

I still don't see any harm in having another backup option on the _id property, but ultimately it's up to you @lime if you want to merge. Feel free to close the PR if you don't want it.

[lime]

Actually I'm not a maintainer. :sweat_smile: I've just had a similar episode with the Mandrill webhooks in https://github.com/evendis/mandrill-rails/pull/25#issuecomment-98988926 so I thought I'd make some noise.

I don't see any harm in an extra fallback either, but that's for @tardate to decide. :)

[tardate]

thanks @kyletns and @lime for investigating and following this up! Makes the job of maintainer easier;-) Yes, I don't see any harm either, so I'll pull in the change.

PS: for a while I've been wondering if it might be an idea to subclass the decorator based on payload, so the special accessors can be more specific and fit for use. Any thoughts good/bad idea?

[lime]

PS: for a while I've been wondering if it might be an idea to subclass the decorator based on payload, so the special accessors can be more specific and fit for use. Any thoughts good/bad idea?

I've only used one type of events (inbound) so far, so I can't really say. But it sounds reasonable. :)

[kyleschmolze]

I'm using this for almost every event type (inbound, sent, click, open, bounce, reject, spam), and my vote would be: Probably not worth the mental overhead of introducing new classes. They're all basically the same object anyways, just sometimes with/without certain fields. I would keep it simple and just add as many helper methods as you can for all event types.

A quick example: My "click" handler first checks if our database already has an entry for that email. If we don't have an entry for it, it first just calls the "sent" handler with the payload, before processing the click. That way, even if I get a click webhook for an email before the sent webhook comes in, the object is created and the click registered properly. If they were different types, this type of simple operation would become confusing. But since, in reality, they have 90% of the same fields, it works great.

Just my two cents. Thanks again for building this great lib!

[kyleschmolze]

Hey all, quick update: I figured out which events were coming in with the msg._id field blank. If a click or open occurs before the email has been indexed by Mandrill, then the msg field is essentially blank. It's not entirely empty though, it looks like this when it comes in:

   "msg":{  
      "tags":[  

      ],
      "sender":null,
      "template":null
   }

So you can't just check if msg is null, so I think the solution of using the payload's _id attribute as backup (as in my PR) is a good way to go.

Now my problem I'm trying to figure out is, if they send me one of these events, and then the "sent" webhook comes in afterwards, how do I know if I'm double-counting the opens/clicks...?

[tardate]

sorry took me a while to get back to this. merged; it will be in 1.3.1