9616 order cycle open webhook

[dacook]

Work in progress

Currently, we have no way of sending a webhook. There's no pre-built way of doing this for Rails surprisingly, although this tutorial looks like a good formula. I followed this, but found some parts unnecessary, and ended up with a different solution. I've pushed through to prove the concept, and was able to trigger an event in Zapier, sending the enterprise name to an email address (yay!). But there's more work to do.

My focus was on making this work firstly for this single event type. Of course we'd like to extend it to for other events in the future, but that will take a bit more work.

It also currently only allows one endpoint URL configured per enterprise. But I think I will go ahead and extend it to allow many, because it seems like a similar amount of work and I expect it will be needed before long. This would require a table or list view in the admin interface with the ability to edit/update/delete and test each webhook (with result appearing below the list I guess). Any tips on an existing example to follow?

---

What? Why?

Closes #9616

What should we test?

• Visit ... page.

Release notes

Changelog Category: User facing changes | Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Depends on #9711

Documentation updates

[dacook]

HI @jibees or @mkllnk, this is not yet finished, but it would be great to have someone's eyes over it to make sure we're heading in the right direction. Would you mind starting perhaps by reviewing the commit messages and let me know if you have any concerns, or you're happy for me to proceed? (the first six commits are from another PR and can be ignored)

You can see the further introduction and screenshot in description above.

[dacook]

Hi @mkllnk , I'd like to validate this approach that I've tried for the form, in this commit: https://github.com/openfoodfoundation/openfoodnetwork/pull/9687/commits/ef82335ebafa49a45caff976431c68f87a623574. As I already had the code ready for multiple endpoints, I've re-used this to add a simple tabular form. I intend to add an action column with delete link, and from there it would be simple to allow multiple endpoints.

There's a couple of caveats but I think they're ok.

• after UsersController#update succeeds, it always returns to the first tab (orders) with message "Account updated". But that's already the case for updating account settings.
• if UsersController#update fails (eg due to validation error), it renders the edit form, which only shows user account settings. I think that hardly matters though, given that I've made use of browser validation to ensure the field is non-empty and contains a url.

Edit screen:

Validation error (although I guarded against this particular error)

Does that seem ok to you?

[mkllnk]

Yes, that's fine for now. It's a pre-existing problem. And I hope that it can be improved when removing AngularJS.

[dacook]

Thank you Luis, this is really helpful feedback! I've responded to some comments and will take this feedback on when I pick the task back up next year.

[dacook]

User Interface: (some tidying up to be done..)

[lin-d-hop]

So exciting :grin:

[dacook]

Thanks @luisramos0 , this is valuable feedback. It seems this system design is not quite right, but I'm not clear on the best path and would like to communicate with the team. I will try to process this and perhaps create a forum post to discuss.

[dacook]

Hi @mkllnk , I'm asking you first because we originally discussed this together and hopefully it makes sense to you. Let me know if we should move to the forum. I've outlined the current structure in the PR description above. But, as you might see in some of the commit messages, I intend to make some changes thanks to Luis' comments.

4. SubscriptionPlacementJob called from OrderCycleOpenedJob I moved the existing SubscriptionPlacementJob to be triggered from here, along with the new OrderCycleWebhookJob. This was to create a consistent way to trigger the event "order cycle opened" within our system. However Luis remarks that it's a bad idea to mix two different tasks. Also I think it's not strictly necessary for this PR (it already worked fine before).

If it was necessary, I now think that this would be better achieved using an internal events system, like described here. That sounds cool and would probably help us scale to the next level..

But do you agree about just leaving SubscriptionPlacementJob as it is for now?

3. OrderCycleWebhookJob: creating and scheduling the payloads: Luis suggests this logic should be in a service or elsewhere. I think I agree, and I think performance would be fine if we just called it directly from the OrderCycleWebhookJob, because I don't imagine there being many subscribers to an order cycle (currently it's limited to the owner of the coordinator or distributors).

It could be taken further with a specific type of webhook class. A framework that defines and enforces certain parts of the webhook and payload structure. We'll probably also want to consider versioning and swagger documentation (but i think that should be in the future).

If we did need to optimise, then I would investigate using an internal event system with async jobs (sidekiq) to solve the problem (also mentioned in the linked article). That's essentially what I built already, but instead of adding yet more specific job classes, an event system would be a more generic and scalable solution.

