From b91a4c27344c4cf89592e52c1d3977e3c29d966e Mon Sep 17 00:00:00 2001 From: Oluwatoyosi Oyegoke <34948675+Tooyosi@users.noreply.github.com> Date: Wed, 6 Mar 2024 11:41:26 +0000 Subject: [PATCH 1/4] set valid_email to true on user email change (#4292) * set valid_email to true on user email change * set valid_email to false before update_request in test --- app/controllers/api/v1/users_controller.rb | 1 + spec/controllers/api/v1/users_controller_spec.rb | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 672239451..5ea2507f1 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/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 diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb index 6e042757c..d28cb8585 100644 --- a/spec/controllers/api/v1/users_controller_spec.rb +++ b/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 From 23605b0af05cafad8bfd081ef74096bb122eb15e Mon Sep 17 00:00:00 2001 From: Cliff Johnson Date: Tue, 12 Mar 2024 13:18:32 -0500 Subject: [PATCH 2/4] Project Copier: tweaks to initialization (#4270) * Add more fields to EXCLUDE_ATTRIBUTES * add tests for the EXCLUDE_ATTRIBUTES functionality * remove unused attribute * fix default_array declaration * refactor project_copier exclude_attributes tests * refactor project_copier exclude_attributes tests --------- Co-authored-by: tooyosi --- lib/project_copier.rb | 12 +++++++++++- spec/lib/project_copier_spec.rb | 8 ++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/project_copier.rb b/lib/project_copier.rb index a4bbc5ed0..3edd513ea 100644 --- a/lib/project_copier.rb +++ b/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, diff --git a/spec/lib/project_copier_spec.rb b/spec/lib/project_copier_spec.rb index 4a01ceaf2..46cf657f8 100644 --- a/spec/lib/project_copier_spec.rb +++ b/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 From a637729d0084b1832fb9e19b4fc3874f8d293fb5 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Mon, 18 Mar 2024 12:57:01 -0500 Subject: [PATCH 3/4] Adjust User group search to mimic user search (#4291) * have user_group search mimic that of user search * add specs for user_group search functionality and downcase the search_names string * rubocop fixes of spec * rubocop fixes on user_group model and user_groups_controller_spec. rename specs with shorter name * Update user_groups_controller.rb * remove accidental indentation on up --- .../api/v1/user_groups_controller.rb | 11 ++++- app/models/user_group.rb | 39 +++++++++++------ .../20240216142515_add_tsv_to_user_groups.rb | 9 ++++ ...0216171653_add_index_tsv_to_user_groups.rb | 13 ++++++ ...create_sql_trigger_to_update_tsv_vector.rb | 24 +++++++++++ db/structure.sql | 24 ++++++++++- lib/tasks/user_groups.rake | 8 ++++ .../api/v1/user_groups_controller_spec.rb | 43 +++++++++++++++++++ 8 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20240216142515_add_tsv_to_user_groups.rb create mode 100644 db/migrate/20240216171653_add_index_tsv_to_user_groups.rb create mode 100644 db/migrate/20240216171937_create_sql_trigger_to_update_tsv_vector.rb diff --git a/app/controllers/api/v1/user_groups_controller.rb b/app/controllers/api/v1/user_groups_controller.rb index 1234d486b..9fa8194a2 100644 --- a/app/controllers/api/v1/user_groups_controller.rb +++ b/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 diff --git a/app/models/user_group.rb b/app/models/user_group.rb index a9a81882b..fab426591 100644 --- a/app/models/user_group.rb +++ b/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 diff --git a/db/migrate/20240216142515_add_tsv_to_user_groups.rb b/db/migrate/20240216142515_add_tsv_to_user_groups.rb new file mode 100644 index 000000000..c8c42d29d --- /dev/null +++ b/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 diff --git a/db/migrate/20240216171653_add_index_tsv_to_user_groups.rb b/db/migrate/20240216171653_add_index_tsv_to_user_groups.rb new file mode 100644 index 000000000..a74b36571 --- /dev/null +++ b/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 diff --git a/db/migrate/20240216171937_create_sql_trigger_to_update_tsv_vector.rb b/db/migrate/20240216171937_create_sql_trigger_to_update_tsv_vector.rb new file mode 100644 index 000000000..5ac99888e --- /dev/null +++ b/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 diff --git a/db/structure.sql b/db/structure.sql index 0c3e5dc97..dcf97baca 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -53,6 +53,8 @@ COMMENT ON EXTENSION pg_trgm IS 'text similarity measurement and index searching SET default_tablespace = ''; +SET default_with_oids = false; + -- -- Name: access_control_lists; Type: TABLE; Schema: public; Owner: - -- @@ -1663,7 +1665,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 ); @@ -3541,6 +3544,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: - -- @@ -3765,6 +3775,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: - -- @@ -4585,6 +4602,9 @@ INSERT INTO "schema_migrations" (version) VALUES ('20211201164326'), ('20221018032140'), ('20230613165746'), -('20231025200957'); +('20231025200957'), +('20240216142515'), +('20240216171653'), +('20240216171937'); diff --git a/lib/tasks/user_groups.rake b/lib/tasks/user_groups.rake index 3b1581c4c..5e1c9c476 100644 --- a/lib/tasks/user_groups.rake +++ b/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 diff --git a/spec/controllers/api/v1/user_groups_controller_spec.rb b/spec/controllers/api/v1/user_groups_controller_spec.rb index a1c22ce26..92d50cdfb 100644 --- a/spec/controllers/api/v1/user_groups_controller_spec.rb +++ b/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 From 818e0d34ffebb0b72d9ec0252e4059c28db72ce0 Mon Sep 17 00:00:00 2001 From: Oluwatoyosi Oyegoke <34948675+Tooyosi@users.noreply.github.com> Date: Wed, 27 Mar 2024 17:32:01 +0000 Subject: [PATCH 4/4] exclude subjects still linked to set_member_subject in subject_remover (#4306) * exclude subjects still linked to set_member_subject in subject_remover * don't call cleanup if subject is linked to other subject sets in SubjectRemoverWorker * hound cleanups * hound cleanups * implement fix on remover.rb, add new test to remover_spec * hound cleanups * pr review: remove type error in remover.rb and stub response properly * more pr cleanups --- .../api/v1/subject_sets_controller.rb | 2 +- app/workers/subject_removal_worker.rb | 4 +- lib/subjects/remover.rb | 13 ++++- .../api/v1/subject_sets_controller_spec.rb | 4 +- spec/lib/subjects/remover_spec.rb | 11 ++++ spec/workers/subject_removal_worker_spec.rb | 58 ++++++++++++++++--- 6 files changed, 77 insertions(+), 15 deletions(-) diff --git a/app/controllers/api/v1/subject_sets_controller.rb b/app/controllers/api/v1/subject_sets_controller.rb index 601c8cfbb..5684483c2 100644 --- a/app/controllers/api/v1/subject_sets_controller.rb +++ b/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 diff --git a/app/workers/subject_removal_worker.rb b/app/workers/subject_removal_worker.rb index a15eb3462..3a9010ea4 100644 --- a/app/workers/subject_removal_worker.rb +++ b/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 diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb index 7783ecaa4..68262698d 100644 --- a/lib/subjects/remover.rb +++ b/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, diff --git a/spec/controllers/api/v1/subject_sets_controller_spec.rb b/spec/controllers/api/v1/subject_sets_controller_spec.rb index 92cf8cfad..360db0c9c 100644 --- a/spec/controllers/api/v1/subject_sets_controller_spec.rb +++ b/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 diff --git a/spec/lib/subjects/remover_spec.rb b/spec/lib/subjects/remover_spec.rb index e2ca8c837..4dfe67f36 100644 --- a/spec/lib/subjects/remover_spec.rb +++ b/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) diff --git a/spec/workers/subject_removal_worker_spec.rb b/spec/workers/subject_removal_worker_spec.rb index eacf95e9f..202244f15 100644 --- a/spec/workers/subject_removal_worker_spec.rb +++ b/spec/workers/subject_removal_worker_spec.rb @@ -3,18 +3,60 @@ RSpec.describe SubjectRemovalWorker do let(:subject_id) { 1 } let(:feature_name) { "remove_orphan_subjects" } - let(:remover) { instance_double(Subjects::Remover) } + let(:panoptes_client) { instance_double(Panoptes::Client) } + let(:subject_remover) { described_class.new } + let(:remover) { instance_double(Subjects::Remover, panoptes_client: panoptes_client) } - it 'should call the orphan remover cleanup when enabled' do - Flipper.enable(feature_name) - expect(Subjects::Remover).to receive(:new).with(subject_id).and_return(remover) - expect(remover).to receive(:cleanup) - subject.perform(subject_id) + describe 'flipper enabled' do + let(:project) { create(:project) } + let!(:subjects) { create_list(:subject, 2) } + let!(:subject_sets) { create_list(:subject_set, 2, project: project) } + let!(:subject_set) { create(:subject_set, project: project) } + let(:discussions_url) { 'https://talk-staging.zooniverse.org/discussions' } + let!(:first_subject) { subjects.first } + let!(:second_subject) { subjects.second } + + def stub_discussions_request(subject_id) + stub_request(:get, discussions_url) + .with(query: { focus_id: subject_id, focus_type: 'Subject' }) + .to_return(status: 200, body: '{"discussions": []}', headers: {}) + end + + before do + Flipper.enable(feature_name) + end + + context 'when running orphan remover cleanup function' do + it 'calls the orphan remover cleanup when enabled' do + allow(Subjects::Remover).to receive(:new).with(subject_id, nil, nil).and_return(remover) + allow(remover).to receive(:cleanup) + subject_remover.perform(subject_id) + expect(Subjects::Remover).to have_received(:new) + expect(remover).to have_received(:cleanup) + end + end + + context 'when deleting subjects' do + it 'deletes subject not associated with set_member_subject' do + stub_discussions_request(first_subject.id) + subject_remover.perform(first_subject.id, subject_set.id) + expect(Subject.where(id: first_subject.id)).not_to exist + end + + it 'does not delete subjects associated with another set_member_subject' do + create(:set_member_subject, subject: second_subject, subject_set: subject_sets.first) + create(:set_member_subject, subject: second_subject, subject_set: subject_sets.last) + stub_discussions_request(second_subject.id) + subject_remover.perform(second_subject.id, subject_sets.first.id) + expect(Subject.where(id: second_subject.id)).to exist + expect(SetMemberSubject.where(subject_set_id: subject_sets.first.id, subject_id: second_subject.id)).to exist + end + end end it 'should not call the orphan remover cleanup when disabled' do - expect(Subjects::Remover).not_to receive(:new) + allow(Subjects::Remover).to receive(:new) + subject_remover.perform(subject_id) expect(remover).not_to receive(:cleanup) - subject.perform(subject_id) end end