Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Webhook triggered on Order Cycle Open #9616

Closed
Tracked by #9615
lin-d-hop opened this issue Aug 31, 2022 · 14 comments · Fixed by #9687
Closed
Tracked by #9615

Webhook triggered on Order Cycle Open #9616

lin-d-hop opened this issue Aug 31, 2022 · 14 comments · Fixed by #9687
Assignees

Comments

@lin-d-hop
Copy link
Contributor

lin-d-hop commented Aug 31, 2022

Description

As an API user I want to be notified when an OC opens.
As an API user I want to be able to turn notifications on and off

Acceptance Criteria & Tests

  1. EDIT We need to be able to turn webhook posting on and off via the UI. This should be done via and additional tab in 'Enterprise Settings' called 'API'. This tab should only be visible for enterprises with shopfronts enabled.
    In product call 26/10 we made the decision to have all API settings at the User level in Developer Settings. The intention is to create the framework for another user type - API user.
    The UI will exist in https://openfoodnetwork.org.uk/account#/developer_settings
    This UI is visible when super-admin enables 'API Key View' for the user.

  2. This setting will be called 'Post webhook on Order Cycle Open'. In line with other settings two radio buttons will denote enabled/disabled:
    Screenshot from 2022-08-31 17-05-57

  3. When 'enabled' a url is shown where the payload will be sent to.

  4. The webhook payload will be triggered when an OC opens for any enterprises this user manages and owns. It is applicable for all distributor and coordinator enterprises (and not for one's for which the user supplies only).

  5. A tooltip should be included saying 'This will trigger a webhook to post the following payload to the given URL when an order cycle opens:
    "id": STRING,
    "at": DATETIME,
    "type": "order_cycle_open",
    "data": {
    "id": INTEGER,
    "name": STRING,
    "open_at": DATETIME,
    "close_at": DATETIME,
    "enterprise_id": INTEGER,
    "enterprise_name": STRING
    "tags": [STRING]
    }"

  6. The payload delivered should be:
    "id": STRING (ID of the webhook)
    "at": DATETIME (when payload sent)
    "type": "order_cycle_open",
    "data": {
    "id": INTEGER (ID of the OC)
    "name": STRING (name of the OC)
    "open_at": DATETIME (OC opening time)
    "close_at": DATETIME (OC closing time)
    "enterprise_id": INTEGER (enterprise ID in DB)
    "enterprise_name": STRING (enterprise name)
    "tags": [STRING] (tags on the OC)
    }

For example:
{
"id": "EV09374580",
"at": "2022-06-15T04:18:00-04:00",
"type": "order_cycle_open",
"data": {
"id": 79234596,
"name": "Aug 3rd"
"open_at": "2022-06-15T04:18:00-04:00",
"close_at": "2022-07-15T04:18:00-04:00",
"enterprise_id": 2093846
"enterprise_name": "Fictional Hub"
}
}

  1. Unit Test coverage as standard
  2. Translations as standard on the Enterprise Settings page.

Other useful Bits

This tutorial looks good:
https://keygen.sh/blog/how-to-build-a-webhook-system-in-rails-using-sidekiq/

@dacook dacook self-assigned this Sep 13, 2022
@dacook

This comment was marked as outdated.

@dacook

This comment was marked as outdated.

@drummer83
Copy link
Contributor

We need to be able to turn webhook posting on and off via the UI. This should be done via and additional tab in 'Enterprise Settings' called 'API'. This tab should only be visible for enterprises with shopfronts enabled.

Reading through this I was just wondering if the 'API' tab should only be visible if the 'API view' is activated for this user (in admin/users/ ID /edit)? Just like the display of the API key in the user account? @lin-d-hop

@dacook
Copy link
Member

dacook commented Oct 7, 2022

Hi @lin-d-hop , Maikel has raised a good question about which enterprises for an order cycle should be notified?
I had initially assumed that the co-ordinator would be notified, but I'm now aware that there can also be different distributor and suppliers enterprises attached to an order cycle. I wonder should all three be able to receive a notification (when they subscribe)?
Re-reading your criteria, I see you mentioned only those with shopfronts enabled, so I guess you intended only distributors.

Perhaps we could make the event type name more specific to support each possibility in the future. Eg:

  • coordinator.order_cycle.open: Notify the enterprise when any cycles open that they are co-ordinating.
  • distributor.order_cycle.open: Notify the enterprise when any cycles open that they are distributing.
  • supplier.order_cycle.open: Notify the enterprise when any cycles open that they are supplying for.

I'm still getting familiar with how things work so I might be over-thinking it. Let me know what you think.

Also can you please answer Konrad's question above?

@kirstenalarsen
Copy link
Contributor

My feeling is probably that it doesn't need to be that complicated - perhaps any coordinator, distributor or supplier in the OC can subscribe? and it's the same information for all of them?

But then I wonder why only them . . this is information that we will likely want to use on other sites like discovery sites . . so perhaps we do

  • Step 1: available with API for any enterprise in the order cycle
  • Step 2 [not in this PR]: some kind of setting somewhere where the coordinating enterprise can make OC webhook publicly available or not?

@lin-d-hop
Copy link
Contributor Author

lin-d-hop commented Oct 26, 2022

Re @drummer83's questions - as this is API v1 I would like this to be visible for all shopfronts. The API is pubic and having this visible in the UI to everyone will enable people to understand they can use the API.

In the case that the 'API key view' is not enabled for the user, can we display a message 'Please contact your instance manager to enable API access.'

Screenshot from 2022-10-26 08-25-23

Ping @kirstenalarsen @RachL are you happy with this?

Re who should receive:
@dacook you message provoked a really interesting conversation in product circle this morning. I've updated the spec to reflect that we'd like to manage the UI for this through https://openfoodnetwork.org.uk/account#/developer_settings
Sorry if this throws a spanner in the works you've already done. I'll post this morning in Slack about some of the rationale.... would be good to double check the reasoning with you and other devs :) Will tag you there.

