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

refactor: made webhook event type an enum in GraphQL schema #3025

Closed
wants to merge 4 commits into from

Conversation

oana-lolea
Copy link
Contributor

@oana-lolea oana-lolea commented Oct 9, 2024

Changes proposed in this pull request

  • Added WebhookEventType scalar in GraphQL schema
  • Added parser to restrict values to be only the allowed types
  • Added tests for the scalar methods

Context

Made WebhookEvent type a scalar instead of a string in GraphQL schema.
Webhook event types format (e.g. incoming_payment.created) are accepted. Given a string with the webhook event type, the mapper returns its corresponding WebhookEventType enum member, or throws an error if it doesn't exist.
Fixes #2786

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Make sure that all checks pass
  • Bruno collection updated (if necessary)
  • Documentation issue created with user-docs label (if necessary)
  • OpenAPI specs updated (if necessary)

@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. type: source Changes business logic pkg: mock-ase pkg: mock-account-service-lib labels Oct 9, 2024
Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 77535df
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6710e3e240240300083496db

@mkurapov
Copy link
Contributor

@BlairCurrey would be good to get another perspective as well:

what do you think about the issue? I like having a proper enum to be used in the graphql query, but knowing that GQL unfortunately only supports upper snake case values like @oana-lolea mentioned, could it be confusing if people try to use the generated enum value when handing the webhook events in the webhook service, since it won't match up?

@BlairCurrey
Copy link
Contributor

BlairCurrey commented Oct 11, 2024

could it be confusing if people try to use the generated enum value when handing the webhook events in the webhook service, since it won't match up?

I think it would be problematic. We couldn't use them for identifying which type of webhook was received (ie receviedWebhookEvent.type === WebhookEventType.INCOMING_PAYMENT_CREATED). The client would need its own definition or map for the event types. And documentation-wise it's close but not fully correct which could be confusing.

What about a custom scalar?

const WebhookEventType = new GraphQLScalarType({
  name: 'WebhookEventType',
  description: 'Webhook event type format (e.g., incoming_payment.created)',
  serialize(value) {
    return value;
  },
  parseValue(value) {
    const allowedTypes = [
      'incoming_payment.created',
      'outgoing_payment.completed',
      // Add other valid event types
    ];
    if (!allowedTypes.includes(value)) {
      throw new Error('Invalid WebhookEventType');
    }
    return value;
  }
});

I guess this might not be clear on the auto-generated documentation side though (it will just show WebhookEventType). But in terms of behavior of the GQL api it seems like what we want.

@mkurapov
Copy link
Contributor

From the meeting:

Let's try to update the WebhookEventFilter to take in an array of WebhookEvent scalar values. If it breaks the frontend webhooks page out of the box, we can just close the issue with a plan to standardize all of our enums (like #3012) in the future.

Otherwise, we can keep the scalar.

@oana-lolea
Copy link
Contributor Author

@mkurapov The scalar cannot be used for the filter because it can only be used within the GraphQL schema. We could however use its parser to validate the filter string values.

@mkurapov
Copy link
Contributor

@oana-lolea ah, I see. Thanks for taking a look into this. Since we can't use it as part of the filter (which would be useful for automatically validating user/client input), maybe we can just close this issue as not planned, and standardize all our enums separately in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: mock-account-service-lib pkg: mock-ase type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Webhook Event types an enum in the Admin API
3 participants