From 1653ba75f07fda2da35b2d7797534095b09db3bc Mon Sep 17 00:00:00 2001 From: Mark Cottman-Fields Date: Sun, 11 Dec 2016 12:57:03 +1000 Subject: [PATCH] ensures admin filter api also respects the project filter for site permissions, closes #306 --- lib/modules/access/by_permission.rb | 32 ++++++------- spec/lib/filter/query_spec.rb | 71 ++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 17 deletions(-) diff --git a/lib/modules/access/by_permission.rb b/lib/modules/access/by_permission.rb index d32e1b8e..2310a003 100644 --- a/lib/modules/access/by_permission.rb +++ b/lib/modules/access/by_permission.rb @@ -146,15 +146,15 @@ def analysis_jobs_items(analysis_job, user, system_mode = false, levels = Access if system_mode query = AnalysisJobsItem.system_query - .joins(audio_recording: :site) - .order(audio_recording_id: :asc) + .joins(audio_recording: :site) + .order(audio_recording_id: :asc) else analysis_job = Access::Core.validate_analysis_job(analysis_job) query = AnalysisJobsItem - .joins(audio_recording: :site) - .order(audio_recording_id: :asc) - .joins(:analysis_job) # this join ensures only non-deleted results are returned - .where(analysis_jobs: {id: analysis_job.id}) + .joins(audio_recording: :site) + .order(audio_recording_id: :asc) + .joins(:analysis_job) # this join ensures only non-deleted results are returned + .where(analysis_jobs: {id: analysis_job.id}) end permission_sites(user, levels, query) @@ -237,7 +237,6 @@ def permission_projects(user, levels) def permission_sites(user, levels, query, project_ids = nil) is_admin, query = permission_admin(user, levels, query) - return query if is_admin =begin EXISTS @@ -252,14 +251,11 @@ def permission_sites(user, levels, query, project_ids = nil) pt = Project.arel_table ps = Arel::Table.new(:projects_sites) st = Site.arel_table + # levels_modified is not used because permission_projects also # calls calculate_levels, which would end up with the inverse results levels_modified, exists = calculate_levels(levels) - # project permission will never be nil, which is the way it should work - # when being used as a part of a subquery rather than the whole subquery - permissions = permission_projects(user, levels) - permissions_by_site = ps .join(pt).on(ps[:project_id].eq(pt[:id])) @@ -272,11 +268,15 @@ def permission_sites(user, levels, query, project_ids = nil) .where(pt[:id].in(project_ids)) end - permissions_by_site = - permissions_by_site - .where(permissions) - .project(1) - .exists + unless is_admin + # project permission will never be nil, which is the way it should work + # when being used as a part of a subquery rather than the whole subquery + permissions = permission_projects(user, levels) + permissions_by_site = permissions_by_site.where(permissions) + end + + permissions_by_site = permissions_by_site.project(1).exists + =begin EXISTS (SELECT 1 diff --git a/spec/lib/filter/query_spec.rb b/spec/lib/filter/query_spec.rb index 4c9822cd..d30bf51b 100644 --- a/spec/lib/filter/query_spec.rb +++ b/spec/lib/filter/query_spec.rb @@ -1413,9 +1413,12 @@ def create_filter(params) expect(ids_actual).to match_array(ids_expected) end - it 'overrides filter parameters that match text partial match field' do + it 'overrides filter parameters that match text partial match field for admin' do + # audio_recording needs a site, otherwise it won't be found + # in by_permission#permission_sites audio_recording = FactoryGirl.create( :audio_recording, + site: Site.first, media_type: 'audio/mp3', recorded_date: '2012-03-26 07:06:59', duration_seconds: 120) @@ -1440,6 +1443,36 @@ def create_filter(params) expect(ids_actual).to match_array(ids_expected) end + it 'overrides filter parameters that match text partial match field for writer' do + # audio_recording needs a site, otherwise it won't be found + # in by_permission#permission_sites + audio_recording = FactoryGirl.create( + :audio_recording, + site: Site.first, + media_type: 'audio/mp3', + recorded_date: '2012-03-26 07:06:59', + duration_seconds: 120) + + filter = Filter::Query.new( + {filter: { + duration_seconds: {eq: 100}, + or: {media_type: {contains: 'wav'}, status: {contains: 'wav'}} + }, + filter_duration_seconds: 120, + filter_partial_match: 'mp3' + }, + Access::ByPermission.audio_recordings(writer_user), + AudioRecording, + AudioRecording.filter_settings + ) + + expect(filter.filter).to eq({duration_seconds: {eq: 120}, or: {media_type: {contains: 'mp3'}, status: {contains: 'mp3'}}}) + + ids_actual = filter.query_full.pluck(:id) + ids_expected = [audio_recording.id] + expect(ids_actual).to match_array(ids_expected) + end + end context 'available filter items' do @@ -1519,4 +1552,40 @@ def create_filter(params) end end + context 'project with no sites' do + it 'returns no sites for admin' do + filter_hash = { filter: { } } + project_id = FactoryGirl.create(:project, creator: admin_user).id + filter_query = Access::ByPermission.sites(admin_user, Access::Core.levels, project_id) + filter = Filter::Query.new( + filter_hash, + filter_query, + Site, + Site.filter_settings + ) + + expect(filter.filter).to eq(filter_hash[:filter]) + + query = filter.query_full + expect(query.pluck(:id)).to match_array([]) + end + + it 'returns no sites for regular user' do + filter_hash = { filter: { } } + project_id = FactoryGirl.create(:project, creator: writer_user).id + filter_query = Access::ByPermission.sites(writer_user, Access::Core.levels, project_id) + filter = Filter::Query.new( + filter_hash, + filter_query, + Site, + Site.filter_settings + ) + + expect(filter.filter).to eq(filter_hash[:filter]) + + query = filter.query_full + expect(query.pluck(:id)).to match_array([]) + end + end + end