Skip to content

Conversation

@andrestejerina97
Copy link
Contributor

@andrestejerina97 andrestejerina97 self-assigned this Oct 31, 2025
Copy link
Collaborator

@smarcet smarcet left a 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

@andrestejerina97 andrestejerina97 force-pushed the feature/add-openapi-documentation-to-controller-oauthsummiteventtypecontroller branch from fa5e8a1 to 5842488 Compare November 20, 2025 16:49
Copy link

@caseylocker caseylocker left a 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.

@matiasperrone-exo matiasperrone-exo force-pushed the feature/add-openapi-documentation-to-controller-oauthsummiteventtypecontroller branch from 5842488 to 65bed13 Compare December 5, 2025 15:44
@matiasperrone-exo
Copy link
Contributor

@caseylocker the Summit schema is defined in the Summit controller. Please approve.

Copy link

@caseylocker caseylocker left a 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"], 

@matiasperrone-exo
Copy link
Contributor

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.
Both protected and public are pointing to the same method, I added the missing public endpoint to have every endpoint documented.

Now is ready to review again

@matiasperrone-exo matiasperrone-exo force-pushed the feature/add-openapi-documentation-to-controller-oauthsummiteventtypecontroller branch from 1b361ec to 0d1777a Compare December 10, 2025 22:36
Copy link

@caseylocker caseylocker left a 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.

@matiasperrone-exo
Copy link
Contributor

Thanks @caseylocker for the comments. Good catch! Is ready now.

I fixed the issues in security and X:

Summary of corrections in OAuth2SummitsEventTypesApiController

Method Change made
getEventTypeBySummit Added security with ReadSummitData, ReadAllSummitData
addEventTypeBySummit Fixed scope order (WriteEventTypeData, WriteSummitData) and required-groups order
updateEventTypeBySummit Fixed scope order and required-groups order
deleteEventTypeBySummit Fixed scope order and required-groups order
seedDefaultEventTypesBySummit Fixed scope order and required-groups order
addSummitDocument Fixed scope order and required-groups order
removeSummitDocument Fixed scope order and required-groups order

Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

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

approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants