Add webhook triggered on Order Cycle Open #9687

dacook · 2022-09-20T04:08:07Z

What? Why?

Closes Webhook triggered on Order Cycle Open #9616

As an API user I want to be notified when an OC opens.

A webhook can be sent to a service like Zapier, which can generate an email notification or perform some other action.

How?

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.

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 let's get one working first before making further decisions.

There's bit going on here. A quick summary:

WebhookDeliveryJob: a job that will be enqueued to deliver a payload for each triggered webhook
WebhookEndpoint: a model to store each webhook endpoint url (in the future it would also store the event type it is listening for).
OrderCycleWebhookService: create webhook payloads for an order cycle event.
OrderCycleOpenedJob: Uses OrderCycleWebhookService to schedule WebhookDeliveryJobs. The order cycle remembers if it's been opened, and can only open once.
(removed)
User account interface to manage webhooks. I was surprised how hard it was to fiddle around and keep the form simple, but got there in the end.
- Rather than enable/disable as originally suggested, you can just create or delete which has the same effect.
- a test mode hasn't been implemented yet, in order to keep this PR from dragging out.

What should we test?

This is a new feature which doesn't touch any existing functionality.
It adds a new form to the Developer tab on the user Accounts page.
Suggested tests:

Prepare a webhook URL ready to receive and capture a webhook payload. The easiest way is:
- Visit https://webhook.site/
- Copy Your unique URL
- Keep the tab open to check results later.
Create/edit a user with API access:
- Admin > Users > Edit
- Tick "SHOW API KEY VIEW FOR USER", save.
Login as this user
Go to My account > Developer settings > Webhook Endpoints
- Verify the interface as per requirements.
Enter your unique Webhook URL and click create
- Verify that the URL has been saved.
Set up an Order Cycle to open at the current time (anytime in the last hour):
- Admin > Order Cycles > + New Order Cycle
- Choose co-ordinator and click continue
- Enter a name, and choose a time that is about to pass
- Save (no need to complete further steps).
Wait up to 5 minutes for the webhook event to be captured ☕️
(refer back to the open tab for https://webhook.site/)
- Verify the result as per requirements
De-activate the webhook:
- Go to My account > Developer settings > Webhook Endpoints
- Delete the webhook endpoint URL.
Set up another Order Cycle to open at the current time
Wait minimum 5 mins
- Verify that no event was received.

Release notes

Changelog Category: User facing changes

If documentation has been written, share a link in the release notes

Documentation updates

I think it should be documented wherever the API is documented. But I can't find any public mention of the API??!
It seems worth mentioning in the user guide: https://guide.openfoodnetwork.org/

Ideally, a test mode will display the webhook payload format, and act as in-place documentation. But this will come in a future PR.

In the future, I suggest we integrate with our OpenAPI documentation too.

app/jobs/order_cycle_opening_job.rb

app/controllers/spree/users_controller.rb

mkllnk · 2022-11-09T02:45:56Z

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

luisramos0

Hi David!
This is a good start! I left a few comments as there are a couple of code/DB design problems imo.

Webhooks usually are named, to have webhooks for multiple events in the system we need named webhooks. For this first PR I'd recommend we have on the webhooks table a "webhook_type" column that we make "order_cycle_opened". On the user page this is a select box with only one option for now: so the user can add a webhook endpoint for a particular event, in this case "Order Cycle Opened".
As mentioned below on not overloading the OC table, I'd create webhook_log table that the Job would fill when the webhook is called, including user_id, oc_id (this can be a generic column like "event_target_object_id" so that other events can put other data in there), etc. This is better than having webhook log data scattered around the data model.

Good luck with this. This is a good start to a great feature!

app/jobs/webhook_delivery_job.rb

luisramos0 · 2022-12-14T13:56:59Z

app/jobs/order_cycle_opened_job.rb

+  def perform
+    ActiveRecord::Base.transaction do
+      recently_opened_order_cycles.find_each do |order_cycle|
+        SubscriptionPlacementJob.perform_later(order_cycle.id)


I dont understand the need to mix subscription placement logic with the web hook logic. I think this can be problematic. Can we separate concerns and just keep webhook notification on this job?

I thought that it would be good to have one place where we define the logic selecting and processing order cycles as "opened".
It does seem strange to be creating so many jobs, yes, but it helps "separate concerns" in other ways I guess. I thought it would be more robust this way.

I understand but I think this field name is very misleading. What is a notification? I'd use the same terminology everywhere so that code readers will know what this means. It's a webhook. It could be called opened_webhook_sent_at.

I still think it would be much better and very easy to implement this outside the OC table (there's already lots of complexity in OCs).
A table where webhooks calls are registered is important imho. This would be one of them.

I think I agree, but this would be better considered as part of #10457. So for now, I plan to rename this column to match specifically what it refers to.

app/jobs/application_job.rb

luisramos0 · 2022-12-14T14:02:12Z

db/migrate/20220919063831_add_opened_notification_at_to_order_cycle.rb

+
+class AddOpenedNotificationAtToOrderCycle < ActiveRecord::Migration[6.1]
+  def change
+    add_column :order_cycles, :opened_notification_at, :timestamp


Is this field on the OC meant to mark that a webhook was called? I'd recommend the name of the column to mention that.
I think this should not be on this OC table but rather on a specific webhook calls table/log. This way we are adding complexity to the critical/core OC table without a real need.

Hmm coming back to this I have a different answer now. We're not intending to log when/if a webhook was called.
This is a flag to ensure the order cycle's "open" state is triggered once, and only once. The application code then schedules any webhooks needing to be delivered. I think I'll leave this as-is for now.

dacook · 2022-12-15T06:42:10Z

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.

lin-d-hop · 2023-02-07T04:52:19Z

So exciting 😁

luisramos0

Hi David, I leave a few more suggestions.

app/jobs/webhook_delivery_job.rb

luisramos0 · 2023-02-15T22:12:13Z

app/jobs/order_cycle_opened_job.rb

+  def perform
+    ActiveRecord::Base.transaction do
+      recently_opened_order_cycles.find_each do |order_cycle|
+        SubscriptionPlacementJob.perform_later(order_cycle.id)


I understand but I think this field name is very misleading. What is a notification? I'd use the same terminology everywhere so that code readers will know what this means. It's a webhook. It could be called opened_webhook_sent_at.

I still think it would be much better and very easy to implement this outside the OC table (there's already lots of complexity in OCs).
A table where webhooks calls are registered is important imho. This would be one of them.

app/jobs/order_cycle_webhook_job.rb

luisramos0 · 2023-02-15T22:16:00Z

app/jobs/order_cycle_opened_job.rb

+    ActiveRecord::Base.transaction do
+      recently_opened_order_cycles.find_each do |order_cycle|
+        SubscriptionPlacementJob.perform_later(order_cycle.id)
+        OrderCycleWebhookJob.perform_later(order_cycle.id, 'order_cycle.opened')


This type of event 'order_cycle.opened' should be extracted to constant somewhere.

I'm not sure where/how exactly yet, but will try to address this in my next update next week.

dacook · 2023-02-16T01:48:14Z

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.

This job is responsible for delivering a payload for one webhook event only. It allows the action to run asynchronously (and not slow down the calling process).

And thus retry later. I tried to test that it actually retries, or ensuring the job remained in the queue to be retried, but couldn't get it to work.

Best reviewed with whitespace hidden. Unfortunately the spec isn't allowed in CI. But it worked on my environment, I promise. I chose `xit` so that it doesn't run unnecessarily. Perhaps we could use `pending` instead, which would execute, and notify us if it suddenly started working one day. But I doubt it.

This will store the URL for each user that wants a notification. We probably don't need URL validation (it's not done on Enterprise for example). It could be validated by browser input, and anyway will be validated if the webhook actually works or not. Inspired by Keygen: https://keygen.sh/blog/how-to-build-a-webhook-system-in-rails-using-sidekiq/

Although we won't be allowing multiple in the this PR, we certainly plan to in the future. The migration helper add_reference couldn't handle the custom column name, so I had to put it together manually.

…er cycle event

But not supplier owners.

Using the clever concurrency testing borrowed from SubscriptionPlacementJob, but I thought a shorter pause time (just 100ms) would be sufficient. I considered doing this with a new 'state' field (upcoming/open/close), but decided to keep it simple.

Allowing creation and deleting via the user association. It probably won't be much effort to allow editing and multiple records, but I cut it down to the minimum needed to avoid any further delays. I couldn't find a way to test a failure in the destroy method, but decided to keep the condition because I thought it was worth having.

Co-authored-by: Maikel <[email protected]>

dacook · 2023-03-07T04:43:11Z

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 · 2023-03-07T04:50:00Z

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?

Not sure if Luis will have time to re-review.

rioug

Looks good 👏 I left a few comments but nothing that should block the PR.
I like your use of pending specs for work to be done, which I should start using myself

app/services/order_cycle_webhook_service.rb

spec/jobs/order_cycle_opened_job_spec.rb

spec/models/order_cycle_spec.rb

spec/controllers/webhook_endpoints_controller_spec.rb

The best way to check if something changed or not, is with 'change' of course.

dacook · 2023-03-16T00:01:45Z

Ready to test!

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

filipefurtad0 · 2023-03-16T17:23:39Z

Hey @dacook,

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

Receiving payload, after OC is opened ✔️

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!! ❤️ This way, we don't need to consider timezone differences when creating the OC, and there is no wait-time till it opens 🎉

Some moments afterwards, the webhook site was updated 🚀 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!

Not receiving any payload, after deleting the webhook endpoint ✔️

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

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

Opening an order cycle in which the user is supplier and distributor but not the coordinator ✔️

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! 👏 👏 👏
Merging.

dacook · 2023-03-20T22:44:29Z

📚 Draft documentation

See here: openfoodfoundation/ofn-UserGuide#41 (comment)

sigmundpetersen assigned dacook Sep 21, 2022

dacook force-pushed the 9616-order-cycle-open-webhook branch 4 times, most recently from 591ae53 to ccf3d5f Compare September 30, 2022 02:02

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

dacook force-pushed the 9616-order-cycle-open-webhook branch from ccf3d5f to ae1ecc3 Compare October 7, 2022 05:31

mkllnk reviewed Oct 9, 2022

View reviewed changes

app/jobs/order_cycle_opening_job.rb Outdated Show resolved Hide resolved

dacook force-pushed the 9616-order-cycle-open-webhook branch 2 times, most recently from 5444c1a to 33dd37c Compare October 28, 2022 06:44

dacook commented Nov 4, 2022

View reviewed changes

app/controllers/spree/users_controller.rb Outdated Show resolved Hide resolved

dacook force-pushed the 9616-order-cycle-open-webhook branch 2 times, most recently from 65292a4 to b1e4c2c Compare November 4, 2022 06:42

This comment was marked as outdated.

Sign in to view

luisramos0 previously requested changes Dec 14, 2022

View reviewed changes

mkllnk mentioned this pull request Jan 18, 2023

Add Rails standard ApplicationJob for consistency #10283

Merged

dacook force-pushed the 9616-order-cycle-open-webhook branch from b1e4c2c to 02520ac Compare January 20, 2023 04:26

dacook force-pushed the 9616-order-cycle-open-webhook branch from 02520ac to 5fb6ab4 Compare February 1, 2023 00:25

This comment was marked as outdated.

Sign in to view

dacook force-pushed the 9616-order-cycle-open-webhook branch from 5fb6ab4 to 9c6c9db Compare February 7, 2023 05:54

dacook mentioned this pull request Feb 7, 2023

Webhook triggered on Order Cycle Open #9616

Closed

dacook force-pushed the 9616-order-cycle-open-webhook branch from 9c6c9db to b4be598 Compare February 10, 2023 04:32

luisramos0 reviewed Feb 15, 2023

View reviewed changes

dacook force-pushed the 9616-order-cycle-open-webhook branch 2 times, most recently from 7a9993c to 7c91867 Compare February 16, 2023 01:22

dacook and others added 11 commits March 7, 2023 15:38

1. Add WebhookDeliveryJob

9d19f37

This job is responsible for delivering a payload for one webhook event only. It allows the action to run asynchronously (and not slow down the calling process).

Raise error on server error

9741935

And thus retry later. I tried to test that it actually retries, or ensuring the job remained in the queue to be retried, but couldn't get it to work.

User may have many WebhookEndpoints [migration]

778baba

Although we won't be allowing multiple in the this PR, we certainly plan to in the future. The migration helper add_reference couldn't handle the custom column name, so I had to put it together manually.

3. Add OrderCycleWebhookService to create webhook payloads for an ord…

ba152f1

…er cycle event

Also send webhook payloads for distributor owners

b91cabc

But not supplier owners.

4. OrderCycleOpenedJob triggers webhook

739df4b

Apply suggestions from code review

9d5ca22

Co-authored-by: Maikel <[email protected]>

dacook force-pushed the 9616-order-cycle-open-webhook branch from 9657974 to 9d5ca22 Compare March 7, 2023 04:43

mkllnk approved these changes Mar 7, 2023

View reviewed changes

dacook mentioned this pull request Mar 7, 2023

Webhook triggered on Order Cycle close #10528

Open

rioug approved these changes Mar 14, 2023

View reviewed changes

dacook changed the title ~~9616 order cycle open webhook~~ Add webhook triggered on Order Cycle Open Mar 15, 2023

Tidy up spec

d59074d

The best way to check if something changed or not, is with 'change' of course.

dacook force-pushed the 9616-order-cycle-open-webhook branch from 0c6bf20 to d59074d Compare March 15, 2023 01:18

dacook requested a review from rioug March 15, 2023 01:45

rioug approved these changes Mar 15, 2023

View reviewed changes

filipefurtad0 self-assigned this Mar 16, 2023

filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Mar 16, 2023

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Mar 16, 2023

filipefurtad0 merged commit 9ea6fa5 into openfoodfoundation:master Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webhook triggered on Order Cycle Open #9687

Add webhook triggered on Order Cycle Open #9687

dacook commented Sep 20, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

mkllnk commented Nov 9, 2022

luisramos0 left a comment •

edited

Loading

luisramos0 Dec 14, 2022

dacook Dec 15, 2022

luisramos0 Feb 15, 2023

dacook Feb 17, 2023

luisramos0 Dec 14, 2022

This comment was marked as outdated.

dacook Feb 7, 2023 •

edited

Loading

dacook commented Dec 15, 2022

This comment was marked as outdated.

lin-d-hop commented Feb 7, 2023

luisramos0 left a comment

luisramos0 Feb 15, 2023

luisramos0 Feb 15, 2023

dacook Feb 17, 2023

dacook commented Feb 16, 2023

dacook commented Mar 7, 2023

dacook commented Mar 7, 2023

rioug left a comment

dacook commented Mar 16, 2023 •

edited

Loading

filipefurtad0 commented Mar 16, 2023 •

edited

Loading

dacook commented Mar 20, 2023

Add webhook triggered on Order Cycle Open #9687

Add webhook triggered on Order Cycle Open #9687

Conversation

dacook commented Sep 20, 2022 • edited Loading

What? Why?

How?

What should we test?

Release notes

Documentation updates

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

mkllnk commented Nov 9, 2022

luisramos0 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

dacook Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

dacook commented Dec 15, 2022

This comment was marked as outdated.

lin-d-hop commented Feb 7, 2023

luisramos0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook commented Feb 16, 2023

dacook commented Mar 7, 2023

dacook commented Mar 7, 2023

rioug left a comment

Choose a reason for hiding this comment

dacook commented Mar 16, 2023 • edited Loading

filipefurtad0 commented Mar 16, 2023 • edited Loading

dacook commented Mar 20, 2023

📚 Draft documentation

dacook commented Sep 20, 2022 •

edited

Loading

luisramos0 left a comment •

edited

Loading

dacook Feb 7, 2023 •

edited

Loading

dacook commented Mar 16, 2023 •

edited

Loading

filipefurtad0 commented Mar 16, 2023 •

edited

Loading