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(api): notification event payload #1239

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Conversation

chrisgacsal
Copy link
Contributor

Overview

Add notification event type playload to the OepnAPI specification. Currently only payload for notifications with entitlements.balance.threshold type is supported.

@chrisgacsal chrisgacsal self-assigned this Jul 23, 2024
@chrisgacsal chrisgacsal requested review from tothandras and turip July 23, 2024 15:23
@chrisgacsal chrisgacsal added kind/feature New feature or request area/api release-note/misc Miscellaneous changes labels Jul 23, 2024
@chrisgacsal chrisgacsal force-pushed the notifications-api-spec branch from 29e9c3a to a8c62d3 Compare July 23, 2024 15:32
@chrisgacsal chrisgacsal requested a review from GAlexIHU July 23, 2024 15:32
$ref: "#/components/schemas/Feature"
subject:
$ref: "#/components/schemas/Subject"
balance:
Copy link
Contributor

@tothandras tothandras Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a balance type, EntitlementValue?

Copy link
Contributor Author

@chrisgacsal chrisgacsal Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was kinda hasitant to use EntitlementValue directly as it is shared across all the entitlement types meaning it has fields for all teh entitlement types while we only need metered here. So it might be confusing for users to see optinal fields in the spec which will be never used for entitlements.balance.threshold payload.

It would be awesome to have entitlement specific Entitlement[Metered|Static|Boolean]Value resources in the spec to support better reusability, but making this type of change out of scope for this PR.

So I am ok with using EntitlementValue here, but I'd improve on this at some point to make the API spec more clean.

@GAlexIHU

Add type specification for notification event payload instead of keeping
it as a free form object.
@chrisgacsal chrisgacsal force-pushed the notifications-api-spec branch from a8c62d3 to 05dde5b Compare July 24, 2024 08:25
@chrisgacsal chrisgacsal requested a review from tothandras July 24, 2024 08:25
@chrisgacsal chrisgacsal merged commit aa58c64 into main Jul 24, 2024
19 checks passed
@chrisgacsal chrisgacsal deleted the notifications-api-spec branch July 24, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api kind/feature New feature or request release-note/misc Miscellaneous changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants