Add roster identifier to assignment repos endpoint #2439
Conversation
ghost
requested a review
from 
jeffrafter
left a comment
jeffrafter
left a comment
There was a problem hiding this comment.
Some small comments... thanks!
|
|
||
| def rosterIdentifier | ||
| return nil unless instance_options[:roster] | ||
| instance_options[:roster].roster_entries.find_by(user_id: object.user.id)&.identifier |
There was a problem hiding this comment.
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.
| def index | ||
| repos = AssignmentRepo.where(assignment: @assignment).order(:id) | ||
| paginate json: repos | ||
| paginate json: repos, roster: @assignment.organization.roster |
There was a problem hiding this comment.
Is there a test which covers this pagination change? Will it be surprising to folks when deployed?
There was a problem hiding this comment.
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 😬
| def index | ||
| repos = AssignmentRepo.where(assignment: @assignment).order(:id) | ||
| paginate json: repos | ||
| paginate json: repos, roster: @assignment.organization.roster |
There was a problem hiding this comment.
Is the @assignment.organization always the same as the @organization here?
There was a problem hiding this comment.
Yep, didn't see @organization, changed it to use that instead.
…b.com:education/classroom into retry_add_roster_entry_to_assignment_repo_api
|
@jeffrafter All comments addressed, this is ready for review again! |
…b.com:education/classroom into retry_add_roster_entry_to_assignment_repo_api
#2404 had to be reverted as it caused exceptions when a user had a roster + assignment repos by a user who wasn't on the roster. The fix is just conditionally grabbing the identifier off of the roster entry which may or may not exist. I broke tests by adding a second assignment repo by a user not on the roster, as seen here: https://travis-ci.org/education/classroom/jobs/600831209#L2145
55c57fb fixes it.