Merge branch 'batch-aggregation' of github.com:zooniverse/panoptes in…

…to batch-aggregation

app/controllers/api/v1/subject_sets_controller.rb

@@ -60,7 +60,7 @@ def destroy
 
     reset_workflow_retired_counts(affected_workflow_ids)
     subject_ids.each_with_index do |subject_id, index|
-      SubjectRemovalWorker.perform_in(index.seconds, subject_id)
+      SubjectRemovalWorker.perform_in(index.seconds, subject_id, controlled_resource.id)
     end
 
     deleted_resource_response

app/controllers/api/v1/user_groups_controller.rb

@@ -13,7 +13,16 @@ class Api::V1::UserGroupsController < Api::ApiController
   allowed_params :update, :name, :stats_visibility, :display_name
 
   search_by do |name, query|
-    query.search_name(name.join(" "))
+    search_names = name.join(' ').downcase
+    display_name_search = query.where('lower(display_name) = ?', search_names)
+
+    if display_name_search.exists?
+      display_name_search
+    elsif search_names.present? && search_names.length >= 3
+      query.full_search_display_name(search_names)
+    else
+      UserGroup.none
+    end
   end
 
   def destroy_links

app/controllers/api/v1/users_controller.rb

@@ -52,6 +52,7 @@ def update
       super do |user|
         if user.email_changed?
           update_email_user_ids << user.id
+          user.update_attribute(:valid_email, true)
         end
       end
 

app/models/user_group.rb

@@ -5,19 +5,19 @@ class UserGroup < ApplicationRecord
   include PgSearch::Model
 
   has_many :memberships, dependent: :destroy
-  has_many :active_memberships, -> { active.not_identity }, class_name: "Membership"
-  has_one  :identity_membership, -> { identity }, class_name: "Membership"
+  has_many :active_memberships, -> { active.not_identity }, class_name: 'Membership'
+  has_one  :identity_membership, -> { identity }, class_name: 'Membership'
   has_many :users, through: :memberships
   has_many :classifications, dependent: :restrict_with_exception
   has_many :access_control_lists, dependent: :destroy
 
   has_many :owned_resources, -> { where("roles && '{owner}'") },
-           class_name: "AccessControlList"
+           class_name: 'AccessControlList'
 
   has_many :projects, through: :owned_resources, source: :resource,
-           source_type: "Project"
+                      source_type: 'Project'
   has_many :collections, through: :owned_resources, source: :resource,
-           source_type: "Collection"
+                         source_type: 'Collection'
 
   ##
   # Stats_Visibility Levels (Used for ERAS stats service)
@@ -46,23 +46,38 @@ class UserGroup < ApplicationRecord
 
   validates :display_name, presence: true
   validates :name, presence: true,
-    uniqueness: { case_sensitive: false },
-    format: { with: User::USER_LOGIN_REGEX }
+                   uniqueness: { case_sensitive: false },
+                   format: { with: User::USER_LOGIN_REGEX }
 
-  before_validation :default_display_name, on: [:create, :update]
+  before_validation :default_display_name, on: %i[create update]
   before_validation :default_join_token, on: [:create]
 
   scope :public_groups, -> { where(private: false) }
 
   pg_search_scope :search_name,
-    against: :display_name,
-    using: :trigram,
-    ranked_by: ":trigram"
+                  against: [:display_name],
+                  using: {
+                    tsearch: {
+                      dictionary: 'english',
+                      tsvector_column: 'tsv'
+                    }
+                  }
+
+  pg_search_scope :full_search_display_name,
+                  against: [:display_name],
+                  using: {
+                    tsearch: {
+                      dictionary: 'english',
+                      tsvector_column: 'tsv'
+                    },
+                    trigram: {}
+                  },
+                  ranked_by: ':tsearch + (0.25 * :trigram)'
 
   def self.roles_allowed_to_access(action, klass=nil)
     roles = case action
             when :show, :index
-              [:group_admin, :group_member]
+              %i[group_admin group_member]
             else
               [:group_admin]
             end

app/workers/subject_removal_worker.rb

@@ -5,9 +5,9 @@ class SubjectRemovalWorker
 
   sidekiq_options queue: :data_low
 
-  def perform(subject_id)
+  def perform(subject_id, subject_set_id=nil)
     return unless Flipper.enabled?(:remove_orphan_subjects)
 
-    Subjects::Remover.new(subject_id).cleanup
+    Subjects::Remover.new(subject_id, nil, subject_set_id).cleanup
   end
 end

db/migrate/20240216142515_add_tsv_to_user_groups.rb

@@ -0,0 +1,9 @@
+class AddTsvToUserGroups < ActiveRecord::Migration[6.1]
+  def up
+    add_column :user_groups, :tsv, :tsvector
+  end
+
+  def down
+    remove_column :user_groups, :tsv
+  end
+end

db/migrate/20240216171653_add_index_tsv_to_user_groups.rb

