-
Notifications
You must be signed in to change notification settings - Fork 2
Feature | Extend Swagger Coverage for controller OAuth2SummitTaxTypeApiController
#394
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
Conversation
5103597 to
7d24b01
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 Doc generation is broken here like in pr 393.
Please verify existence of, or create
new OA\Parameter(ref: '#/components/parameters/page'),
new OA\Parameter(ref: '#/components/parameters/per_page'),
Also verify 'OAuth2' sec schema.
For 'SummitTaxType' look at the 'ticket_types' property.
Without relations=ticket_types: Field not present
With relations=ticket_types: Returns array of integers (IDs): [1, 2, 3]
With expand=ticket_types: Returns array of full objects
Should probably be something like:
new OA\Property(
property: 'ticket_types',
type: 'array',
nullable: true,
items: new OA\Items(type: 'integer'),
example: [1, 2, 3],
description: 'Array of ticket type IDs (only present when relations=ticket_types). When expanded with expand=ticket_types, contains full ticket type objects instead of IDs.'
),
|
@caseylocker regarding the mentioned field this is how it was handled: new OA\Property(
property: 'ticket_types',
type: 'array',
items: new OA\Items(oneOf: [
new OA\Schema(type: 'integer'),
new OA\Schema(type: 'SummitTicketType')
]),
example: [1, 2, 3],
description: 'Array of ticket type IDs or its Model (only present when relations=ticket_types), expanded when expand includes ticket_types.'
), |
|
@caseylocker The security schema for the controller was moved to its own file |
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
TaxTypesAuthSchema.php needs the namespace updated.
// Current (WRONG)
namespace App\Swagger\Security;
// Required (CORRECT)
namespace App\Swagger\schemas;
The epoch timestamp seems incorrect and will cause issues.
// Invalid - missing type attribute
new OA\Property(property: 'sales_start_date:datetime_epoch'),
new OA\Property(property: 'sales_end_date:datetime_epoch'),
// Correct
new OA\Property(property: 'sales_start_date', type: 'integer', format: 'int64', description: 'Epoch timestamp'),
new OA\Property(property: 'sales_end_date', type: 'integer', format: 'int64', description: 'Epoch timestamp'),
Missing operandIds. Examples below:
| Endpoint | Suggested operationId |
|---|---|
| GET /tax-types | getAllTaxTypes |
| POST /tax-types | createTaxType |
| GET /tax-types/{tax_id} | getTaxType |
| PUT /tax-types/{tax_id} | updateTaxType |
| DELETE /tax-types/{tax_id} | deleteTaxType |
| PUT .../ticket-types/{ticket_type_id} | addTaxToTicketType |
| DELETE .../ticket-types/{ticket_type_id} | removeTaxFromTicketType |
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.
The issue with ticket_types in app/Swagger/Models/SummitTaxTypeSchema.php is that "new OA\Schema(type: 'SummitTicketType')" is an invalid type reference.
Since we have the schema for ticket types you can use:
new OA\Property(
property: 'ticket_types',
type: 'array',
items: new OA\Items(oneOf: [
new OA\Schema(type: 'integer'),
new OA\Schema(ref: '#/components/schemas/SummitTicketType')
]),
example: [1, 2, 3],
description: 'Array of ticket type IDs or its Model (only present when relations=ticket_types), expanded when expand includes ticket_types.'
),
2c3a71b to
c56d65b
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 the conflicts many thanks |
c6ecdd0 to
728ae67
Compare
…nActionTypeApiController`
5b83f3e to
a134fa4
Compare
|
@smarcet now it is rebased! |
smarcet
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
Task:
Ref: https://app.clickup.com/t/86b6wkh40