Allow a GitHub Organization to be used by multiple Classrooms #1248
Conversation
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.
All good on my side ✨
Just a quick question, if two classrooms have each an assignment "First lesson" for example and a student is in both classroom, they would be no way to say which one's from which classroom except the slug at the end ? (or this case is too tricky to happen)
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.
Nothing in here seems too wild to me 💃
Let me know when it's 👍 and I can pull it down as well.
ab20696
to
70808ed
Compare
… slug, also assign webhook from github or existing classroom
spec/models/organization_spec.rb
Outdated
|
||
context "last classroom on organization" do | ||
it "deletes the webhook from GitHub" do | ||
subject.update_attributes(webhook_id: 9_999_999, is_webhook_active: true) |
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.
Rails/ActiveRecordAliases: Use update instead of update_attributes.
spec/models/organization_spec.rb
Outdated
end | ||
|
||
it "does not delete the webhook from GitHub" do | ||
subject.update_attributes(webhook_id: 9_999_999, is_webhook_active: true) |
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.
Rails/ActiveRecordAliases: Use update instead of update_attributes.
app/models/organization/creator.rb
Outdated
|
||
# There should only be one webhook that Classroom creates in production | ||
webhooks.first.id | ||
rescue |
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.
Style/RescueStandardError: Avoid rescuing without specifying an error class.
app/models/organization/creator.rb
Outdated
@@ -162,7 +172,10 @@ def title | |||
|
|||
github_client = users.sample.github_client | |||
github_org = GitHubOrganization.new(github_client, github_id) | |||
github_org.name.present? ? github_org.name : github_org.login | |||
|
|||
org_identifier = github_org.name.present? ? github_org.name : github_org.login |
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.
Rails/Presence: Use github_org.name.presence || github_org.login instead of github_org.name.present? ? github_org.name : github_org.login.
app/models/organization/creator.rb
Outdated
|
||
# There should only be one webhook that Classroom creates in production | ||
webhooks.first.id | ||
rescue GitHub::Error => err |
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.
Lint/UselessAssignment: Useless assignment to variable - err.
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.
lol LGTM 😄
Before shipping we better ensure the app functions as normal. I have a hunch this could introduce some side effects. |
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.
Played around with it a little bit locally, all looked good. Excited to get this out the door 🎉
We have deployed a couple of fixes to prevent starter code imports from hanging. Please report any instance of starter code importing being slow. |
@ejgutierrez74 Sorry for the delay, we were busy fixing up some bugs that popped up on Classroom and Classroom Assistant last week. We're working on getting this feature out quickly and will post more updates soon, please drop any questions in #595 since we are using the thread on the PR here to track development. |
@BenEmdon @johndbritton Would love a re-review before we merge this PR. Just merged a feature flag to control who can create multiple classrooms in an organization. Like I mentioned in our standup meeting, users without the feature flag turned on will be restricted from creating multiple classrooms just like before. Let me know if you guys have any questions! If all looks good, I'll merge this next week and monitor for needles for a few hours. We can revert this PR if anything goes wrong. |
Saw that stafftools were hard scan over with this change so I submitted a PR to fix that in #1681 |
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.
Feature flag changes look good to me 👍
Closes #595
After I opened #1247 I wondered what it would look like if we just removed all of the safe guards and just let an org be used for multiple classrooms.
Turns out it's not nearly as bad as I thought 😄
ToDO
Follow Up
Organization
model toClassroom
model (that's gonna be a pain in the butt)