@@ -0,0 +1,13 @@
+class AddIndexTsvToUserGroups < ActiveRecord::Migration[6.1]
+  # Adding an index non-concurrently blocks writes. Therefore we need to disable ddl transaction
+
+  disable_ddl_transaction!
+
+  def up
+    add_index :user_groups, :tsv, using: "gin", algorithm: :concurrently
+  end
+
+  def down
+    remove_index :user_groups, :tsv
+  end
+end

db/migrate/20240216171937_create_sql_trigger_to_update_tsv_vector.rb

@@ -0,0 +1,24 @@
+class CreateSqlTriggerToUpdateTsvVector < ActiveRecord::Migration[6.1]
+  disable_ddl_transaction!
+
+  def up
+    safety_assured {
+      execute <<-SQL
+        CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE
+        ON user_groups FOR EACH ROW EXECUTE PROCEDURE
+        tsvector_update_trigger(
+          tsv, 'pg_catalog.english', display_name
+        );
+      SQL
+    }
+  end
+
+  def down
+    safety_assured {
+      execute <<-SQL
+        DROP TRIGGER tsvectorupdate
+        ON user_groups;
+      SQL
+    }
+  end
+end

db/structure.sql

@@ -1668,7 +1668,8 @@ CREATE TABLE public.user_groups (
     private boolean DEFAULT true NOT NULL,
     lock_version integer DEFAULT 0,
     join_token character varying,
-    stats_visibility integer DEFAULT 0
+    stats_visibility integer DEFAULT 0,
+    tsv tsvector
 );
 
 
@@ -3539,6 +3540,13 @@ CREATE UNIQUE INDEX index_user_groups_on_name ON public.user_groups USING btree
 CREATE INDEX index_user_groups_on_private ON public.user_groups USING btree (private);
 
 
+--
+-- Name: index_user_groups_on_tsv; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_user_groups_on_tsv ON public.user_groups USING gin (tsv);
+
+
 --
 -- Name: index_user_project_preferences_on_project_id_and_user_id; Type: INDEX; Schema: public; Owner: -
 --
