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

chore(oas): replace response with $ref class JSDoc (Admin O-PRI) #3018

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

patrick-medusajs
Copy link
Contributor

Scope

Admin routes directories O to PRI.

What

Move inline OAS response schema declaration under their respective class declarations in order to expose them through #/components/schemas. Replace inline OAS response schema with a $ref reference pointing to the newly declared schema.

Why

Having response declared as its own "named" schema will allow OAS code generators to output typed entities/DTO that can be consumed without having to reference the route/operation.

How

Declare a new @Schema JSDoc for each "Res" class used to parse and validate request body. Move the current inline requestBody to the new @Schema.

Test

  • Ran OAS validator.
  • Ran docs build script.

Expect no visible changes to the documentation.

@patrick-medusajs patrick-medusajs self-assigned this Jan 12, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jan 12, 2023

⚠️ No Changeset found

Latest commit: 179ddac

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor Author

@patrick-medusajs patrick-medusajs left a comment

Choose a reason for hiding this comment

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

self-review: highlighting noteworthy code changes.

* object:
* type: string
* description: The type of the object that was deleted.
* default: item_change
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced format with default for consistency's sake.

* object:
* type: string
* description: The type of the object that was deleted.
* default: order_edit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced format with default for consistency's sake.

export type AdminOrdersRes = {
order: Order
}

export type AdminDeleteRes = DeleteResponse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too generic, plus could not find any usage in the codebase.

* type: integer
* description: The number of items per page
*/
export type AdminPriceListsProductsListRes = PaginatedResponse & {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing declaration for admin/price-lists/list-price-list-products.ts.
JS client has response typed as any. See file.

@patrick-medusajs patrick-medusajs marked this pull request as ready for review January 12, 2023 18:50
@patrick-medusajs patrick-medusajs requested a review from a team as a code owner January 12, 2023 18:50
@patrick-medusajs patrick-medusajs requested review from shahednasser and a team January 12, 2023 18:50
Copy link
Member

@shahednasser shahednasser left a comment

Choose a reason for hiding this comment

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

Just some small comments 🙂

* properties:
* order_edits:
* type: array
* $ref: "#/components/schemas/OrderEdit"
Copy link
Member

Choose a reason for hiding this comment

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

The original OAS for this one seems to be incorrect. This should be under the items property since order_edits is an array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed in 5d88100

* type: boolean
* description: Whether or not the items were deleted.
* default: true
*/
export type AdminPriceListDeleteBatchRes = {
ids: string[]
deleted: boolean
object: string
Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to OAS and isn't necessarily something that should be looked into in this PR: seems like there is an inconsistency across different files in how this type of response is defined. In some places, the default value is set within the type definition (for example, object: 'payment_collection') whereas in other places like here the type is generic. Maybe this is something to take into account if we want to automatically generate OAS comments in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I believe that latest approach is to declare object: as a constant. The way to do it in OAS would be to declare object as the following:

object:
  - type: string
  - description: The type of the object that was deleted.
  - enum: [money-amount]
  - default: money-amount

Copy link
Member

@shahednasser shahednasser left a comment

Choose a reason for hiding this comment

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

LGTM!

@kodiakhq kodiakhq bot merged commit cdcbc06 into develop Jan 13, 2023
@kodiakhq kodiakhq bot deleted the chore/oas-response-schema-OtoPRI branch January 13, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants