Allow a GitHub Organization to be used by multiple Classrooms

app/controllers/organizations_controller.rb

@@ -120,27 +120,25 @@ def new_organization_params
     params.require(:organization).permit(:github_id).merge(users: [current_user])
   end
 
-  # rubocop:disable AbcSize
   def set_users_github_organizations
     @users_github_organizations = current_user.github_user.organization_memberships.map do |membership|
       {
-        classroom:   Organization.unscoped.find_by(github_id: membership.organization.id),
         github_id:   membership.organization.id,
         login:       membership.organization.login,
         owner_login: membership.user.login,
         role:        membership.role
       }
     end
   end
-  # rubocop:enable AbcSize
 
   # Check if the current user has any organizations with admin privilege,
   # if so add the user to the corresponding classroom automatically.
   def add_current_user_to_organizations
     @users_github_organizations.each do |organization|
-      classroom = organization[:classroom]
-      if classroom.present? && !classroom.users.include?(current_user)
-        create_user_organization_access(classroom)
+      user_orgs = Organization.where(github_id: organization[:github_id])
+
+      user_orgs.map do |user_org|
+        create_user_organization_access(user_org) unless user_org.users.include?(current_user)
       end
     end
   end

app/models/concerns/sluggable.rb

@@ -4,16 +4,20 @@ module Sluggable
   extend ActiveSupport::Concern
 
   included do
-    before_validation do
-      slugify
-    end
+    before_validation :slugify
   end
 
   def slugify
-    self.slug = title.parameterize
+    self.slug = name_for_slug.parameterize
   end
 
   def to_param
     slug
   end
+
+  private
+
+  def name_for_slug
+    title
+  end
 end

app/models/github_organization.rb

@@ -116,6 +116,12 @@ def create_organization_webhook(config: {}, options: {})
     end
   end
 
+  def organization_webhooks
+    GitHub::Errors.with_error_handling do
+      @client.org_hooks(@id)
+    end
+  end
+
   def remove_organization_webhook(webhook_id)
     return if webhook_id.blank?
     GitHub::Errors.with_error_handling do

app/models/organization.rb

@@ -17,15 +17,14 @@ class Organization < ApplicationRecord
 
   has_and_belongs_to_many :users
 
-  validates :github_id, presence: true, uniqueness: true
+  validates :github_id, presence: true
 
   validates :title, presence: true
   validates :title, length: { maximum: 60 }
+  validates :title, uniqueness: { scope: :github_id }
 
   validates :slug, uniqueness: true
 
-  validates :webhook_id, uniqueness: true, allow_nil: true
-
   before_destroy :silently_remove_organization_webhook
 
   def all_assignments(with_invitations: false)
@@ -48,15 +47,22 @@ def github_organization
     @github_organization ||= GitHubOrganization.new(github_client, github_id)
   end
 
-  def slugify
-    self.slug = "#{github_id} #{title}".parameterize
+  def name_for_slug
+    "#{github_id} #{title}"
   end
 
   def one_owner_remains?
     users.count == 1
   end
 
+  # Check if we are the last Classroom on this GitHub Organization
+  def last_classroom_on_org?
+    Organization.where(github_id: github_id).length <= 1
+  end
+
   def silently_remove_organization_webhook
+    return true unless last_classroom_on_org?
+
     begin
       github_organization.remove_organization_webhook(webhook_id)
     rescue GitHub::Error => err

app/models/organization/creator.rb

@@ -95,20 +95,30 @@ def perform
     # Internal: Create an GitHub Organization WebHook if there
     # is a user who has the correct token scope.
     #
-    # Returns an Integer id, or raises a Result::Error
+    # Returns an Integer id, or raises a Result::Error#
+    # rubocop:disable MethodLength
+    # rubocop:disable AbcSize
     def create_organization_webhook!
       return unless (user = user_with_admin_org_hook_scope)
 
       begin
+        return existing_webhook_id unless existing_webhook_id.nil?
+
         github_organization = GitHubOrganization.new(user.github_client, github_id)
         webhook = github_organization.create_organization_webhook(config: { url: webhook_url })
 