5. OrderCycleOpenedJob: marking order cycles as having been 'notified' Luis makes a good point that the name opened_notification_at doesn't relate to what it's doing. It was intended to say "all tasks related to the opening of an order cycle have been triggered", and prevent accidentally triggering them again.

He recommends having a separate table as a log. I can't think of what to name that yet.

But I'm thinking why not maintain a more generic state field instead (state: :upcoming > :open > :closed). This doesn't say that the webhooks were generated, just that the OrderCycleOpenedJob ran and doesn't need to run again. We can then also use this to replace OrderCycleClosingJob's processed_at, which really means state: :closed. What do you think of that approach?

PS let me know if you'd like to discuss in a call; I'm available tomorrow (Friday) morning only.

[dacook]

Thanks for the discussion @mkllnk and @luisramos0. Based on the above, I've created a new issue for a path forward with event management: https://github.com/openfoodfoundation/openfoodnetwork/issues/10457

For this PR, I will avoid intersecting with that as much as possible.

[dacook]

Sorry I requested review prematurely. This is now rebased, and all tests are green.

[mkllnk]

I've applied your suggestions as new commits which I'll rebase once you've reviewed.

Oops, I didn't get to review before the rebase but I'll do my best in retrospect.

[dacook]

Oops, I didn't get to review before the rebase but I'll do my best in retrospect.

Sorry, that was a mistake in what I said. I just meant that I rebased from master to resolve conflicts. Anyway, thanks for the review, I've now also rebased to incorporate the changes (including the position of the FailedRequestError class in "Raise error on server error").

[dacook]

Now ready for a second review. @luisramos0 I understand if you don't have time, but if you do, would you like to re-review?

[dacook]

Ready to test!

Tested deployment on au-staging: https://semaphoreci.com/openfoodfoundation/openfoodnetwork-2/servers/au-staging

[filipefurtad0]

Hey @dacook,

Thanks, really, for all the detail in the description of the PR. It comes really handy.

1. Receiving payload, after OC is opened :heavy_check_mark:

After enabling the API key for a user which is coordinator and distributor of an OC, we can check the UI:

Creating the webhook displays:

Proceeded to create an order cycle to open in a future time, taking note of the order cycle id ("id": 3671) I've found it easier to use the procedure to open the OC as per this wiki -> thanks again to @luisramos0 for introducing this nice testing code, back in the day!! :heart: This way, we don't need to consider timezone differences when creating the OC, and there is no wait-time till it opens :tada:

Some moments afterwards, the webhook site was updated :rocket: we can check the payload below:

And compare it to the info we get from the rails console, on the server:

[#<OrderCycle:0x00000000090d9540
  id: 3671,
  name: "webhook_OC",
  orders_open_at: Thu, 16 Mar 2023 12:32:55.095320000 EDT -04:00,
  orders_close_at: Thu, 16 Mar 2023 13:02:55.095421000 EDT -04:00,
  coordinator_id: 1822,
  created_at: Thu, 16 Mar 2023 12:38:36.100790000 EDT -04:00,
  updated_at: Thu, 16 Mar 2023 12:49:23.926191000 EDT -04:00,
  processed_at: nil,
  automatic_notifications: false,
  mails_sent: false,
  opened_at: Thu, 16 Mar 2023 12:49:23.926191000 EDT -04:00>]

All good here!

2. Not receiving any payload, after deleting the webhook endpoint :heavy_check_mark:

After deleting the endpoint, we receive no payload, when the OC is re-opened.

3. Re-entering the webhook endpoint on /account#/developer_settings :heavy_check_mark: ...works as before, and delivers the payload as expected:

4. Opening an order cycle in which the user is supplier and distributor but not the coordinator :heavy_check_mark:

Also works, payload is delivered with the respective data ("id": 3661):

Notice the different coordinator_name, when comparing to the examples in 1) and 3).

The UI is according to specification, functionality as well, data is consistent. I'd say this looks great - awesome! :clap: :clap: :clap: Merging.

[dacook]

📚 Draft documentation

See here: https://github.com/openfoodfoundation/ofn-UserGuide/issues/41#issuecomment-1473118352