Include role in owners api response#5821
Conversation
|
Hello, you're right there is no pattern established to compose payload so it is in-lined usually. rubygems.org/app/controllers/api/v1/activities_controller.rb Lines 14 to 17 in 9bcbf60 rubygems.org/app/controllers/api/v2/contents_controller.rb Lines 30 to 34 in 9bcbf60 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5821 +/- ##
==========================================
- Coverage 38.67% 38.03% -0.64%
==========================================
Files 486 486
Lines 10698 10760 +62
==========================================
- Hits 4137 4093 -44
- Misses 6561 6667 +106 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| rescue_from ActiveRecord::RecordNotFound, with: :render_not_found | ||
|
|
||
| def show | ||
| payload = @rubygem.owners.map { |owner| owner.payload.merge("role" => owner.ownerships.find_by(rubygem: @rubygem).role) } |
There was a problem hiding this comment.
Would be good to make this more SQL effective. Now it seems to trigger additional query per each owner.
There was a problem hiding this comment.
Thanks @simi
I believe my latest commit reduces it back down to one query
|
Is the codecov decrease a blocker for this? I was looking into solving ruby/rubygems#9267 which this PR depends on. |
There was a problem hiding this comment.
Pull request overview
Updates the v1 owners endpoint to include each gem member’s ownership role in the /api/v1/gems/:gem_id/owners.(json|yaml) response, addressing #5816.
Changes:
- Modify
Api::V1::OwnersController#showto render a custom owners payload includingrole. - Add an
owners_payloadhelper to assemble the response data. - Add a functional test asserting
roleis present in the JSON response.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/controllers/api/v1/owners_controller.rb | Switches owners response rendering to a custom payload that includes role. |
| test/functional/api/v1/owners_controller_test.rb | Adds a JSON test asserting returned owner fields now include role. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| context "GET /api/v1/gems/:gem/owners.json" do | ||
| setup do | ||
| @user = create(:user, public_email: true) | ||
| @rubygem = create(:rubygem, owners: [@user]) | ||
| get :show, params: { rubygem_id: @rubygem.slug }, format: :json | ||
| @user_payload = JSON.parse(@response.body).first | ||
| end | ||
| should "include associated users id, handle, email, and role in response" do | ||
| assert_equal "owner", @user_payload["role"] | ||
| assert_equal @user.id, @user_payload["id"] | ||
| assert_equal @user.handle, @user_payload["handle"] | ||
| assert_equal @user.email, @user_payload["email"] | ||
| end | ||
| end |
There was a problem hiding this comment.
The new test only asserts the default "owner" role in the JSON response. Since this PR changes the API contract to include roles, it should also cover at least one non-default role (e.g., a maintainer) and ideally exercise the YAML format too, so regressions in role selection/serialization are caught.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
4 participants
Addresses #5816 by including the role of each member of a gem in the response to /api/v1/gems/:gem_id/owners.json
Note: There could be a better solution to this, but I found it difficult to work around the User#payload method. It looks like using serializers was discussed in the past but rejected. #1027 (comment) As this is my first PR here, I am absolutely open to feedback or better approaches.