-
Notifications
You must be signed in to change notification settings - Fork 2
Feature | Extend Swagger Coverage for controller OAuth2SummitMetricsApiController
#414
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
Feature | Extend Swagger Coverage for controller OAuth2SummitMetricsApiController
#414
Conversation
7e87cef to
14414cc
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.
Wrong namespace in security schema:
// Current (line 3)
namespace App\Swagger\Security;
// Should be
namespace App\Swagger\schemas;
Wrong x: parameter key (3 times):
x: [
"authz_groups" => [IGroup::SummitAccessControl]
],
// Should be
x: [
"required-groups" => [IGroup::SummitAccessControl]
],
Wrong paths for member schedule metrics endpoints: Per routes/api_v1.php, the actual routes are:
PUT {summit_id}/members/{member_id}/schedule/{event_id}/enter
POST {summit_id}/members/{member_id}/schedule/{event_id}/leave
// Current (line 214) - extra "/metrics/"
path: "/api/v1/summits/{id}/members/{member_id}/schedule/{event_id}/metrics/enter"
// Should be
path: "/api/v1/summits/{id}/members/{member_id}/schedule/{event_id}/enter"
// Current (line 285) - extra "/metrics/"
path: "/api/v1/summits/{id}/members/{member_id}/schedule/{event_id}/metrics/leave"
// Should be
path: "/api/v1/summits/{id}/members/{member_id}/schedule/{event_id}/leave"
…schema, fix path routes and change schema to be defined as requested
cfafaf8 to
d09e4a5
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.
LGTM @smarcet if you agree please merge
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/86b6wkh8n