Merge pull request #482 from NickLaMuro/triage-team-label-exceptions

[AddLabel/RemoveLabel] Allow triage team to add all labels

Gemfile

@@ -47,7 +47,7 @@ gem 'travis',         '~> 1.7.6'
 gem 'awesome_spawn',        '>= 1.4.1'
 gem 'default_value_for',    '>= 3.1.0'
 gem 'haml_lint',            '~> 0.20.0', :require => false
-gem 'more_core_extensions', '~> 2.0.0',  :require => 'more_core_extensions/all'
+gem 'more_core_extensions', '~> 4.0.0',  :require => 'more_core_extensions/all'
 gem 'rubocop',              '~> 0.69.0', :require => false
 gem 'rubocop-performance',  '~> 1.3',    :require => false
 gem 'rugged',                            :require => false

Gemfile.lock

@@ -163,8 +163,8 @@ GEM
     mini_portile2 (2.4.0)
     minigit (0.0.4)
     minitest (5.10.3)
-    more_core_extensions (2.0.0)
-      activesupport (> 3.2)
+    more_core_extensions (4.0.0)
+      activesupport
     multi_json (1.14.1)
     multipart-post (2.0.0)
     net-http-persistent (2.9.4)
@@ -370,7 +370,7 @@ DEPENDENCIES
   jquery-rails
   listen
   minigit (~> 0.0.4)
-  more_core_extensions (~> 2.0.0)
+  more_core_extensions (~> 4.0.0)
   octokit (~> 4.8.0)
   pg
   rails (~> 4.2.4)

config/application.rb

@@ -21,6 +21,7 @@ class Application < Rails::Application
     # config.i18n.default_locale = :de
 
     config.eager_load_paths << Rails.root.join("app/workers/concerns")
+    config.eager_load_paths << Rails.root.join("lib/github_service/concerns")
     config.eager_load_paths << Rails.root.join("lib")
 
     console do

lib/github_service/commands/add_label.rb

@@ -1,6 +1,8 @@
 module GithubService
   module Commands
     class AddLabel < Base
+      include IsTeamMember
+
       UNASSIGNABLE = {
         "jansa/yes" => "jansa/yes?"
       }.freeze
@@ -9,7 +11,7 @@ class AddLabel < Base
 
       def _execute(issuer:, value:)
         valid, invalid = extract_label_names(value)
-        process_extracted_labels(valid, invalid)
+        process_extracted_labels(issuer, valid, invalid)
 
         if invalid.any?
           issue.add_comment(invalid_label_message(issuer, invalid))
@@ -34,9 +36,9 @@ def validate_labels(label_names)
         label_names.partition { |l| GithubService.valid_label?(issue.fq_repo_name, l) }
       end
 
-      def process_extracted_labels(valid_labels, invalid_labels)
+      def process_extracted_labels(issuer, valid_labels, invalid_labels)
         correct_invalid_labels(valid_labels, invalid_labels)
-        handle_unassignable_labels(valid_labels)
+        handle_unassignable_labels(valid_labels) unless triage_member?(issuer)
 
         [valid_labels, invalid_labels]
       end

lib/github_service/commands/remove_label.rb

@@ -1,6 +1,8 @@
 module GithubService
   module Commands
     class RemoveLabel < Base
+      include IsTeamMember
+
       UNREMOVABLE = %w[jansa/yes jansa/no].freeze
 
       alias_as 'rm_label'
@@ -10,7 +12,7 @@ class RemoveLabel < Base
       def _execute(issuer:, value:)
         unremovable    = []
         valid, invalid = extract_label_names(value)
-        process_extracted_labels(valid, invalid, unremovable)
+        process_extracted_labels(issuer, valid, invalid, unremovable)
 
         if invalid.any?
           message = "@#{issuer} Cannot remove the following label#{"s" if invalid.length > 1} because they are not recognized: "
@@ -36,9 +38,11 @@ def extract_label_names(value)
         validate_labels(label_names)
       end
 
-      def process_extracted_labels(valid_labels, _invalid_labels, unremovable)
-        valid_labels.each { |label| unremovable << label if UNREMOVABLE.include?(label) }
-        unremovable.each  { |label| valid_labels.delete(label) }
+      def process_extracted_labels(issuer, valid_labels, _invalid_labels, unremovable)
+        unless triage_member?(issuer)
+          valid_labels.each { |label| unremovable << label if UNREMOVABLE.include?(label) }
+          unremovable.each  { |label| valid_labels.delete(label) }
+        end
       end
 
       def validate_labels(label_names)

lib/github_service/concerns/is_team_member.rb

@@ -0,0 +1,36 @@
+module IsTeamMember
+  def triage_member?(username)
+    IsTeamMember.triage_team_members.include?(username)
+  end
+
+  # List of usernames for the traige team
+  #
+  # Cache triage_team_members, and refresh cache every 24 hours
+  #
+  # Note:  This is created as a class method
+  #
+  cache_with_timeout(:triage_team_members, 24 * 60 * 60) do
+    if member_organization_name && triage_team_name
+      team = GithubService.org_teams(member_organization_name)
+                          .detect { |t| t.name == triage_team_name }
+
+      if team.nil?
+        []
+      else
+        GithubService.team_members(team.id).map(&:login)
+      end
+    else
+      []
+    end
+  end
+
+  module_function
+
+  def triage_team_name
+    @triage_team_name ||= Settings.triage_team_name || nil
+  end
+
+  def member_organization_name
+    @member_organization_name ||= Settings.member_organization_name || nil
+  end
+end

spec/lib/github_service/commands/add_label_spec.rb

@@ -7,6 +7,15 @@
   let(:command_issuer) { "chessbyte" }
   let(:command_value) { "question, wontfix" }
 
+  let(:miq_teams) do
+    [
+      {"id" => 1234, "name" => "commiters"},
+      {"id" => 2345, "name" => "core-triage"},
+      {"id" => 3456, "name" => "my-triage-team"},
+      {"id" => 4567, "name" => "UI"}
+    ]
+  end
+
   before do
     allow(issue).to receive(:applied_label?).with("question").and_return(true)
     allow(issue).to receive(:applied_label?).with("wontfix").and_return(false)
@@ -18,6 +27,11 @@
   end
 
   after do
+    # unset class variables (weird ordering thanks to @chrisarcand...)
+    [:@triage_team_name, :@member_organization_name].each do |var|
+      IsTeamMember.remove_instance_variable(var) if IsTeamMember.instance_variable_defined?(var)
+    end
+
     subject.execute!(:issuer => command_issuer, :value => command_value)
   end
 
@@ -68,16 +82,59 @@
   end
 
   context "un-assignable labels" do
-    let(:command_value) { "jansa/yes" }
+    let(:command_value)  { "jansa/yes" }
+    let(:triage_members) { [{"login" => "Fryguy"}] }
 
     before do
-      allow(issue).to receive(:applied_label?).with("jansa/yes?").and_return(false)
-      allow(GithubService).to receive(:valid_label?).with("foo/bar", command_value).and_return(true)
+      github_service_add_stub :url           => "/orgs/ManageIQ/teams?per_page=100",
+                              :response_body => miq_teams.to_json
+      github_service_add_stub :url           => "/teams/3456/members?per_page=100",
+                              :response_body => triage_members.to_json
+    end
+
+    context "without a triage team" do
+      before do
+        allow(issue).to receive(:applied_label?).with("jansa/yes?").and_return(false)
+        allow(GithubService).to receive(:valid_label?).with("foo/bar", command_value).and_return(true)
+      end
+
+      it "corrects the label to 'jansa/yes?'" do
+        expect(issue).to receive(:add_labels).with(["jansa/yes?"])
+        expect(issue).not_to receive(:add_comment)
+      end
     end
 
-    it "corrects the label to 'jansa/yes?'" do
-      expect(issue).to receive(:add_labels).with(["jansa/yes?"])
-      expect(issue).not_to receive(:add_comment)
+    context "with a non-triage user" do
+      before do
+        allow(issue).to receive(:applied_label?).with("jansa/yes?").and_return(false)
+        allow(GithubService).to receive(:valid_label?).with("foo/bar", command_value).and_return(true)
+        stub_settings(:member_organization_name => "ManageIQ", :triage_team_name => "my-triage-team")
+      end
+
+      it "applies the original label" do
+        expect(issue).to receive(:add_labels).with(["jansa/yes?"])
+        expect(issue).not_to receive(:add_comment)
+      end
+    end
+
+    context "with a triage user" do
+      let(:triage_members) do
+        [
+          {"login" => "Fryguy"},
+          {"login" => command_issuer}
+        ]
+      end
+
+      before do
+        allow(issue).to receive(:applied_label?).with("jansa/yes").and_return(false)
+        allow(GithubService).to receive(:valid_label?).with("foo/bar", command_value).and_return(true)
+        stub_settings(:member_organization_name => "ManageIQ", :triage_team_name => "my-triage-team")
+      end
+
+      it "applies the original label" do
+        expect(issue).to receive(:add_labels).with(["jansa/yes"])
+        expect(issue).not_to receive(:add_comment)
+      end
     end
   end
 end

spec/lib/github_service/commands/remove_label_spec.rb

@@ -6,7 +6,21 @@
   let(:command_issuer) { "chessbyte" }
   let(:command_value) { "question, wontfix" }
 
+  let(:miq_teams) do
+    [
+      {"id" => 1234, "name" => "commiters"},
+      {"id" => 2345, "name" => "core-triage"},
+      {"id" => 3456, "name" => "my-triage-team"},
+      {"id" => 4567, "name" => "UI"}
+    ]
+  end
+
   after do
+    # unset class variables (weird ordering thanks to @chrisarcand...)
+    [:@triage_team_name, :@member_organization_name].each do |var|
+      IsTeamMember.remove_instance_variable(var) if IsTeamMember.instance_variable_defined?(var)
+    end
+
     subject.execute!(:issuer => command_issuer, :value => command_value)
   end
 
@@ -46,7 +60,15 @@
 
     context "with labels that are UNREMOVABLE" do
       # An invalid situation, just testing in one go
-      let(:command_value) { "wontfix, jansa/no, jansa/yes, jansa/yes?" }
+      let(:command_value)  { "wontfix, jansa/no, jansa/yes, jansa/yes?" }
+      let(:triage_members) { [{"login" => "Fryguy"}] }
+
+      before do
+        github_service_add_stub :url           => "/orgs/ManageIQ/teams?per_page=100",
+                                :response_body => miq_teams.to_json
+        github_service_add_stub :url           => "/teams/3456/members?per_page=100",
+                                :response_body => triage_members.to_json
+      end
 
       before do
         %w[wontfix jansa/no jansa/yes jansa/yes?].each do |label|
@@ -55,19 +77,64 @@
 
         expect(issue).to receive(:applied_label?).with("wontfix").and_return(true)
         expect(issue).to receive(:applied_label?).with("jansa/yes?").and_return(true)
+      end
+
+      context "without a triage team" do
+        before do
+          message = "@chessbyte Cannot remove the following labels since they require "              \
+                    "[triage team permissions](https://github.com/orgs/ManageIQ/teams/core-triage)" \
+                    ": jansa/no, jansa/yes"
 
-        message = "@chessbyte Cannot remove the following labels since they require "              \
-                  "[triage team permissions](https://github.com/orgs/ManageIQ/teams/core-triage)" \
-                  ": jansa/no, jansa/yes"
+          expect(issue).to receive(:add_comment).with(message)
+        end
 
-        expect(issue).to receive(:add_comment).with(message)
+        it "only removes the removable applied labels" do
+          expect(issue).to     receive(:remove_label).with("wontfix")
+          expect(issue).not_to receive(:remove_label).with("jansa/no")
+          expect(issue).not_to receive(:remove_label).with("jansa/yes")
+          expect(issue).to     receive(:remove_label).with("jansa/yes?")
+        end
       end
 
-      it "only removes the applied label" do
-        expect(issue).to     receive(:remove_label).with("wontfix")
-        expect(issue).not_to receive(:remove_label).with("jansa/no")
-        expect(issue).not_to receive(:remove_label).with("jansa/yes")
-        expect(issue).to     receive(:remove_label).with("jansa/yes?")
+      context "with a triage team" do
+        before do
+          message = "@chessbyte Cannot remove the following labels since they require "              \
+                    "[triage team permissions](https://github.com/orgs/ManageIQ/teams/core-triage)" \
+                    ": jansa/no, jansa/yes"
+
+          expect(issue).to receive(:add_comment).with(message)
+          stub_settings(:member_organization_name => "ManageIQ", :triage_team_name => "my-triage-team")
+        end
+
+        it "only removes the removable applied labels" do
+          expect(issue).to     receive(:remove_label).with("wontfix")
+          expect(issue).not_to receive(:remove_label).with("jansa/no")
+          expect(issue).not_to receive(:remove_label).with("jansa/yes")
+          expect(issue).to     receive(:remove_label).with("jansa/yes?")
+        end
+      end
+
+      context "with a triage user" do
+        let(:triage_members) do
+          [
+            {"login" => "Fryguy"},
+            {"login" => command_issuer}
+          ]
+        end
+
+        before do
+          expect(issue).to receive(:applied_label?).with("jansa/no").and_return(true)
+          expect(issue).to receive(:applied_label?).with("jansa/yes").and_return(true)
+
+          stub_settings(:member_organization_name => "ManageIQ", :triage_team_name => "my-triage-team")
+        end
+
+        it "only removes the applied label" do
+          expect(issue).to receive(:remove_label).with("wontfix")
+          expect(issue).to receive(:remove_label).with("jansa/no")
+          expect(issue).to receive(:remove_label).with("jansa/yes")
+          expect(issue).to receive(:remove_label).with("jansa/yes?")
+        end
       end
     end
   end

spec/spec_helper.rb

@@ -48,6 +48,8 @@
     allow_any_instance_of(MinigitService).to receive(:service)
       .and_raise("Live execution is not allowed in specs.  Use stubs/expectations on service instead.")
   end
+
+  config.after { Module.clear_all_cache_with_timeout }
 end
 
 WebMock.disable_net_connect!(:allow_localhost => true)