-        return webhook.id if webhook.try(:id).present?
-        raise GitHub::Error
-      rescue GitHub::Error
+        raise GitHub::Error if webhook.try(:id).nil?
+
+        webhook.id
+      rescue GitHub::Error => e
+        github_webhook_id = process_webhook_error(e.message, github_organization)
+        return github_webhook_id unless github_webhook_id.nil?
+
         raise Result::Error, "Could not create WebHook, please try again."
       end
     end
+    # rubocop:enable AbcSize
+    # rubocop:enable MethodLength
 
     # Internal: Make sure every user being added to the
     # Organization is an admin on GitHub.
@@ -167,7 +177,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.presence || github_org.login
+
+      find_unique_title(org_identifier)
     end
 
     # Internal: Find User that has the `admin:org_hook` scope
@@ -222,5 +235,62 @@ def webhook_url
 
       raise Result::Error, error_message
     end
+
+    # Internal: Check error from GitHub when we try to create the webhook
+    # If the hook already exists get the webhook id from GitHub and return
+    #
+    # Returns a String for the webhook_id or nil
+    def process_webhook_error(message, github_organization)
+      return nil unless message == "Hook: Hook already exists on this organization"
+
+      get_organization_webhook_id(github_organization)
+    end
+
+    # Internal: Get id of webhook already on GitHub
+    #
+    # Returns a String for the webhook_id or nil
+    def get_organization_webhook_id(github_organization)
+      webhooks = github_organization.organization_webhooks
+
+      # There should only be one webhook that Classroom creates in production
+      webhooks.first.id
+    rescue GitHub::Error
+      nil
+    end
+
+    # Internal: Find webhook_id of any existing classrooms with
+    # same github id
+    #
+    # Returns a String for the webhook_id or nil
+    def existing_webhook_id
+      classroom_with_same_org = Organization.find_by(github_id: github_id)
+      classroom_with_same_org ? classroom_with_same_org.webhook_id : nil
+    end
+
+    # Internal: Generates unique name for classroom if default
+    # name has already been taken
+    #
+    # Returns a String with an unique title
+    def find_unique_title(identifier)
+      # First Classroom on Org will have postfix 1
+      base_title = "#{identifier}-classroom-1"
+
+      count = 1
+      until unique_title?(base_title)
+        # Increments count at end of title
+        base_title = base_title.gsub(/\-\d+$/, "") + "-#{count}"
+        count += 1
+      end
+
+      base_title
+    end
+
+    # Internal: Checks if a classroom with the same title and github_id
+    # exists already
+    #
+    # Returns a Boolean on whether duplicate classroom title is
+    def unique_title?(base_title)
+      Organization.where(title: base_title, github_id: github_id).blank?
+    end
   end
 end

app/views/organizations/_disabled_organization_select.html.erb

@@ -1,5 +1,4 @@
-<% aria_label = org[:role] != 'admin' ? t('views.organizations.you_are_not_owner') : t('views.organizations.already_added', owner: org[:owner_login]) %>
-<div class="organization-select-target disabled tooltipped tooltipped-s" aria-label="<%= aria_label %>">
+<div class="organization-select-target disabled tooltipped tooltipped-s" aria-label="<%= t('views.organizations.you_are_not_owner') %>">
   <%= image_tag "https://avatars.githubusercontent.com/u/#{org[:github_id]}?v=3&size=200", class: 'avatar organization-select-avatar', width: 100, height: 100, alt: "@#{org[:login]}" %>
   <span class="organization css-truncate css-truncate-target" title=<%= "@#{org[:login]}" %>><%= "@#{org[:login]}" %></span>
 </div>

app/views/organizations/_reset_and_remove_organization_modal.html.erb

@@ -12,7 +12,7 @@
     </p>
   </div>
 
-  <%= form_for current_organization, html: { "data-name" => current_organization.github_organization.login, method: 'delete' } do |f| %>
+  <%= form_for current_organization, html: { "data-name" => current_organization.title, method: 'delete' } do |f| %>
     <dl class="form js-normalize-submit">
       <dt><%= t('views.organizations.delete_confirm') %></dt>
       <dd><input type="text" class="js-input-block form-control input-contrast input-block" autofocus></dd>

app/views/organizations/new.html.erb

@@ -16,7 +16,7 @@
 
         <div class="org-select-grid">
           <% @users_github_organizations.each do |org| %>
-            <% if org[:role] == 'admin' && !org[:classroom].present? %>
+            <% if org[:role] == 'admin' %>
               <%= render partial: 'organization_select', locals: { org: org } %>
             <% else %>
               <%= render partial: 'disabled_organization_select', locals: { org: org } %>

config/locales/views/organizations/en.yml

@@ -30,8 +30,8 @@ en:
       group_assignments: 'Group Assignments'
       are_you_sure: 'Are you ABSOLUTELY sure?'
       delete_warning: "Unexpected things will happen if you don't read this!"
-      delete_consequences_html: "This action <strong>CANNOT</strong> be undone. Please note that reseting and removing this classroom will delete all repositories and teams under the <strong>%{organization}</strong> account that were created by GitHub Classroom."
-      delete_confirm: 'Please type the name of the organization to confirm'
+      delete_consequences_html: "This action <strong>CANNOT</strong> be undone. Please note that reseting and removing this classroom will delete all repositories and teams created by this classroom in the <strong>%{organization}</strong> account."
+      delete_confirm: 'Please type the name of the classroom to confirm'
       delete_submit: 'Reset and remove this classroom'
       classroom_settings: 'Classroom settings'
       classroom_profile: 'Classroom profile'

spec/controllers/organizations_controller_spec.rb

@@ -114,13 +114,6 @@
       end.to_not change(Organization, :count)
     end
 
-    it "will not add an organization that already exists" do
-      existing_organization_options = { github_id: organization.github_id }
-      expect do
-        post :create, params: { organization: existing_organization_options }
-      end.to_not change(Organization, :count)
-    end
-
     it "will add an organization that the user is admin of on GitHub" do
       organization_params = { github_id: organization.github_id, users: organization.users }
       organization.destroy!

spec/models/organization/creator_spec.rb

@@ -7,7 +7,7 @@
   let(:user)                   { classroom_teacher                           }
 
   after(:each) do
-    @organization.try(:destroy)
+    Organization.find_each(&:destroy)
   end
 
   describe "::perform", :vcr do
@@ -26,6 +26,41 @@
 
         Organization::Creator.perform(github_id: github_organization_id, users: [user])
       end
+
+      context "multiple classrooms on same organization" do
+        before do
+          result = Organization::Creator.perform(github_id: github_organization_id, users: [user])
+          @org = result.organization
+        end
+
+        it "creates a classroom with the same webhook id as the existing one" do
+          result = Organization::Creator.perform(github_id: github_organization_id, users: [user])
+          expect(result.organization.webhook_id).to eql(@org.webhook_id)
+        end
+
+        it "creates a classroom with the default title but incremented id" do
+          result = Organization::Creator.perform(github_id: github_organization_id, users: [user])
+          expect(result.organization.title).to eql("#{@org.title[0...-2]}-2")
+        end
+      end
+
+      context "already created webhook on GitHub org" do
+        before do
+          GitHubOrganization
+            .any_instance.stub(:create_organization_webhook)
+            .and_raise GitHub::Error, "Hook: Hook already exists on this organization"
+
+          @dummy_webhook_id = 12_345
+          Organization::Creator
+            .any_instance.stub(:get_organization_webhook_id)
+            .and_return @dummy_webhook_id
+        end
+
+        it "sets webhook id to what it is already set on org" do
+          result = Organization::Creator.perform(github_id: github_organization_id, users: [user])
+          expect(result.organization.webhook_id).to eql(@dummy_webhook_id)
+        end
+      end
     end
 
     describe "unsucessful creation" do

spec/models/organization_spec.rb

@@ -58,18 +58,65 @@
     end
   end
 
