-
Notifications
You must be signed in to change notification settings - Fork 10
DEVORTEX-5439 add webhooks api #125
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
base: master
Are you sure you want to change the base?
Conversation
interface SubscriptionSecretDto { | ||
payloadSecret: string; | ||
} | ||
|
||
export { SubscriptionSecretDto }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no actions points: weird naming. it's already clear this secret is in payload.
import { CreateUpdateSubscriptionParameters } from "./create-update-subscription-parameters"; | ||
import { SubscriptionEvent } from "./subscription-event"; | ||
|
||
export class UpdateSubscriptionParameters extends CreateUpdateSubscriptionParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename CreateUpdateSubscriptionParameters
to CreateSubscriptionParameters
since you have separate class for updating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UpdateSubscriptionParameters doesn’t support all methods available in CreateSubscriptionParameters, so we can't inherit one from the other directly.
Instead, both CreateSubscriptionParameters and UpdateSubscriptionParameters extend a common base class.
getSubscriptions( | ||
accountUid: string | ||
): Promise<SmartlingListResponse<SubscriptionDto>> { | ||
return this.makeRequest( | ||
"get", | ||
`${this.entrypoint}/accounts/${accountUid}/subscriptions` | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this endpoint support pagination?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the endpoint returns the full list of subscriptions, but it's capped to a small fixed number.
enableSubscription( | ||
accountUid: string, | ||
subscriptionUid: string | ||
): Promise<void> { | ||
return this.makeRequest( | ||
"post", | ||
`${this.entrypoint}/accounts/${accountUid}/subscriptions/${subscriptionUid}/enable` | ||
); | ||
} | ||
|
||
disableSubscription( | ||
accountUid: string, | ||
subscriptionUid: string | ||
): Promise<void> { | ||
return this.makeRequest( | ||
"post", | ||
`${this.entrypoint}/accounts/${accountUid}/subscriptions/${subscriptionUid}/disable` | ||
); | ||
} | ||
|
||
testSubscription( | ||
accountUid: string, | ||
subscriptionUid: string | ||
): Promise<void> { | ||
return this.makeRequest( | ||
"post", | ||
`${this.entrypoint}/accounts/${accountUid}/subscriptions/${subscriptionUid}/test` | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember initial arch review but have you guys arranged API review? These verbs in endpoints look strange (test/enable/disable/etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dnetrebenko-smartling could you please answer?
No description provided.