Skip to content

feat(oas): identify required fields in responses - admin #3278

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

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

patrick-medusajs
Copy link
Contributor

@patrick-medusajs patrick-medusajs commented Feb 16, 2023

Scope

Admin routes

What

Add OAS required to response schemas.

Why

Code generator can use the required property of a schema to mark fields as optional or not when generating TS types.

Test

  • Run yarn build
  • Run yarn openapi:generate
  • Run yarn redocly preview-docs docs/api/admin/openapi.yaml --config=docs-util/redocly/config.yaml
  • Open the documentation preview URL in a browser (http://127.0.0.1:8080)
  • Expect responses to have their fields declared as required

@patrick-medusajs patrick-medusajs self-assigned this Feb 16, 2023
@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2023

🦋 Changeset detected

Latest commit: 454621a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/medusa Patch
@medusajs/inventory Patch

Not sure what this means? Click here to learn what changesets are.

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

@patrick-medusajs patrick-medusajs marked this pull request as ready for review February 16, 2023 16:42
@patrick-medusajs patrick-medusajs requested a review from a team as a code owner February 16, 2023 16:42
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

Super nice work @patrick-medusajs!

Tested locally ✅

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.

I noticed missing required fields for these responses, but I'm not sure if there's a reason behind it:

  • AdminCollectionsDeleteRes
  • AdminDeleteProductsFromCollectionRes
  • AdminCustomerGroupsListRes

Comment on lines +58 to +60
* required:
* - shipping_options
* - count
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also have here offset and limit but they're also missing from the response properties.

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 5c3615b
I also added the missing required on the other schemas.

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 48ad242 into develop Feb 17, 2023
@kodiakhq kodiakhq bot deleted the feat/oas-response-required-admin branch February 17, 2023 14:19
adrien2p pushed a commit that referenced this pull request Feb 20, 2023
## Scope

Admin routes

## What

Add OAS `required` to response schemas.

## Why

Code generator can use the `required` property of a schema to mark fields as optional or not when generating TS types.

## Test

* Run `yarn build`
* Run `yarn openapi:generate`
* Run `yarn redocly preview-docs docs/api/admin/openapi.yaml --config=docs-util/redocly/config.yaml`
* Open the documentation preview URL in a browser (http://127.0.0.1:8080)
* Expect responses to have their fields declared as `required`
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.

3 participants