-
Notifications
You must be signed in to change notification settings - Fork 2
Feature | Extend Swagger Coverage for controller OAuth2SummitMediaUploadTypeApiController
#398
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: main
Are you sure you want to change the base?
Conversation
a6557c2 to
a38ef4e
Compare
af04bd8 to
3eb31b7
Compare
caseylocker
left a comment
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.
@matiasperrone-exo Parameter mismatch on all 8 endpoints.
// Path uses {id}
path: "/api/v1/summits/{id}/media-upload-types"
// But parameter says 'summit_id' in 'query' - WRONG!
new OA\Parameter(
name: 'summit_id', // Should be 'id'
in: 'query', // Should be 'path'
required: false, // Should be true
...
),
// Correct definition:
new OA\Parameter(
name: 'id',
in: 'path',
required: true,
description: 'Summit ID',
schema: new OA\Schema(type: 'integer')
),
Add operanIds to all the endpoint definitions
| Endpoint | Suggested operationId |
|---|---|
| GET /media-upload-types | getAllMediaUploadTypes |
| GET /media-upload-types/{id} | getMediaUploadType |
| POST /media-upload-types | createMediaUploadType |
| PUT /media-upload-types/{id} | updateMediaUploadType |
| DELETE /media-upload-types/{id} | deleteMediaUploadType |
| PUT .../presentation-types/{id} | addMediaUploadTypeToPresentationType |
| DELETE .../presentation-types/{id} | removeMediaUploadTypeFromPresentationType |
| POST .../all/clone/{to_summit_id} | cloneMediaUploadTypes |
caseylocker
left a comment
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 may not have been clear enough regarding the 'id' in 'query' being wrong and that it's actually in the 'path' for all 8 endpoints.
Example:
new OA\Parameter(
name: 'id',
in: 'query', // WRONG - should be 'path'
required: false, // WRONG - path params must be required: true
schema: new OA\Schema(type: 'integer'),
description: 'The summit ID'
),
Should be:
new OA\Parameter(
name: 'id',
in: 'path', // Correct
required: true, // Path parameters MUST be required
schema: new OA\Schema(type: 'integer'),
description: 'The summit ID'
),
Affected lines:
Lines 94-100 (getAllMediaUploadTypes)
Lines 187-193 (getMediaUploadType)
Lines 253-259 (createMediaUploadType)
Lines 304-310 (updateMediaUploadType)
Lines 362-368 (deleteMediaUploadType)
Lines 531-537 (addMediaUploadTypeToPresentationType)
Lines 610-616 (removeMediaUploadTypeFromPresentationType)
Lines 688-694 (cloneMediaUploadTypes)
69ff1e9 to
3a57467
Compare
|
Thanks @caseylocker for the comments. Now is ready to review again |
32fa05f to
7dd6e2d
Compare
caseylocker
left a comment
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.
Approved
|
@matiasperrone please rebase against main and fix conflicts many thanks |
c6ecdd0 to
728ae67
Compare
…TypeApiController`
7dd6e2d to
cb17dad
Compare
Task:
Ref: https://app.clickup.com/t/86b6wkh5e