@@ -3763,6 +3771,13 @@ CREATE INDEX users_idx_trgm_login ON public.users USING gin (COALESCE((login)::t
 CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON public.projects FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('tsv', 'pg_catalog.english', 'display_name');
 
 
+--
+-- Name: user_groups tsvectorupdate; Type: TRIGGER; Schema: public; Owner: -
+--
+
+CREATE TRIGGER tsvectorupdate BEFORE INSERT OR UPDATE ON public.user_groups FOR EACH ROW EXECUTE PROCEDURE tsvector_update_trigger('tsv', 'pg_catalog.english', 'display_name');
+
+
 --
 -- Name: users tsvectorupdate; Type: TRIGGER; Schema: public; Owner: -
 --
@@ -4592,6 +4607,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20221018032140'),
 ('20230613165746'),
 ('20231025200957'),
-('20240304201959');
-
-
+('20240216142515'),
+('20240216171653'),
+('20240216171937'),
+('20240304201959');

lib/project_copier.rb

@@ -1,7 +1,17 @@
 class ProjectCopier
   attr_reader :project_to_copy, :user
 
-  EXCLUDE_ATTRIBUTES = %i[classifications_count launched_row_order beta_row_order].freeze
+  EXCLUDE_ATTRIBUTES = %i[
+    classifications_count
+    classifiers_count
+    launch_date
+    completeness
+    activity
+    lock_version
+    launched_row_order
+    beta_row_order
+  ].freeze
+
   INCLUDE_ASSOCIATIONS = [
     :tutorials,
     :pages,

lib/subjects/remover.rb

@@ -1,10 +1,11 @@
 module Subjects
   class Remover
-    attr_reader :subject_id, :panoptes_client
+    attr_reader :subject_id, :panoptes_client, :subject_set_id
 
-    def initialize(subject_id, client=nil)
+    def initialize(subject_id, client=nil, subject_set_id=nil)
       @subject_id = subject_id
       @panoptes_client = client || Panoptes::Client.new(env: Rails.env)
+      @subject_set_id = subject_set_id
     end
 
     def cleanup
@@ -29,6 +30,8 @@ def cleanup
     private
 
     def can_be_removed?
+      return false if belongs_to_other_subject_set?
+
       return false if has_been_collected_or_classified?
 
       return false if has_been_talked_about?
@@ -56,6 +59,12 @@ def has_been_collected_or_classified?
       !orphan_subject
     end
 
+    def belongs_to_other_subject_set?
+      return false if subject_set_id.nil?
+
+      orphan_subject.set_member_subjects.where.not(subject_set_id: subject_set_id).count.positive?
+    end
+
     def has_been_talked_about?
       panoptes_client.discussions(
         focus_id: subject_id,

lib/tasks/user_groups.rake

@@ -8,4 +8,12 @@ namespace :user_groups do
       UserGroup.where(id: user_group_ids_to_update).update_all(stats_visibility: 0)
     end
   end
+
+  desc 'Touch user_group updated_at'
+  task touch_user_group_updated_at: :environment do
+    UserGroup.select(:id).find_in_batches do |user_groups|
+      user_group_ids_to_update = user_groups.map(&:id)
+      UserGroup.where(id: user_group_ids_to_update).update_all(updated_at: Time.current.to_s(:db))
+    end
+  end
 end

spec/controllers/api/v1/subject_sets_controller_spec.rb

@@ -410,13 +410,13 @@
 
     it 'calls the subject removal worker' do
       subject_set.subjects.each_with_index do |s, index|
-        allow(SubjectRemovalWorker).to receive(:perform_in).with(index.seconds, s.id)
+        allow(SubjectRemovalWorker).to receive(:perform_in).with(index.seconds, s.id, subject_set.id)
       end
       delete_resources
       subject_set.subjects.each_with_index do |s, index|
         expect(SubjectRemovalWorker)
           .to have_received(:perform_in)
-          .with(index.seconds, s.id)
+          .with(index.seconds, s.id, subject_set.id)
       end
     end
   end

spec/controllers/api/v1/user_groups_controller_spec.rb

@@ -49,6 +49,49 @@
       end
     end
 
+    describe 'search' do
+      let(:user_group_with_uniq_name) { create(:user_group, private: false, display_name: 'My Unique Group') }
+
+      before do
+        # force an update of all user_groups to set the tsv column
+        user_group_with_uniq_name.reload
+        user_groups.each(&:reload)
+      end
+
+      it 'returns exact matched user_group' do
+        get :index, params: { search: user_group_with_uniq_name.display_name }
+        expect(json_response[api_resource_name].length).to eq(1)
+        expect(json_response[api_resource_name][0]['id']).to eq(user_group_with_uniq_name.id.to_s)
+      end
+
+      it 'returns no user_groups if search is < than 3 chars' do
+        get :index, params: { search: 'my' }
+        expect(json_response[api_resource_name].length).to eq(0)
+      end
+
+      it 'does a full text search on display_name when no exact match' do
+        get :index, params: { search: 'my uniq' }
+        expect(json_response[api_resource_name].length).to eq(1)
+        expect(json_response[api_resource_name][0]['id']).to eq(user_group_with_uniq_name.id.to_s)
+      end
+
+      it 'does a full text search on display_name on public and accessible user_groups' do
+        get :index, params: { search: 'group' }
+        # returns user_group_with_users, user_group_with_uniq_name, the public user_group
+        expect(json_response[api_resource_name].length).to eq(3)
+      end
+
+      describe 'as admin' do
+        it 'does a full text search against display_name on private and public user_groups' do
+          admin_user = create(:user, admin: true)
+          default_request scopes: scopes, user_id: admin_user.id
+          get :index, params: { search: 'group', admin: true }
+          # returns all 5 user groups
+          expect(json_response[api_resource_name].length).to eq(5)
+        end
+      end
+    end
+
     context 'with no filters' do
       it_behaves_like 'is indexable'
     end

spec/controllers/api/v1/users_controller_spec.rb

@@ -482,6 +482,13 @@ def update_request
         expect(UserInfoChangedMailerWorker).to receive(:perform_async).with(user.id, "email")
       end
 
+      it "sets valid_email parameter to true" do
+        # updating the valid email to false before the request as it is set at true by default
+        user.update(valid_email: false)
+        update_request
+        expect(user.reload.valid_email).to be_truthy
+      end
+
       describe "with an email that already exists" do
         let(:put_operations) { {users: {email: User.where.not(id: user.id).first.email}} }
         it "doesn't send an email to the new address if user is not valid" do

spec/lib/project_copier_spec.rb

@@ -41,6 +41,14 @@
       expect(copied_project.configuration['source_project_id']).to be(project.id)
     end
 
+    it 'does not copy over excluded attributes' do
+      project_with_excluded_keys = create(:full_project, classifications_count: 3, classifiers_count: 2, launch_date: Date.yesterday, completeness: 0.5, activity: 1, lock_version: 8)
+      other_copied_project = described_class.new(project_with_excluded_keys.id, copyist.id).copy
+      ProjectCopier::EXCLUDE_ATTRIBUTES.each do |attr|
+        expect(other_copied_project[attr]).not_to eq(project_with_excluded_keys[attr])
+      end
+    end
+
     it 'creates Talk roles for the new project and its owner' do
       allow(TalkAdminCreateWorker).to receive(:perform_async)
       copied_project

spec/lib/subjects/remover_spec.rb

@@ -105,6 +105,17 @@
         expect { SetMemberSubject.find(sms_ids) }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
+      context 'with multiple subject sets' do
+        let(:alternate_subject_set) { create(:subject_set) }
+        let(:remover) { described_class.new(subject.id, panoptes_client, alternate_subject_set.id) }
+        let(:new_sms) { create(:set_member_subject, subject: subject, subject_set: alternate_subject_set) }
+
+        it 'does not remove subjects associated with multiple set_member_subjects' do
+          remover.cleanup
+          expect(Subject.where(id: subject.id)).to exist
+        end
+      end
+
       it "should remove the associated media resources" do
         locations = subjects.map { |s| create(:medium, linked: s) }
         media_ids = subject.reload.locations.map(&:id)