Retry webhook emission if failure

[ryanwjackson]

Fix #20

[mattgmarcus]

just had some comments.

did you get a chance to test this too?

[mattgmarcus]

Should this be specific to the organization that you're retrying for? Otherwise, how would you know who was the last organization notified?

[ryanwjackson]

@mattgmarcus this is sent to the provider (e.g. ModernTreasury). The event ID is also included in that email. It's not organization-specific, because if the webhook endpoint is down, it's likely down for all organizations.

[mattgmarcus]

Ah got it

[mattgmarcus]

Do you need an event.save!

[ryanwjackson]

@mattgmarcus I do need to add tests for this still.

[ryanwjackson]

@mattgmarcus After some research, it doesn't give you too much control over retries: https://github.com/mperham/sidekiq/wiki/Job-Lifecycle#altering-the-lifecycle

It just allows you to set the number of retries but not the backoff. Here is the hard-coded backoff: https://github.com/mperham/sidekiq/wiki/Error-Handling#automatic-job-retry

If we are OK with that, I can do the following:

• Remove retries column from events
• Add exception handling and provider email notifications to Forms::Events::Emit, though will probably need a way to store when we last sent an email. The first retry is 30s and then increases exponentially, so they would get a few emails the first 24 hours if not.
• Remove Forms::Events::Retry

Thoughts?

[mattgmarcus]

I say we use the default Sidekiq retry stuff. We can customize later if we want but let's just keep it simpler. Your approach looks good.
As for keeping track of the last emails you got - could we just punt on that and track in a separate ticket? I know that could get annoying to have a bunch of emails with scale. But we can fix this later (i.e. not needed to ship)

[ryanwjackson]

We can, although relatively easy to do as that codes already written so it sends at most one in a 24 hour period.

…

-- Ryan Jackson ryanwjackson@gmail.com

On Sat, Sep 28, 2019 at 12:02 PM Matt Marcus ***@***.***> wrote: I say we use the default Sidekiq retry stuff. We can customize later if we want but let's just keep it simpler. Your approach looks good. As for keeping track of the last emails you got - could we just punt on that and track in a separate ticket? I know that could get annoying to have a bunch of emails with scale. But we can fix this later (i.e. not needed to ship) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#49?email_source=notifications&email_token=AAFAT3UVNTQRW42DXFUODKDQL55Z3A5CNFSM4I26IKN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD724XYA#issuecomment-536202208>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFAT3XSWZAQCDDXV2ZOXSLQL55Z3ANCNFSM4I26IKNQ> .

[mattgmarcus]

Fair enough. Okay let’s use sidekiq and keep the stuff to do once a day. Sound good?

…

On Sat, Sep 28, 2019 at 3:33 PM Ryan ***@***.***> wrote: We can, although relatively easy to do as that codes already written so it sends at most one in a 24 hour period. -- Ryan Jackson ***@***.*** On Sat, Sep 28, 2019 at 12:02 PM Matt Marcus ***@***.***> wrote: > I say we use the default Sidekiq retry stuff. We can customize later if we > want but let's just keep it simpler. Your approach looks good. > As for keeping track of the last emails you got - could we just punt on > that and track in a separate ticket? I know that could get annoying to have > a bunch of emails with scale. But we can fix this later (i.e. not needed to > ship) > > — > You are receiving this because you were assigned. > Reply to this email directly, view it on GitHub > < #49?email_source=notifications&email_token=AAFAT3UVNTQRW42DXFUODKDQL55Z3A5CNFSM4I26IKN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD724XYA#issuecomment-536202208 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAFAT3XSWZAQCDDXV2ZOXSLQL55Z3ANCNFSM4I26IKNQ > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49?email_source=notifications&email_token=ABEI75HTHAH3G5N3XPNPLM3QL6WSHA5CNFSM4I26IKN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73A6CA#issuecomment-536219400>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEI75AZZX5PKTGCRFETQXLQL6WSHANCNFSM4I26IKNQ> .

[ryanwjackson]

Yup. Give me a few and will sub it for a review.

On Sat, Sep 28, 2019 at 15:34 Matt Marcus ***@***.***> wrote: Fair enough. Okay let’s use sidekiq and keep the stuff to do once a day. Sound good? On Sat, Sep 28, 2019 at 3:33 PM Ryan ***@***.***> wrote: > We can, although relatively easy to do as that codes already written so it > sends at most one in a 24 hour period. > > > -- > Ryan Jackson > ***@***.*** > > > On Sat, Sep 28, 2019 at 12:02 PM Matt Marcus ***@***.***> > wrote: > > > I say we use the default Sidekiq retry stuff. We can customize later if > we > > want but let's just keep it simpler. Your approach looks good. > > As for keeping track of the last emails you got - could we just punt on > > that and track in a separate ticket? I know that could get annoying to > have > > a bunch of emails with scale. But we can fix this later (i.e. not needed > to > > ship) > > > > — > > You are receiving this because you were assigned. > > Reply to this email directly, view it on GitHub > > < > #49?email_source=notifications&email_token=AAFAT3UVNTQRW42DXFUODKDQL55Z3A5CNFSM4I26IKN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD724XYA#issuecomment-536202208 > >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/AAFAT3XSWZAQCDDXV2ZOXSLQL55Z3ANCNFSM4I26IKNQ > > > > . > > > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > < #49?email_source=notifications&email_token=ABEI75HTHAH3G5N3XPNPLM3QL6WSHA5CNFSM4I26IKN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73A6CA#issuecomment-536219400 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ABEI75AZZX5PKTGCRFETQXLQL6WSHANCNFSM4I26IKNQ > > . > — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#49?email_source=notifications&email_token=AAFAT3QIIG5UPVIJQ56XMMLQL6WVRA5CNFSM4I26IKN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73A6SI#issuecomment-536219465>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFAT3XBG6DIYQ43H2UEKTLQL6WVRANCNFSM4I26IKNQ> .

--

…

-- Ryan Jackson ryanwjackson@gmail.com

[ryanwjackson]

@mattgmarcus Feel free to merge if you approve (and tests pass). I increased test coverage above 90%, though we have some work to do there in general.