+  describe "#last_classroom_on_org?" do
+    context "only one classroom with github_id" do
+      it "returns true" do
+        expect(subject.last_classroom_on_org?).to be_truthy
+      end
+    end
+
+    context "multiple classrooms with same github_id" do
+      before do
+        create(:organization, github_id: 12_345)
+      end
+
+      it "returns false" do
+        expect(subject.last_classroom_on_org?).to be_falsey
+      end
+    end
+
+    context "multiple classrooms with different github_ids" do
+      before do
+        create(:organization, github_id: 0)
+      end
+
+      it "returns true" do
+        expect(subject.last_classroom_on_org?).to be_truthy
+      end
+    end
+  end
+
   describe "callbacks" do
     describe "before_destroy" do
       describe "#silently_remove_organization_webhook", :vcr do
-        it "deletes the webhook from GitHub" do
-          subject.update_attributes(webhook_id: 9_999_999, is_webhook_active: true)
+        context "multiple classrooms on organization" do
+          before do
+            create(:organization, github_id: 12_345)
+          end
+
+          it "does not delete the webhook from GitHub" do
+            subject.update(webhook_id: 9_999_999, is_webhook_active: true)
+
+            org_id     = subject.github_id
+            webhook_id = subject.webhook_id
+
+            subject.destroy
+
+            expect(WebMock).to_not have_requested(:delete, github_url("/organizations/#{org_id}/hooks/#{webhook_id}"))
+          end
+        end
+
+        context "last classroom on organization" do
+          it "deletes the webhook from GitHub" do
+            subject.update(webhook_id: 9_999_999, is_webhook_active: true)
 
-          org_id     = subject.github_id
-          webhook_id = subject.webhook_id
+            org_id     = subject.github_id
+            webhook_id = subject.webhook_id
 
-          subject.destroy
+            subject.destroy
 
-          expect(WebMock).to have_requested(:delete, github_url("/organizations/#{org_id}/hooks/#{webhook_id}"))
+            expect(WebMock).to have_requested(:delete, github_url("/organizations/#{org_id}/hooks/#{webhook_id}"))
+          end
         end
       end
     end

spec/support/cassettes/Organization/callbacks/before_destroy/_silently_remove_organization_webhook/last_classroom_on_organization/deletes_the_webhook_from_GitHub.json

@@ -0,0 +1 @@
+{"http_interactions":[{"request":{"method":"get","uri":"https://api.github.com/organizations/12345","body":{"encoding":"US-ASCII","base64_string":""},"headers":{"User-Agent":["Octokit Ruby Gem 4.9.0"],"Accept":["application/vnd.github.v3+json"],"Content-Type":["application/json"],"Expect":[""]}},"response":{"status":{"code":401,"message":"Unauthorized"},"headers":{"Date":["Wed, 25 Jul 2018 05:38:11 GMT"],"Content-Type":["application/json; charset=utf-8"],"Content-Length":["83"],"Server":["GitHub.com"],"Status":["401 Unauthorized"],"X-Github-Media-Type":["github.v3; format=json"],"X-Ratelimit-Limit":["60"],"X-Ratelimit-Remaining":["59"],"X-Ratelimit-Reset":["1532500691"],"Access-Control-Expose-Headers":["ETag, Link, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval"],"Access-Control-Allow-Origin":["*"],"Strict-Transport-Security":["max-age=31536000; includeSubdomains; preload"],"X-Frame-Options":["deny"],"X-Content-Type-Options":["nosniff"],"X-Xss-Protection":["1; mode=block"],"Referrer-Policy":["origin-when-cross-origin, strict-origin-when-cross-origin"],"Content-Security-Policy":["default-src 'none'"],"X-Runtime-Rack":["0.019451"],"X-Github-Request-Id":["F509:1708:93F504:C1FC47:5B580CC3"]},"body":{"encoding":"UTF-8","base64_string":"eyJtZXNzYWdlIjoiQmFkIGNyZWRlbnRpYWxzIiwiZG9jdW1lbnRhdGlvbl91\ncmwiOiJodHRwczovL2RldmVsb3Blci5naXRodWIuY29tL3YzIn0=\n"},"http_version":null},"recorded_at":"Wed, 25 Jul 2018 05:38:11 GMT"},{"request":{"method":"get","uri":"https://api.github.com/organizations/12345?client_id=\u003cTEST_APPLICATION_GITHUB_CLIENT_ID\u003e\u0026client_secret=\u003cTEST_APPLICATION_GITHUB_CLIENT_SECRET\u003e","body":{"encoding":"US-ASCII","base64_string":""},"headers":{"User-Agent":["Octokit Ruby Gem 4.9.0"],"Accept":["application/vnd.github.v3+json"],"Content-Type":["application/json"],"Expect":[""]}},"response":{"status":{"code":404,"message":"Not Found"},"headers":{"Date":["Wed, 25 Jul 2018 05:38:11 GMT"],"Content-Type":["application/json; charset=utf-8"],"Content-Length":["103"],"Server":["GitHub.com"],"Status":["404 Not Found"],"X-Ratelimit-Limit":["5000"],"X-Ratelimit-Remaining":["4998"],"X-Ratelimit-Reset":["1532500691"],"X-Github-Media-Type":["github.v3; format=json"],"Access-Control-Expose-Headers":["ETag, Link, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval"],"Access-Control-Allow-Origin":["*"],"Strict-Transport-Security":["max-age=31536000; includeSubdomains; preload"],"X-Frame-Options":["deny"],"X-Content-Type-Options":["nosniff"],"X-Xss-Protection":["1; mode=block"],"Referrer-Policy":["origin-when-cross-origin, strict-origin-when-cross-origin"],"Content-Security-Policy":["default-src 'none'"],"X-Runtime-Rack":["0.016530"],"X-Github-Request-Id":["F509:1708:93F516:C1FC5D:5B580CC3"]},"body":{"encoding":"UTF-8","base64_string":"eyJtZXNzYWdlIjoiTm90IEZvdW5kIiwiZG9jdW1lbnRhdGlvbl91cmwiOiJo\ndHRwczovL2RldmVsb3Blci5naXRodWIuY29tL3YzL29yZ3MvI2dldC1hbi1v\ncmdhbml6YXRpb24ifQ==\n"},"http_version":null},"recorded_at":"Wed, 25 Jul 2018 05:38:11 GMT"},{"request":{"method":"delete","uri":"https://api.github.com/organizations/12345/hooks/9999999","body":{"encoding":"UTF-8","base64_string":"e30=\n"},"headers":{"User-Agent":["Octokit Ruby Gem 4.9.0"],"Accept":["application/vnd.github.v3+json"],"Content-Type":["application/json"],"Expect":[""]}},"response":{"status":{"code":401,"message":"Unauthorized"},"headers":{"Date":["Wed, 25 Jul 2018 05:38:11 GMT"],"Content-Type":["application/json; charset=utf-8"],"Content-Length":["83"],"Server":["GitHub.com"],"Status":["401 Unauthorized"],"X-Github-Media-Type":["github.v3; format=json"],"X-Ratelimit-Limit":["60"],"X-Ratelimit-Remaining":["57"],"X-Ratelimit-Reset":["1532500691"],"Access-Control-Expose-Headers":["ETag, Link, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval"],"Access-Control-Allow-Origin":["*"],"Strict-Transport-Security":["max-age=31536000; includeSubdomains; preload"],"X-Frame-Options":["deny"],"X-Content-Type-Options":["nosniff"],"X-Xss-Protection":["1; mode=block"],"Referrer-Policy":["origin-when-cross-origin, strict-origin-when-cross-origin"],"Content-Security-Policy":["default-src 'none'"],"X-Runtime-Rack":["0.013376"],"X-Github-Request-Id":["F509:1708:93F522:C1FC73:5B580CC3"]},"body":{"encoding":"UTF-8","base64_string":"eyJtZXNzYWdlIjoiQmFkIGNyZWRlbnRpYWxzIiwiZG9jdW1lbnRhdGlvbl91\ncmwiOiJodHRwczovL2RldmVsb3Blci5naXRodWIuY29tL3YzIn0=\n"},"http_version":null},"recorded_at":"Wed, 25 Jul 2018 05:38:11 GMT"}],"recorded_with":"VCR 3.0.3"}