-
Notifications
You must be signed in to change notification settings - Fork 2
Feature | Extend Swagger Coverage for controller OAuth2SummitTicketApiController
#440
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?
Feature | Extend Swagger Coverage for controller OAuth2SummitTicketApiController
#440
Conversation
08fb819 to
acaa457
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.
In app/Swagger/SummitRegistrationSchemas.php
items: new OA\Items(type: 'SummitAttendeeTicket')
Is an invalid type.
In app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitTicketApiController.php
tags: ['tickets'],
Should be title cased
tags: ['Tickets'],
|
Thanks @caseylocker for the comments. Now is ready to review again |
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.
This was not addressed:
"In app/Swagger/SummitRegistrationSchemas.php
items: new OA\Items(type: 'SummitAttendeeTicket')
Is an invalid type."
c6ecdd0 to
728ae67
Compare
f41ca9b to
01db7d5
Compare
|
Thanks @caseylocker for the comments. Now is ready to review again |
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.
SummitRegistrationSchemas.php line 70 Schema reference error:
items: new OA\Items(type: 'SummitAttendeeTicket') // WRONG
Should be:
items: new OA\Items(ref: '#/components/schemas/SummitAttendeeTicket') // CORRECT
OAuth2SummitTicketApiController.php line 1125 Invalid schema syntax
// Current (incorrect):
content: new OA\JsonContent(schema: 'SummitAttendeeBadge')
// Should be:
content: new OA\JsonContent(ref: '#/components/schemas/SummitAttendeeBadge')
OA\JsonContent doesn't have a schema parameter for referencing other schemas. Use ref instead.
01db7d5 to
1c786ab
Compare
|
Thanks @caseylocker for the comments. Now is ready to review again. |
132d5fd to
54b98ac
Compare
romanetar
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 two checks are failing, please review
74b0a50 to
7a734db
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.
I'm seeing usage of 'numeric' for type in several places. 'numeric' is a Laravel validation rule, not a valid OpenAPI type. Please use a valid OpenAPI type like 'integer' or 'number' for floating point values like 'final_amount'.
|
Thanks @caseylocker for the comments. Now is ready to review again |
9c7424f to
8208cd5
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.
'float' isn't a type, it's a format. If this is a floating point number then the syntax should be:
new OA\Property(property: 'raw_cost', type: 'number', format: 'float'),
Please address the other instances of 'float' as well.
Properties to fix (lines 25, 26, 28, 30, 31, 33, 35, 39):
- raw_cost
- net_selling_cost
- final_amount
- discount
- discount_rate
- refunded_amount
- total_refunded_amount
- taxes_amount
OAuth2SummitTicketApiControllerOAuth2SummitTicketApiController
|
@caseylocker all the requested changes were introduced, you may review it again.
|
6395cd5 to
9ede57c
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.
LGTM
romanetar
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 please review
| #[OA\Get( | ||
| path: '/api/v1/summits/{id}/tickets/csv', | ||
| operationId: 'getAllTicketsCSV', | ||
| summary: 'Get all tickets for a summit', |
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 summary and description must reflect an export, not a list
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.
good catch @romanetar
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.
@romanetar thank you. The description was changed and it was ready along with a new rebase
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
Signed-off-by: Matias Perrone <[email protected]>
9ede57c to
e8a8e1b
Compare
Task:
Ref: https://app.clickup.com/t/86b6wkhce
Dependencies:
#436 #450