Skip to content
This repository was archived by the owner on Jun 24, 2024. It is now read-only.
2 changes: 1 addition & 1 deletion app/controllers/api/assignment_repos_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class AssignmentReposController < API::ApplicationController

def index
repos = AssignmentRepo.where(assignment: @assignment).order(:id)
paginate json: repos
paginate json: repos, roster: @assignment.organization.roster

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a test which covers this pagination change? Will it be surprising to folks when deployed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The roster kwarg here doesn't add anything to the JSON payload, it adds context that I can access in the serializer via instance_options. The alternative is to do something like repo.assignment.organization.roster.roster_entries.find... for every repo in the serializer 😬

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the @assignment.organization always the same as the @organization here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, didn't see @organization, changed it to use that instead.

end

def clone_url
Expand Down
6 changes: 6 additions & 0 deletions app/serializers/assignment_repo_serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ class AssignmentRepoSerializer < ActiveModel::Serializer
attributes :id
attributes :username
attributes :displayName
attributes :rosterIdentifier

def username
object.user.github_user.login
Expand All @@ -13,5 +14,10 @@ def username
def displayName
object.user.github_user.name || ""
end

def rosterIdentifier
return nil unless instance_options[:roster]
instance_options[:roster].roster_entries.find_by(user_id: object.user.id)&.identifier

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this an N+1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The results of the roster.roster_entries query would probably be cached, but I changed this to pass in roster entries instead of roster so we don't need to make this query every time.

end
# rubocop:enable MethodName
end
80 changes: 61 additions & 19 deletions spec/controllers/api/assignment_repos_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,34 +5,76 @@
RSpec.describe API::AssignmentReposController, type: :controller do
let(:organization) { classroom_org }
let(:user) { classroom_teacher }
let(:second_user) { classroom_student }
let(:assignment) { create(:assignment, organization: organization, title: "Learn Clojure") }

describe "GET #index", :vcr do
before do
@assignment_repo = create(:assignment_repo, assignment: assignment, github_repo_id: 42, user: user)
context "with a roster" do
before do
create(:assignment_repo, assignment: assignment, github_repo_id: 42, user: user)
create(:assignment_repo, assignment: assignment, github_repo_id: 43, user: second_user)

get :index, params: {
organization_id: organization.slug,
assignment_id: assignment.slug,
access_token: user.api_token
}
end
roster = create(:roster)
@entry = create(:roster_entry, roster: roster, identifier: "entryA", user: user)

after do
AssignmentRepo.destroy_all
end
organization.roster = roster
organization.save

it "returns success" do
expect(response).to have_http_status(200)
end
get :index, params: {
organization_id: organization.slug,
assignment_id: assignment.slug,
access_token: user.api_token
}
end

after do
AssignmentRepo.destroy_all
end

it "returns success" do
expect(response).to have_http_status(200)
end

it "returns all of the assignment repos" do
expect(json.length).to eql(1)
it "returns all of the assignment repos" do
expect(json.length).to eql(2)
end

it "returns correct attributes in assignment repo serializer" do
expect(json.first["username"]).to eq(user.github_user.login)
expect(json.first["displayName"]).to eq(user.github_user.name)
expect(json.first["rosterIdentifier"]).to eq(@entry.identifier)
end
end

it "returns correct attributes in assignment repo serializer" do
expect(json.first["username"]).to eq(user.github_user.login)
expect(json.first["displayName"]).to eq(user.github_user.name)
context "without a roster" do
before do
create(:assignment_repo, assignment: assignment, github_repo_id: 42, user: user)
create(:assignment_repo, assignment: assignment, github_repo_id: 43, user: second_user)

get :index, params: {
organization_id: organization.slug,
assignment_id: assignment.slug,
access_token: user.api_token
}
end

after do
AssignmentRepo.destroy_all
end

it "returns success" do
expect(response).to have_http_status(200)
end

it "returns all of the assignment repos" do
expect(json.length).to eql(2)
end

it "returns correct attributes in assignment repo serializer" do
expect(json.first["username"]).to eq(user.github_user.login)
expect(json.first["displayName"]).to eq(user.github_user.name)
expect(json.first["rosterIdentifier"]).to be_nil
end
end
end

Expand Down

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.