-
Notifications
You must be signed in to change notification settings - Fork 568
Fetch GitHub Team name for Classroom Assistant #1727
base: master
Are you sure you want to change the base?
Conversation
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
@srinjoym Is this still WIP? Should we close it? |
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.
Small clarification but I love how simple this is ❇️
@@ -6,7 +6,7 @@ class GroupAssignmentRepoSerializer < ActiveModel::Serializer | |||
attributes :displayName | |||
|
|||
def username | |||
object.group.title | |||
object.group.github_team.name |
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.
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.
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.
Depending on our setup, I think OctoKit would cache these at the client level?
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.
Octokit should cache them, correct
Fetch the team name from GitHub everytime we serialize a group assignment repo. This fixes github-education-resources/classroom-assistant#117