DEV: use group ids to allow assign on groups [Undo Revert] (#41)

* DEV: use group ids to allow assign on groups (#38) [Undo Revert]

This reverts commit 8f92c5f9bd.

* Set the  default inside a migration based on the core migration. Improve migration check to work against tests-passed/beta/stable.

* Update db/migrate/20190718144722_set_assign_allowed_on_groups_default.rb

Co-Authored-By: Robin Ward <robin.ward@gmail.com>

config/settings.yml

@@ -31,6 +31,6 @@ plugins:
     client: true
     type: group_list
     list_type: compact
-    default: 'staff'
+    default: ''
     allow_any: false
     refresh: true

db/migrate/20190718144722_set_assign_allowed_on_groups_default.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class SetAssignAllowedOnGroupsDefault < ActiveRecord::Migration[5.2]
+  def up
+    current_values = DB.query_single("SELECT value FROM site_settings WHERE name = 'assign_allowed_on_groups'").first
+
+    # Dynamically sets the default value, supports older versions.
+    if current_values.nil?
+      min_version = 201_907_171_337_43
+      migrated_site_setting = DB.query_single(
+        "SELECT schema_migrations.version FROM schema_migrations WHERE schema_migrations.version = '#{min_version}'"
+      ).first
+      default = migrated_site_setting.present? ? Group::AUTO_GROUPS[:staff] : 'staff'
+
+      execute "INSERT INTO site_settings (name, data_type, value, created_at, updated_at)
+                VALUES ('assign_allowed_on_groups', #{SiteSettings::TypeSupervisor.types[:group_list]}, '#{default}', now(), now())"
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

jobs/scheduled/enqueue_reminders.rb

@@ -16,8 +16,7 @@ module Jobs
     end
 
     def allowed_group_ids
-      allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
-      Group.where(name: allowed_groups).pluck(:id).join(',')
+      Group.assign_allowed_groups.pluck(:id).join(',')
     end
 
     def user_ids

lib/topic_assigner.rb

@@ -96,7 +96,7 @@ class ::TopicAssigner
       allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
 
       User.human_users
-        .joins(:groups).where(groups: { name: allowed_groups })
+        .assign_allowed
         .where('username_lower IN (?)', mentions.map(&:downcase))
         .first
     end

plugin.rb

@@ -23,6 +23,13 @@ Discourse::Application.routes.append do
   get "topics/messages-assigned/:username" => "list#messages_assigned", as: "topics_messages_assigned", constraints: { username: /[\w.\-]+?/ }
 end
 
+
+# TODO: Remove this once 2.4.0.beta3 is released.
+# HACK: Checking if the file exists, this means we can assume the migration happenned
+above_min_version = File.exist?(
+  File.expand_path('../../../db/migrate/20190717133743_migrate_group_list_site_settings.rb', __FILE__)
+)
+
 after_initialize do
   require File.expand_path('../jobs/scheduled/enqueue_reminders.rb', __FILE__)
   require File.expand_path('../jobs/regular/remind_user.rb', __FILE__)
@@ -48,11 +55,19 @@ after_initialize do
 =end
   reviewable_api_enabled = defined?(Reviewable)
 
+  # TODO: Remove this once 2.4 becomes the new stable.
+  attribute = above_min_version ? 'id' : 'name'
+
+  add_class_method(:group, :assign_allowed_groups) do
+    allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
+    where("groups.#{attribute} IN (?)", allowed_groups)
+  end
+
   add_to_class(:user, :can_assign?) do
     @can_assign ||=
       begin
         allowed_groups = SiteSetting.assign_allowed_on_groups.split('|').compact
-        allowed_groups.present? && groups.where('name in (?)', allowed_groups).exists? ?
+        allowed_groups.present? && groups.where("groups.#{attribute} in (?)", allowed_groups).exists? ?
           :true : :false
       end
     @can_assign == :true
@@ -62,21 +77,22 @@ after_initialize do
 
   add_class_method(:user, :assign_allowed) do
     allowed_groups = SiteSetting.assign_allowed_on_groups.split('|')
-    where('users.id IN (
+    where("users.id IN (
       SELECT user_id FROM group_users
       INNER JOIN groups ON group_users.group_id = groups.id
-      WHERE groups.name IN (?)
-    )', allowed_groups)
+      WHERE groups.#{attribute} IN (?)
+    )", allowed_groups)
   end
 
   add_model_callback(Group, :before_update) do
-    if name_changed?
+    if !above_min_version && name_changed?
       SiteSetting.assign_allowed_on_groups = SiteSetting.assign_allowed_on_groups.gsub(name_was, name)
     end
   end
 
   add_model_callback(Group, :before_destroy) do
-    new_setting = SiteSetting.assign_allowed_on_groups.gsub(/#{name}[|]?/, '')
+    to_remove = above_min_version ? id : name
+    new_setting = SiteSetting.assign_allowed_on_groups.gsub(/#{to_remove}[|]?/, '')
     new_setting = new_setting.chomp('|') if new_setting.ends_with?('|')
     SiteSetting.assign_allowed_on_groups = new_setting
   end

spec/models/group_spec.rb

@@ -10,19 +10,18 @@ RSpec.describe Group do
       SiteSetting.assign_enabled = true
     end
 
-    it 'updates the site setting when the group name changes' do
-      SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators"
-      different_name = 'different_name'
-
-      group.update!(name: different_name)
-
-      expect(SiteSetting.assign_allowed_on_groups).to eq "#{different_name}|staff|moderators"
+    let(:above_min_version) do
+      min_version = 201_907_171_337_43
+      migrated_site_setting = DB.query_single(
+        "SELECT schema_migrations.version FROM schema_migrations WHERE schema_migrations.version = '#{min_version}'"
+      ).first.present?
     end
 
-    let(:removed_group_setting) { 'staff|moderators' }
+    let(:removed_group_setting) { above_min_version ? '3|4' : 'staff|moderators' }
+    let(:group_attribute) { above_min_version ? group.id : group.name }
 
     it 'removes the group from the setting when the group gets destroyed' do
-      SiteSetting.assign_allowed_on_groups = "#{group.name}|staff|moderators"
+      SiteSetting.assign_allowed_on_groups = "#{group_attribute}|#{removed_group_setting}"
 
       group.destroy!
 
@@ -30,7 +29,7 @@ RSpec.describe Group do
     end
 
     it 'removes the group from the setting when this is the last one on the list' do
-      SiteSetting.assign_allowed_on_groups = "staff|moderators|#{group.name}"
+      SiteSetting.assign_allowed_on_groups = "#{removed_group_setting}|#{group_attribute}"
 
       group.destroy!
 
@@ -38,7 +37,8 @@ RSpec.describe Group do
     end
 
     it 'removes the group from the list when it is on the middle of the list' do
-      SiteSetting.assign_allowed_on_groups = "staff|#{group.name}|moderators"
+      allowed_groups = above_min_version ? "3|#{group_attribute}|4" : "staff|#{group_attribute}|moderators"
+      SiteSetting.assign_allowed_on_groups = allowed_groups
 
       group.destroy!
 

spec/requests/assign_controller_spec.rb

@@ -11,6 +11,13 @@ RSpec.describe DiscourseAssign::AssignController do
   let(:post) { Fabricate(:post) }
   let(:user2) { Fabricate(:active_user) }
 
+  let(:above_min_version) do
+    min_version = 201_907_171_337_43
+      migrated_site_setting = DB.query_single(
+        "SELECT schema_migrations.version FROM schema_migrations WHERE schema_migrations.version = '#{min_version}'"
+      ).first.present?
+  end
+
   describe 'only allow users from allowed groups' do
     before { sign_in(user2) }
 
@@ -31,7 +38,13 @@ RSpec.describe DiscourseAssign::AssignController do
         allowed_group = Group.find_by(name: 'everyone')
         user2.groups << allowed_group
         user2.groups << default_allowed_group
-        SiteSetting.assign_allowed_on_groups = 'staff|everyone'
+        defaults = if above_min_version
+          "#{default_allowed_group.id}|#{allowed_group.id}"
+        else
+          "#{default_allowed_group.name}|#{allowed_group.name}"
+        end
+
+        SiteSetting.assign_allowed_on_groups = defaults
         TopicAssigner.new(post.topic, user).assign(user2)
 
         get '/assign/suggestions.json'
@@ -43,7 +56,7 @@ RSpec.describe DiscourseAssign::AssignController do
       it 'does not include users from disallowed groups' do
         allowed_group = Group.find_by(name: 'everyone')
         user2.groups << allowed_group
-        SiteSetting.assign_allowed_on_groups = 'staff'
+        SiteSetting.assign_allowed_on_groups = above_min_version ? default_allowed_group.id.to_s : default_allowed_group.name
         TopicAssigner.new(post.topic, user).assign(user2)
 
         get '/assign/suggestions.json'