Skip to content
This repository was archived by the owner on Jun 24, 2024. It is now read-only.

Fetch GitHub Team name for Classroom Assistant #1727

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fetch GitHub Team name for Classroom Assistant #1727

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
62d3bd5
fetch team name from github
srinjoym Jan 2, 2019
27ea3bc
Merge branch 'master' into classroom-assistant-github-team-name
srinjoym Jan 2, 2019
890ac59
Merge branch 'master' into classroom-assistant-github-team-name
srinjoym Jun 3, 2019
daf0a09
Merge branch 'master' into classroom-assistant-github-team-name
JessRudder Sep 19, 2019
e15add3
Merge branch 'master' into classroom-assistant-github-team-name
srinjoym Sep 24, 2019
8be229a
Merge branch 'master' into classroom-assistant-github-team-name
jeffrafter Oct 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/serializers/group_assignment_repo_serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class GroupAssignmentRepoSerializer < ActiveModel::Serializer
attributes :displayName

def username
object.group.title
object.group.github_team.name
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand the implications of fetching from the API here; If I am understanding correctly we fetch from the API every time we access the github_* properties. This means every time we serialize a GroupAssignmentRepo to JSON we will hit the github API. The main place this feels problematic is here: https://github.com/education/classroom/blob/8be229a98e7a56b808e977472c46d48b2d62c2ce/app/controllers/api/group_assignment_repos_controller.rb#L11-L12

This introduces and github API N+1 if I am reading this right. I'm not clear if that is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on our setup, I think OctoKit would cache these at the client level?

Copy link
Contributor

Choose a reason for hiding this comment

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

Octokit should cache them, correct

end

# rubocop:disable MethodName
Expand Down