-
Notifications
You must be signed in to change notification settings - Fork 2
Feature: Extend Swagger Coverage for controller OAuth2SummitsEventTypesApiController #425
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
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitsEventTypesApiController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitsEventTypesApiController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitsEventTypesApiController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitsEventTypesApiController.php
Outdated
Show resolved
Hide resolved
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.
@andrestejerina97 please review comments
fa5e8a1 to
5842488
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 The POST in app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitsEventTypesApiController.php should be a PUT based on the route.
Incorrect
#[OA\Post(
path: "/api/v1/summits/{id}/event-types/{event_type_id}/summit-documents/{document_id}",
operationId: "addSummitDocument",
Correct
#[OA\Put(
path: "/api/v1/summits/{id}/event-types/{event_type_id}/summit-documents/{document_id}",
operationId: "addSummitDocument",
Is app/Swagger/Models/SummitSchema.php complete?
There is a TODO comment in there.
5842488 to
65bed13
Compare
|
@caseylocker the Summit schema is defined in the Summit controller. Please approve. |
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.
getEventTypeBySummit is documented as public but the route, /api/v1/summits/{id}/event-types/{event_type_id}, requires OAuth2 authentication. Based on the scope of the PR add proper security parameter and change the tag.
security: [['summit_event_types_oauth2' => [
SummitScopes::ReadSummitData,
SummitScopes::ReadAllSummitData,
]]],
tags: ["Event Types"],
Thanks @caseylocker for the comments. Now is ready to review again |
1b361ec to
0d1777a
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.
PUT operations document HTTP 200 but updated() actually returns a 201.
updateEventTypeBySummit, addSummitDocument, removeSummitDocument Change Response::HTTP_OK to Response::HTTP_CREATED
Missing security attribute on protected GET endpoint in getEventTypeBySummit.
Thanks for adding the public endpoint but the protected one needs documentation of the security attribute.
|
Thanks @caseylocker for the comments. Good catch! Is ready now. I fixed the issues in security and X: Summary of corrections in OAuth2SummitsEventTypesApiController
|
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
ref: https://app.clickup.com/t/86b6wkha7