From 990fe0563a1d74737259c46308ae9349e7283bc6 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Mon, 29 Apr 2024 11:06:30 -0500 Subject: [PATCH 1/8] allow setting private attribute of user_groups (#4320) --- app/controllers/api/v1/user_groups_controller.rb | 4 ++-- spec/controllers/api/v1/user_groups_controller_spec.rb | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/v1/user_groups_controller.rb b/app/controllers/api/v1/user_groups_controller.rb index 9fa8194a2..5137b84b1 100644 --- a/app/controllers/api/v1/user_groups_controller.rb +++ b/app/controllers/api/v1/user_groups_controller.rb @@ -9,8 +9,8 @@ class Api::V1::UserGroupsController < Api::ApiController alias_method :user_group, :controlled_resource - allowed_params :create, :name, :display_name, :stats_visibility, links: [users: []] - allowed_params :update, :name, :stats_visibility, :display_name + allowed_params :create, :name, :display_name, :private, :stats_visibility, links: [users: []] + allowed_params :update, :name, :private, :stats_visibility, :display_name search_by do |name, query| search_names = name.join(' ').downcase diff --git a/spec/controllers/api/v1/user_groups_controller_spec.rb b/spec/controllers/api/v1/user_groups_controller_spec.rb index 92d50cdfb..9883578ab 100644 --- a/spec/controllers/api/v1/user_groups_controller_spec.rb +++ b/spec/controllers/api/v1/user_groups_controller_spec.rb @@ -106,7 +106,8 @@ let(:update_params) do { user_groups: { - display_name: 'A-Different-Name' + display_name: 'A-Different-Name', + private: false } } end @@ -193,7 +194,7 @@ describe '#create' do let(:test_attr) { :name } let(:test_attr_value) { 'Zooniverse' } - let(:create_params) { { user_groups: { name: 'Zooniverse' } } } + let(:create_params) { { user_groups: { name: 'Zooniverse', private: false } } } it_behaves_like 'is creatable' From eac5139c986cea5d3238742c2666a83f35f3dc36 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Tue, 30 Apr 2024 11:27:50 -0500 Subject: [PATCH 2/8] switch order within can_be_removed to ensure we have orphan subject before querying for attributes of orphan subject (#4321) --- lib/subjects/remover.rb | 4 ++-- spec/lib/subjects/remover_spec.rb | 32 +++++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lib/subjects/remover.rb b/lib/subjects/remover.rb index 68262698d..17c9592be 100644 --- a/lib/subjects/remover.rb +++ b/lib/subjects/remover.rb @@ -30,10 +30,10 @@ 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 belongs_to_other_subject_set? + return false if has_been_talked_about? return false if has_been_counted_or_retired? diff --git a/spec/lib/subjects/remover_spec.rb b/spec/lib/subjects/remover_spec.rb index 4dfe67f36..ccbf186dd 100644 --- a/spec/lib/subjects/remover_spec.rb +++ b/spec/lib/subjects/remover_spec.rb @@ -105,14 +105,30 @@ 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 + context 'when subject_set_id is param in init' do + let(:remover_with_subject_set) { + described_class.new(subject.id, panoptes_client, subject_set.id) + } + + it 'removes a subject that has not been used' do + remover_with_subject_set.cleanup + expect { Subject.find(subject.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'does not remove a subject that has been classified' do + create(:classification, subjects: [subject]) + expect(remover_with_subject_set.cleanup).to be_falsey + 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 end From 1bc7a96a6a7327333e437c29d2d7dfd6ec6b2284 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Wed, 15 May 2024 17:11:31 -0500 Subject: [PATCH 3/8] Update workflow_serializer.rb (#4327) --- app/serializers/workflow_serializer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serializers/workflow_serializer.rb b/app/serializers/workflow_serializer.rb index 1e361fa7e..dbb9bc022 100644 --- a/app/serializers/workflow_serializer.rb +++ b/app/serializers/workflow_serializer.rb @@ -18,7 +18,7 @@ class WorkflowSerializer can_filter_by :active, :mobile_friendly - can_sort_by :completeness + can_sort_by :completeness, :id preload :subject_sets, :attached_images, :classifications_export, :published_version From 7db6beb1b2a46d398526283a48dfd87931bda68c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 17 May 2024 10:35:04 -0500 Subject: [PATCH 4/8] Bump rexml from 3.2.5 to 3.2.8 (#4329) Bumps [rexml](https://github.com/ruby/rexml) from 3.2.5 to 3.2.8. - [Release notes](https://github.com/ruby/rexml/releases) - [Changelog](https://github.com/ruby/rexml/blob/master/NEWS.md) - [Commits](https://github.com/ruby/rexml/compare/v3.2.5...v3.2.8) --- updated-dependencies: - dependency-name: rexml dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- Gemfile.lock | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index cf5998618..21c0d2371 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -402,7 +402,8 @@ GEM http-cookie (>= 1.0.2, < 2.0) mime-types (>= 1.16, < 4.0) netrc (~> 0.8) - rexml (3.2.5) + rexml (3.2.8) + strscan (>= 3.0.9) rspec (3.12.0) rspec-core (~> 3.12.0) rspec-expectations (~> 3.12.0) @@ -487,6 +488,7 @@ GEM stringex (2.8.6) strong_migrations (1.4.4) activerecord (>= 5.2) + strscan (3.1.0) ten_years_rails (0.2.0) actionview activesupport From 2bc258375caaaecc3c61a96395455664e3adef13 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Mon, 20 May 2024 10:12:29 -0500 Subject: [PATCH 5/8] Querying Memberships Fix: ensure distinct memberships when querying for ones own public memberships (#4333) * ensure unique memberships when querying for ones own public memberships * update membership policy spec with clearer variable names and spec clean up * hound remove extra spacing * Update spec/policies/membership_policy_spec.rb Co-authored-by: Zach Wolfenbarger * Update spec/policies/membership_policy_spec.rb Co-authored-by: Zach Wolfenbarger * update membership policy spec spacing format when expecting a long list of memberships --------- Co-authored-by: Zach Wolfenbarger --- app/policies/membership_policy.rb | 2 +- spec/policies/membership_policy_spec.rb | 107 ++++++++++++++++-------- 2 files changed, 74 insertions(+), 35 deletions(-) diff --git a/app/policies/membership_policy.rb b/app/policies/membership_policy.rb index e22c5b3a9..6e46d8842 100644 --- a/app/policies/membership_policy.rb +++ b/app/policies/membership_policy.rb @@ -20,7 +20,7 @@ def resolve(action) when :show, :index public_memberships = scope.where(identity: false, state: Membership.states[:active]) .joins(:user_group).merge(UserGroup.where(private: false)) - Membership.union_all(query, public_memberships) + Membership.union_all(query, public_memberships).distinct else query end diff --git a/spec/policies/membership_policy_spec.rb b/spec/policies/membership_policy_spec.rb index 292d1dff6..6ef0ac79b 100644 --- a/spec/policies/membership_policy_spec.rb +++ b/spec/policies/membership_policy_spec.rb @@ -18,6 +18,11 @@ MembershipPolicy::Scope.new(api_user, Membership) end + let(:group_admin_public_membership) { Membership.where(user_id: group_admin.id, user_group_id: public_user_group.id, state: 'active')[0] } + let(:group_admin_private_membership) { + Membership.where(user_id: group_admin.id, user_group_id: private_user_group.id, state: 'active')[0] + } + context 'an anonymous user' do let(:api_user) { ApiUser.new(anonymous_user) } @@ -31,44 +36,58 @@ context 'a normal user', :aggregate_failures do let(:api_user) { ApiUser.new(logged_in_user) } + let(:other_user) { create(:user) } + let(:other_user_public_membership) { create(:membership, user: other_user, user_group: public_user_group) } + let(:other_user_private_membership) { create(:membership, user: other_user, user_group: private_user_group) } + let(:logged_in_user_public_membership) { create(:membership, user: logged_in_user, user_group: public_user_group) } + let(:logged_in_user_private_membership) { create(:membership, user: logged_in_user, user_group: private_user_group) } it 'can see other people in public groups' do - other_user = create :user - membership1 = create :membership, user: other_user, user_group: public_user_group - membership2 = create :membership, user: other_user, user_group: private_user_group - membership3 = create :membership, user_group: public_user_group, state: :inactive - - expect(scope.resolve(:index)).to include(membership1) - expect(scope.resolve(:show)).to include(membership1) - expect(scope.resolve(:index)).not_to include(membership2) - expect(scope.resolve(:show)).not_to include(membership2) - expect(scope.resolve(:index)).not_to include(membership3) - expect(scope.resolve(:show)).not_to include(membership3) + inactive_membership = create :membership, user_group: public_user_group, state: :inactive + + expect(scope.resolve(:index)).to contain_exactly(other_user_public_membership, group_admin_public_membership) + expect(scope.resolve(:show)).to contain_exactly(other_user_public_membership, group_admin_public_membership) + expect(scope.resolve(:index)).not_to include(other_user_private_membership) + expect(scope.resolve(:show)).not_to include(other_user_private_membership) + expect(scope.resolve(:index)).not_to include(inactive_membership) + expect(scope.resolve(:show)).not_to include(inactive_membership) end it 'can see who are members of private groups they are in' do - other_user = create :user - membership1 = create :membership, user: logged_in_user, user_group: public_user_group - membership2 = create :membership, user: logged_in_user, user_group: private_user_group - membership3 = create :membership, user: other_user, user_group: public_user_group - membership4 = create :membership, user: other_user, user_group: private_user_group - - expect(scope.resolve(:index)).to include(membership1, membership2, membership3, membership4) - expect(scope.resolve(:show)).to include(membership1, membership2, membership3, membership4) + expect(scope.resolve(:index)).to contain_exactly( + logged_in_user_public_membership, + logged_in_user_private_membership, + other_user_public_membership, + other_user_private_membership, + group_admin_public_membership, + group_admin_private_membership + ) + expect(scope.resolve(:show)).to contain_exactly( + logged_in_user_public_membership, + logged_in_user_private_membership, + other_user_public_membership, + other_user_private_membership, + group_admin_public_membership, + group_admin_private_membership + ) end it "cannot modify other user's memberships" do - other_user = create :user - membership1 = create :membership, user: logged_in_user, user_group: public_user_group - membership2 = create :membership, user: logged_in_user, user_group: private_user_group - membership3 = create :membership, user: other_user, user_group: public_user_group - membership4 = create :membership, user: other_user, user_group: private_user_group - - expect(scope.resolve(:update)).to include(membership1, membership2) - expect(scope.resolve(:destroy)).to include(membership1, membership2) - - expect(scope.resolve(:update)).not_to include(membership3, membership4) - expect(scope.resolve(:destroy)).not_to include(membership3, membership4) + expect(scope.resolve(:update)).to include(logged_in_user_public_membership, logged_in_user_private_membership) + expect(scope.resolve(:destroy)).to include(logged_in_user_public_membership, logged_in_user_private_membership) + + expect(scope.resolve(:update)).not_to include( + other_user_private_membership, + other_user_public_membership, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:destroy)).not_to include( + other_user_private_membership, + other_user_public_membership, + group_admin_private_membership, + group_admin_public_membership + ) end end @@ -79,10 +98,30 @@ membership1 = create :membership, user: logged_in_user, user_group: public_user_group membership2 = create :membership, user: logged_in_user, user_group: private_user_group - expect(scope.resolve(:index)).to include(membership1, membership2) - expect(scope.resolve(:show)).to include(membership1, membership2) - expect(scope.resolve(:update)).to include(membership1, membership2) - expect(scope.resolve(:destroy)).to include(membership1, membership2) + expect(scope.resolve(:index)).to contain_exactly( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:show)).to contain_exactly( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:update)).to include( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) + expect(scope.resolve(:destroy)).to include( + membership1, + membership2, + group_admin_private_membership, + group_admin_public_membership + ) end end end From 7375ca807dc9cd2f70f35ab7ebcdcb872e5a3a68 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Mon, 20 May 2024 12:06:45 -0500 Subject: [PATCH 6/8] allow updating of membership roles via memberships controller (#4335) * allow updating of membership roles * update spacing and " to ' found by hound sniffs --- .../api/v1/memberships_controller.rb | 2 +- .../api/v1/memberships_controller_spec.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v1/memberships_controller.rb b/app/controllers/api/v1/memberships_controller.rb index c46ac7af4..fc4a6a325 100644 --- a/app/controllers/api/v1/memberships_controller.rb +++ b/app/controllers/api/v1/memberships_controller.rb @@ -6,7 +6,7 @@ class Api::V1::MembershipsController < Api::ApiController resource_actions :index, :show, :create, :update, :deactivate schema_type :strong_params - allowed_params :update, :state + allowed_params :update, :state, roles: [] def create resources = resource_class.transaction(requires_new: true) do diff --git a/spec/controllers/api/v1/memberships_controller_spec.rb b/spec/controllers/api/v1/memberships_controller_spec.rb index a39289ff6..a6ebe9d9f 100644 --- a/spec/controllers/api/v1/memberships_controller_spec.rb +++ b/spec/controllers/api/v1/memberships_controller_spec.rb @@ -35,6 +35,23 @@ end it_behaves_like "is updatable" + + context 'when updating roles as group_admin' do + let(:other_user) { create(:user) } + let(:other_user_group) { create(:user_group, admin: authorized_user, private: true) } + let(:other_user_membership) { create(:membership, user: other_user, user_group: other_user_group, roles: ['group_member']) } + + it 'allows update of membership roles' do + default_request user_id: authorized_user.id, scopes: scopes + params = { + memberships: { roles: ['group_admin'] }, + id: other_user_membership.id + } + + put :update, params: params + expect(other_user_membership.reload.roles).to eq(['group_admin']) + end + end end describe "#create" do From d9e6c275ba39a67a985e12b018b13256fcc33d52 Mon Sep 17 00:00:00 2001 From: yuenmichelle1 Date: Mon, 20 May 2024 14:04:51 -0500 Subject: [PATCH 7/8] Clear unsubscribe token on user deactivation (#4328) * update user info scrubber to nil out unsubscribe_token * add rake task to scrub unsubscribe token of already deactivated users --- app/models/user.rb | 2 +- lib/tasks/user.rake | 8 ++++++++ lib/user_info_scrubber.rb | 1 + spec/lib/user_info_scrubber_spec.rb | 4 ++++ 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 384064897..b620d8fcb 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,7 +50,7 @@ class User < ApplicationRecord validates :login, presence: true, format: { with: USER_LOGIN_REGEX } validates_uniqueness_of :login, case_sensitive: false validates :display_name, presence: true - validates :unsubscribe_token, presence: true, uniqueness: true + validates :unsubscribe_token, presence: true, uniqueness: true, on: :create validates_inclusion_of :valid_email, in: [true, false], message: "must be true or false" validates_with IdentityGroupNameValidator diff --git a/lib/tasks/user.rake b/lib/tasks/user.rake index ab6be1f67..5622ea90b 100644 --- a/lib/tasks/user.rake +++ b/lib/tasks/user.rake @@ -11,6 +11,14 @@ namespace :user do end end + desc 'Scrub unsubscribe_token of deactivated users' + task scrub_inactive_user_unsubscribe_token: :environment do + User.where(activated_state: 'inactive').select(:id).find_in_batches do |users| + ids = users.map(&:id) + User.where(id: ids).update_all(unsubscribe_token: nil) + end + end + desc 'Backfill UX testing emails comms field in batches' task backfill_ux_testing_email_field: :environment do User.select(:id).find_in_batches do |users| diff --git a/lib/user_info_scrubber.rb b/lib/user_info_scrubber.rb index 1a0fe2d09..a42b23692 100644 --- a/lib/user_info_scrubber.rb +++ b/lib/user_info_scrubber.rb @@ -29,6 +29,7 @@ def self.scrub_details(user) user.valid_email = false user.private_profile = true user.api_key = nil + user.unsubscribe_token = nil user.tsv = nil user.save! user diff --git a/spec/lib/user_info_scrubber_spec.rb b/spec/lib/user_info_scrubber_spec.rb index fc8781d1d..64a65cf59 100644 --- a/spec/lib/user_info_scrubber_spec.rb +++ b/spec/lib/user_info_scrubber_spec.rb @@ -33,6 +33,10 @@ def scrub_user_details(user)(user) expect { described_class.scrub_personal_info!(user) }.to change(user, :credited_name).to(nil) end + it 'scrubs the unsubscribe_token' do + expect { described_class.scrub_personal_info!(user) }.to change(user, :unsubscribe_token).to(nil) + end + it 'scrubs the encrypted_password' do expect { described_class.scrub_personal_info!(user) }.to change(user, :encrypted_password) end From 7e6b449d9f96eb481fb4512def5f856d93cdd7ed Mon Sep 17 00:00:00 2001 From: Oluwatoyosi Oyegoke <34948675+Tooyosi@users.noreply.github.com> Date: Wed, 22 May 2024 16:34:10 +0100 Subject: [PATCH 8/8] append current time to copied project display name (#4331) * append current time to copied project display name * hound-fix: switch double to single quotes * add copy with timestamp for all copied projects * include fix in tests --- lib/project_copier.rb | 4 ++-- spec/lib/project_copier_spec.rb | 10 +++------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/project_copier.rb b/lib/project_copier.rb index 3edd513ea..e6f458917 100644 --- a/lib/project_copier.rb +++ b/lib/project_copier.rb @@ -11,7 +11,7 @@ class ProjectCopier launched_row_order beta_row_order ].freeze - + INCLUDE_ASSOCIATIONS = [ :tutorials, :pages, @@ -61,7 +61,7 @@ def copy_project copied_project = project_to_copy.deep_clone include: INCLUDE_ASSOCIATIONS, except: EXCLUDE_ATTRIBUTES copied_project.owner = user - copied_project.display_name += ' (copy)' if user == project_to_copy.owner + copied_project.display_name += " (copy: #{Time.now.utc.strftime('%Y-%m-%d %H:%M:%S')})" copied_project.assign_attributes(launch_approved: false, live: false) # reset the project's configuration but record the source project id diff --git a/spec/lib/project_copier_spec.rb b/spec/lib/project_copier_spec.rb index 46cf657f8..407efa56b 100644 --- a/spec/lib/project_copier_spec.rb +++ b/spec/lib/project_copier_spec.rb @@ -16,13 +16,9 @@ expect(copied_project.owner).to eq(copyist) end - it 'renames a project when the owner is copying their own project' do - new_copy = described_class.new(project.id, project.owner.id).copy - expect(new_copy.display_name).to include('(copy)') - end - - it 'has matching attributes' do - expect(copied_project.display_name).to eq(project.display_name) + it 'appends copy and current timestamp to display_name' do + allow(Time).to receive(:now).and_return(Time.utc(2024, 5, 17, 12, 0, 0)) + expect(copied_project.display_name).to eq("#{project.display_name} (copy: 2024-05-17 12:00:00)") end it 'resets the live attribute' do