@kirstenalarsen
Copy link
Contributor

Hey @lin-d-hop - updates above look all good except that i think we agreed that the webhook would be for all enterprises that the User manages and would trigger when those enterprises were either Coordinator or Distributor on the order cycle

But we are not including when User is manager of enterprises that are Suppliers on the Order Cycle

We also discussed including the Supplier Enterprise IDs in the payload, so that the Coordinator could easily set up an integration to send notifications to them if they wanted?

Should all this go into amended scope above or you are seeing as separate issues?

@lin-d-hop
Copy link
Contributor Author

I thought that's what I wrote.... will reword to clarify.

Re supplier ids - I saw this as a potential additional issue. Another solution would be an OC endpoint that could be called after the webhook with full OC details. Might be preferable.

@dacook
Copy link
Member

dacook commented Feb 7, 2023

Payload format

I've set up the proof of concept with a slightly different format to the above. Any issues?

{
  "id": "374e0a40-f1d4-410c-9dc3-965baddde66b",
  "at": "2023-02-07 17:12:45 +1100",
  "event": "order_cycle.opened",
  "data": {
    "id": 3813,
    "name": "Another test order cycle",
    "orders_open_at": "2023-02-07T17:10:00.000+11:00",
    "orders_close_at": "2023-02-08T12:00:00.000+11:00",
    "coordinator_id": 2515,
    "coordinator_name": "David's Dev Shop"
  }
}
  • The first ID is a Sidekiq job ID. It acts as a unique ID for this webhook event only, and doesn't refer to anything persistent (once successful, our system will discard the job).
  • Instead of "type", I chose "event". I think because this is a more common standard in webhooks.
  • Data is nested, and refers more directly to the Order Cycle in question. I think also because it was a more common standard.
  • The field names have been updated to more closely match the underlying names in our system, which makes it clearer where information is coming from and what it's linked to.

@dacook
Copy link
Member

dacook commented Feb 7, 2023

User interface

As discussed, this is a per-user setting. With the expectation that there will be other types of webhook, I've set up a table format (preview here). The table format won't be as conducive to showing an example of the full data structure, so I wonder if we can omit this? Maybe we can have a more general description at the bottom of the table, and link to full documentation..

Documentation

Hmm, this would require a similar type of documentation to the OpenAPI (swagger) documentation that we generate for the API. I need to learn more about how these can work together...

@lin-d-hop
Copy link
Contributor Author

This is all looking brilliant! Thanks @dacook! Thanks for you corrections to the spec 👌
Linking to the documentation sounds like a good solution. If we don't find a good Swagger style automated documenting tool then we can document in the API handbook and link to that.

@dacook
Copy link
Member

dacook commented Feb 9, 2023

@dacook
Copy link
Member

dacook commented Feb 10, 2023

📚 Regarding documentation:
OpenAPI (aka Swagger) can be used to document webhooks (example), which I think would be handy. I think it makes sense to consider webhooks another part of the application's API. Maybe it should even be packaged up as part of API v1, or versioned in its own right.

But it requires the latest version of OpenAPI (3.1), which Rswag doesn't support yet (looks like they're low on maintainers). We might also need to update the existing spec.

So I think we leave this to consider in the future.

@dacook
Copy link
Member

dacook commented Mar 20, 2023

📚 Draft documentation

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants