From a8d2d0245ed74d163fecf151071b124971b9b5c7 Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Tue, 19 Oct 2021 17:01:22 +1300 Subject: [PATCH 01/10] Allow adding multiple items to multiple stories --- .../supplejack_api/stories_controller.rb | 33 +++++++ config/routes.rb | 2 + .../supplejack_api/stories_controller_spec.rb | 91 +++++++++++++++++++ 3 files changed, 126 insertions(+) diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index d87f45180..a19fd18a8 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -69,8 +69,39 @@ def reposition_items end end + def add_to_stories + changes = add_to_stories_params["stories"].each_with_object([]) do |story, changes| + + set = SupplejackApi::UserSet.custom_find(story['id']) + + return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set + + item_ids = story['items'].each_with_object([]) do |item, ids| + item = set.set_items.build(item) + + if item.valid? + ids.push(item.id) + end + end + + set.save! + + changes.push({ + story_id: story['id'], + item_ids: item_ids + }) + end + + render json: changes + end + private + def add_to_stories_params + # TODO confirm all fields + params.permit(:api_key, stories: [:id, items: [:position, :type, :sub_type, :image_url, :display_collection, :category, :meta, :record_id, content: [:value, :image_url, :display_collection, :category]]]) + end + def story_params fields = [:name, :description, :privacy, :copyright, :cover_thumbnail, { tags: [], subjects: [] }] @@ -107,5 +138,7 @@ def create_story_record_views SupplejackApi::RequestMetric.spawn(log, 'user_story_views') if log end + + end end diff --git a/config/routes.rb b/config/routes.rb index 056cf947d..d23f24e5b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -37,11 +37,13 @@ # Stories namespace 'stories' do resources :featured, only: [:index] + post :add_to_stories end resources :stories, except: [:new, :edit] do post :reposition_items + resources :items, controller: :story_items, except: [:new, :edit] end end diff --git a/spec/controllers/supplejack_api/stories_controller_spec.rb b/spec/controllers/supplejack_api/stories_controller_spec.rb index 024b2a62f..97b1c8642 100644 --- a/spec/controllers/supplejack_api/stories_controller_spec.rb +++ b/spec/controllers/supplejack_api/stories_controller_spec.rb @@ -365,5 +365,96 @@ module SupplejackApi end end end + + describe 'POST add_to_stories' do + let(:user) { create(:user) } + let!(:story_one) { create(:user_set, user_id: user.id, privacy: 'public') } + let!(:story_two) { create(:user_set, user_id: user.id, privacy: 'hidden') } + + let(:story_item_one) { attributes_for(:story_item) } + let(:story_item_two) { attributes_for(:story_item) } + let(:story_item_three) { attributes_for(:story_item) } + let(:story_item_four) { attributes_for(:story_item) } + let(:story_item_five) { attributes_for(:story_item) } + + context 'valid' do + before do + post :add_to_stories, + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + story_item_one, + story_item_two + ] + }, + { + id: story_two.id, + items: [ + story_item_three, + story_item_four, + story_item_five + ] + } + ] + } + end + + it 'adds multiple story items to multiple stories' do + expect(story_one.reload.set_items.count).to eq 2 + expect(story_two.reload.set_items.count).to eq 3 + end + + it 'returns a serialized response with the stories and new items' do + expect(JSON.parse(response.body).count).to eq 2 + + expect(JSON.parse(response.body).first).to have_key 'story_id' + expect(JSON.parse(response.body).first).to have_key 'item_ids' + + expect(JSON.parse(response.body).last).to have_key 'story_id' + expect(JSON.parse(response.body).last).to have_key 'item_ids' + end + end + + context 'invalid' do + it 'returns an error when given an invalid story id' do + post :add_to_stories, + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: 'a', + items: [ + story_item_one, + story_item_two + ] + }, + { + id: story_two.id, + items: [ + story_item_three, + story_item_four, + story_item_five + ] + } + ] + } + + expect(response.status).to eq 404 + end + + it 'returns an error when given an invalid item' do + + end + + it 'returns an error when trying to update stories that you do not have access too' do + + end + end + end end end From 572a8ea6beefe90c436183e0730ba3b76173786a Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 10:30:11 +1300 Subject: [PATCH 02/10] Update to authorize and find all stories before continuing --- .../supplejack_api/stories_controller.rb | 20 ++++---- .../supplejack_api/user_set_policy.rb | 1 + .../supplejack_api/stories_controller_spec.rb | 48 ++++++++++++++++++- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index a19fd18a8..de1159c8e 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -70,24 +70,31 @@ def reposition_items end def add_to_stories - changes = add_to_stories_params["stories"].each_with_object([]) do |story, changes| - + stories = add_to_stories_params['stories'].each_with_object([]) do |story, stories| set = SupplejackApi::UserSet.custom_find(story['id']) - return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set - item_ids = story['items'].each_with_object([]) do |item, ids| + authorize(set) + stories.push(set) + end + + changes = add_to_stories_params['stories'].each_with_object([]) do |story_params, changes| + set = stories.find { |s| s.id.to_s == story_params['id'] } + + item_ids = story_params['items'].each_with_object([]) do |item, ids| item = set.set_items.build(item) if item.valid? ids.push(item.id) + else + return render_error_with(item.errors.messages.values.join(', '), :bad_request) end end set.save! changes.push({ - story_id: story['id'], + story_id: story_params['id'], item_ids: item_ids }) end @@ -95,10 +102,7 @@ def add_to_stories render json: changes end - private - def add_to_stories_params - # TODO confirm all fields params.permit(:api_key, stories: [:id, items: [:position, :type, :sub_type, :image_url, :display_collection, :category, :meta, :record_id, content: [:value, :image_url, :display_collection, :category]]]) end diff --git a/app/policies/supplejack_api/user_set_policy.rb b/app/policies/supplejack_api/user_set_policy.rb index 4903e474e..892481d0b 100644 --- a/app/policies/supplejack_api/user_set_policy.rb +++ b/app/policies/supplejack_api/user_set_policy.rb @@ -20,5 +20,6 @@ def show? end alias update? admin_or_owner? + alias add_to_stories? admin_or_owner? end end diff --git a/spec/controllers/supplejack_api/stories_controller_spec.rb b/spec/controllers/supplejack_api/stories_controller_spec.rb index 97b1c8642..3a8821b3e 100644 --- a/spec/controllers/supplejack_api/stories_controller_spec.rb +++ b/spec/controllers/supplejack_api/stories_controller_spec.rb @@ -367,7 +367,8 @@ module SupplejackApi end describe 'POST add_to_stories' do - let(:user) { create(:user) } + let(:user) { create(:user) } + let(:user_two) { create(:user) } let!(:story_one) { create(:user_set, user_id: user.id, privacy: 'public') } let!(:story_two) { create(:user_set, user_id: user.id, privacy: 'hidden') } @@ -448,11 +449,54 @@ module SupplejackApi end it 'returns an error when given an invalid item' do - + post :add_to_stories, + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + story_item_one, + { + type: 'text', + sub_type: 'heading' + } + ] + } + ] + } + + expect(response.status).to eq 400 + expect(JSON.parse(response.body)['errors']).to eq 'Content value is missing: content must contain value field' end it 'returns an error when trying to update stories that you do not have access too' do + post :add_to_stories, + params: { + api_key: user_two.api_key, + user_key: user_two.api_key, + stories: [ + { + id: story_one.id, + items: [ + story_item_one, + story_item_two + ] + }, + { + id: story_two.id, + items: [ + story_item_three, + story_item_four, + story_item_five + ] + } + ] + } + expect(response.status).to eq 401 + expect(JSON.parse(response.body)['errors']).to eq 'Provided user key is not authorized to access this story' end end end From aa10daec6bba9a8c26f8402e45db3e0f4516fe00 Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 10:56:55 +1300 Subject: [PATCH 03/10] rename add_to_stories to multiple_add --- app/controllers/supplejack_api/stories_controller.rb | 2 +- app/policies/supplejack_api/user_set_policy.rb | 2 +- config/routes.rb | 2 +- .../supplejack_api/stories_controller_spec.rb | 10 +++++----- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index de1159c8e..00a2126b6 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -69,7 +69,7 @@ def reposition_items end end - def add_to_stories + def multiple_add stories = add_to_stories_params['stories'].each_with_object([]) do |story, stories| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set diff --git a/app/policies/supplejack_api/user_set_policy.rb b/app/policies/supplejack_api/user_set_policy.rb index 892481d0b..09ae5d5b3 100644 --- a/app/policies/supplejack_api/user_set_policy.rb +++ b/app/policies/supplejack_api/user_set_policy.rb @@ -20,6 +20,6 @@ def show? end alias update? admin_or_owner? - alias add_to_stories? admin_or_owner? + alias multiple_add? admin_or_owner? end end diff --git a/config/routes.rb b/config/routes.rb index d23f24e5b..677a14fb0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -37,7 +37,7 @@ # Stories namespace 'stories' do resources :featured, only: [:index] - post :add_to_stories + post :multiple_add end resources :stories, except: [:new, :edit] do diff --git a/spec/controllers/supplejack_api/stories_controller_spec.rb b/spec/controllers/supplejack_api/stories_controller_spec.rb index 3a8821b3e..b7238d4f5 100644 --- a/spec/controllers/supplejack_api/stories_controller_spec.rb +++ b/spec/controllers/supplejack_api/stories_controller_spec.rb @@ -366,7 +366,7 @@ module SupplejackApi end end - describe 'POST add_to_stories' do + describe 'POST multiple_add' do let(:user) { create(:user) } let(:user_two) { create(:user) } let!(:story_one) { create(:user_set, user_id: user.id, privacy: 'public') } @@ -380,7 +380,7 @@ module SupplejackApi context 'valid' do before do - post :add_to_stories, + post :multiple_add, params: { api_key: user.api_key, user_key: user.api_key, @@ -422,7 +422,7 @@ module SupplejackApi context 'invalid' do it 'returns an error when given an invalid story id' do - post :add_to_stories, + post :multiple_add, params: { api_key: user.api_key, user_key: user.api_key, @@ -449,7 +449,7 @@ module SupplejackApi end it 'returns an error when given an invalid item' do - post :add_to_stories, + post :multiple_add, params: { api_key: user.api_key, user_key: user.api_key, @@ -472,7 +472,7 @@ module SupplejackApi end it 'returns an error when trying to update stories that you do not have access too' do - post :add_to_stories, + post :multiple_add, params: { api_key: user_two.api_key, user_key: user_two.api_key, From ed981d6d584684e2e0a9718933ccb2d0ff999034 Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 12:09:07 +1300 Subject: [PATCH 04/10] Multiple delete --- .../supplejack_api/stories_controller.rb | 37 ++++- .../supplejack_api/user_set_policy.rb | 1 + config/routes.rb | 1 + .../supplejack_api/stories_controller_spec.rb | 133 ++++++++++++++++++ 4 files changed, 168 insertions(+), 4 deletions(-) diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index 00a2126b6..3aa5a1ebf 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -70,7 +70,7 @@ def reposition_items end def multiple_add - stories = add_to_stories_params['stories'].each_with_object([]) do |story, stories| + stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set @@ -78,7 +78,7 @@ def multiple_add stories.push(set) end - changes = add_to_stories_params['stories'].each_with_object([]) do |story_params, changes| + changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes| set = stories.find { |s| s.id.to_s == story_params['id'] } item_ids = story_params['items'].each_with_object([]) do |item, ids| @@ -102,8 +102,37 @@ def multiple_add render json: changes end - def add_to_stories_params - params.permit(:api_key, stories: [:id, items: [:position, :type, :sub_type, :image_url, :display_collection, :category, :meta, :record_id, content: [:value, :image_url, :display_collection, :category]]]) + def multiple_remove + + stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories| + set = SupplejackApi::UserSet.custom_find(story['id']) + return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set + + authorize(set) + stories.push(set) + end + + multiple_stories_params['stories'].each do |story_params| + set = stories.find { |s| s.id.to_s == story_params['id'] } + + story_params['items'].each do |item| + set_item = set.set_items.find_by_id(item[:id]) + + if set_item + set_item.destroy! + else + return render json: { errors: I18n.t('errors.record_not_found', id: item[:id]) }, status: :not_found + end + end + + set.save! + end + + head :no_content + end + + def multiple_stories_params + params.permit(:api_key, stories: [:id, items: [:id, :position, :type, :sub_type, :image_url, :display_collection, :category, :meta, :record_id, content: [:value, :image_url, :display_collection, :category]]]) end def story_params diff --git a/app/policies/supplejack_api/user_set_policy.rb b/app/policies/supplejack_api/user_set_policy.rb index 09ae5d5b3..e8e9f1120 100644 --- a/app/policies/supplejack_api/user_set_policy.rb +++ b/app/policies/supplejack_api/user_set_policy.rb @@ -21,5 +21,6 @@ def show? alias update? admin_or_owner? alias multiple_add? admin_or_owner? + alias multiple_remove? admin_or_owner? end end diff --git a/config/routes.rb b/config/routes.rb index 677a14fb0..7677d67d6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -38,6 +38,7 @@ namespace 'stories' do resources :featured, only: [:index] post :multiple_add + post :multiple_remove end resources :stories, except: [:new, :edit] do diff --git a/spec/controllers/supplejack_api/stories_controller_spec.rb b/spec/controllers/supplejack_api/stories_controller_spec.rb index b7238d4f5..b34d72ba8 100644 --- a/spec/controllers/supplejack_api/stories_controller_spec.rb +++ b/spec/controllers/supplejack_api/stories_controller_spec.rb @@ -500,5 +500,138 @@ module SupplejackApi end end end + + describe 'POST multiple_delete' do + let(:user) { create(:user) } + let(:user_two) { create(:user) } + + let!(:story_one) { create(:story_with_dnz_story_items, user_id: user.id) } + let!(:story_two) { create(:story_with_dnz_story_items, user_id: user.id) } + + + context 'valid' do + it 'deletes multiple story items from multiple stories' do + expect(story_one.reload.set_items.count).to eq 4 + expect(story_two.reload.set_items.count).to eq 4 + + post :multiple_remove, + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: story_one.set_items.last.id.to_s } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } + + expect(story_one.reload.set_items.count).to eq 2 + expect(story_two.reload.set_items.count).to eq 2 + end + + it 'returns a success response' do + post :multiple_remove, + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: story_one.set_items.last.id.to_s } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } + + expect(response.status).to eq 204 + end + end + + context 'invalid' do + it 'returns an error for stories that do not belong to the user' do + expect(story_one.reload.set_items.count).to eq 4 + expect(story_two.reload.set_items.count).to eq 4 + + post :multiple_remove, + params: { + api_key: user_two.api_key, + user_key: user_two.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: story_one.set_items.last.id.to_s } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } + + expect(response.status).to eq 401 + expect(JSON.parse(response.body)['errors']).to eq 'Provided user key is not authorized to access this story' + expect(story_one.reload.set_items.count).to eq 4 + expect(story_two.reload.set_items.count).to eq 4 + end + + it 'returns an error for invalid story items' do + expect(story_one.reload.set_items.count).to eq 4 + expect(story_two.reload.set_items.count).to eq 4 + + post :multiple_remove, + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: 'a' } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } + + expect(response.status).to eq 404 + expect(JSON.parse(response.body)['errors']).to eq 'Record with ID a was not found' + expect(story_one.reload.set_items.count).to eq 3 + expect(story_two.reload.set_items.count).to eq 4 + end + end + end end end From 123188255c68ef25bc1f4f603b810107679d81cf Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 14:09:20 +1300 Subject: [PATCH 05/10] Remove extra line --- app/controllers/supplejack_api/stories_controller.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index 3aa5a1ebf..849f07377 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -103,7 +103,6 @@ def multiple_add end def multiple_remove - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set From ed1f35b7684f4422876f927ed556eba77cd4d64f Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 14:20:06 +1300 Subject: [PATCH 06/10] rubocoppping --- .../supplejack_api/stories_controller.rb | 46 +-- .../supplejack_api/stories_controller_spec.rb | 335 +++++++++--------- 2 files changed, 191 insertions(+), 190 deletions(-) diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index 849f07377..780685df6 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +# rubocop:disable Metrics/ClassLength module SupplejackApi class StoriesController < SupplejackApplicationController include Pundit @@ -70,45 +71,43 @@ def reposition_items end def multiple_add - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories| + stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set authorize(set) - stories.push(set) + stories_array.push(set) end - changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes| + changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes_array| set = stories.find { |s| s.id.to_s == story_params['id'] } item_ids = story_params['items'].each_with_object([]) do |item, ids| item = set.set_items.build(item) - if item.valid? - ids.push(item.id) - else - return render_error_with(item.errors.messages.values.join(', '), :bad_request) - end + return render_error_with(item.errors.messages.values.join(', '), :bad_request) unless item.valid? + + ids.push(item.id) end set.save! - changes.push({ - story_id: story_params['id'], - item_ids: item_ids - }) + changes_array.push({ + story_id: story_params['id'], + item_ids: item_ids + }) end render json: changes end def multiple_remove - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories| + stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set authorize(set) - stories.push(set) + stories_array.push(set) end multiple_stories_params['stories'].each do |story_params| @@ -117,11 +116,10 @@ def multiple_remove story_params['items'].each do |item| set_item = set.set_items.find_by_id(item[:id]) - if set_item - set_item.destroy! - else - return render json: { errors: I18n.t('errors.record_not_found', id: item[:id]) }, status: :not_found - end + return render json: { errors: I18n.t('errors.record_not_found', id: item[:id]) }, + status: :not_found unless set_item + + set_item.destroy! end set.save! @@ -131,7 +129,11 @@ def multiple_remove end def multiple_stories_params - params.permit(:api_key, stories: [:id, items: [:id, :position, :type, :sub_type, :image_url, :display_collection, :category, :meta, :record_id, content: [:value, :image_url, :display_collection, :category]]]) + params.permit(:api_key, + stories: [:id, + { items: [:id, :position, :type, :sub_type, :image_url, + :display_collection, :category, :meta, :record_id, + { content: %i[value image_url display_collection category] }] }]) end def story_params @@ -170,7 +172,7 @@ def create_story_record_views SupplejackApi::RequestMetric.spawn(log, 'user_story_views') if log end - - end end + +# rubocop:enable Metrics/ClassLength diff --git a/spec/controllers/supplejack_api/stories_controller_spec.rb b/spec/controllers/supplejack_api/stories_controller_spec.rb index b34d72ba8..a60268153 100644 --- a/spec/controllers/supplejack_api/stories_controller_spec.rb +++ b/spec/controllers/supplejack_api/stories_controller_spec.rb @@ -378,30 +378,30 @@ module SupplejackApi let(:story_item_four) { attributes_for(:story_item) } let(:story_item_five) { attributes_for(:story_item) } - context 'valid' do + context 'valid' do before do post :multiple_add, - params: { - api_key: user.api_key, - user_key: user.api_key, - stories: [ - { - id: story_one.id, - items: [ - story_item_one, - story_item_two - ] - }, - { - id: story_two.id, - items: [ - story_item_three, - story_item_four, - story_item_five - ] - } - ] - } + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + story_item_one, + story_item_two + ] + }, + { + id: story_two.id, + items: [ + story_item_three, + story_item_four, + story_item_five + ] + } + ] + } end it 'adds multiple story items to multiple stories' do @@ -423,49 +423,49 @@ module SupplejackApi context 'invalid' do it 'returns an error when given an invalid story id' do post :multiple_add, - params: { - api_key: user.api_key, - user_key: user.api_key, - stories: [ - { - id: 'a', - items: [ - story_item_one, - story_item_two - ] - }, - { - id: story_two.id, - items: [ - story_item_three, - story_item_four, - story_item_five - ] - } - ] - } + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: 'a', + items: [ + story_item_one, + story_item_two + ] + }, + { + id: story_two.id, + items: [ + story_item_three, + story_item_four, + story_item_five + ] + } + ] + } expect(response.status).to eq 404 end it 'returns an error when given an invalid item' do post :multiple_add, - params: { - api_key: user.api_key, - user_key: user.api_key, - stories: [ - { - id: story_one.id, - items: [ - story_item_one, - { - type: 'text', - sub_type: 'heading' - } - ] - } - ] - } + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + story_item_one, + { + type: 'text', + sub_type: 'heading' + } + ] + } + ] + } expect(response.status).to eq 400 expect(JSON.parse(response.body)['errors']).to eq 'Content value is missing: content must contain value field' @@ -473,27 +473,27 @@ module SupplejackApi it 'returns an error when trying to update stories that you do not have access too' do post :multiple_add, - params: { - api_key: user_two.api_key, - user_key: user_two.api_key, - stories: [ - { - id: story_one.id, - items: [ - story_item_one, - story_item_two - ] - }, - { - id: story_two.id, - items: [ - story_item_three, - story_item_four, - story_item_five - ] - } - ] - } + params: { + api_key: user_two.api_key, + user_key: user_two.api_key, + stories: [ + { + id: story_one.id, + items: [ + story_item_one, + story_item_two + ] + }, + { + id: story_two.id, + items: [ + story_item_three, + story_item_four, + story_item_five + ] + } + ] + } expect(response.status).to eq 401 expect(JSON.parse(response.body)['errors']).to eq 'Provided user key is not authorized to access this story' @@ -504,98 +504,97 @@ module SupplejackApi describe 'POST multiple_delete' do let(:user) { create(:user) } let(:user_two) { create(:user) } - + let!(:story_one) { create(:story_with_dnz_story_items, user_id: user.id) } let!(:story_two) { create(:story_with_dnz_story_items, user_id: user.id) } - - - context 'valid' do + + context 'valid' do it 'deletes multiple story items from multiple stories' do expect(story_one.reload.set_items.count).to eq 4 expect(story_two.reload.set_items.count).to eq 4 post :multiple_remove, - params: { - api_key: user.api_key, - user_key: user.api_key, - stories: [ - { - id: story_one.id, - items: [ - { id: story_one.set_items.first.id.to_s }, - { id: story_one.set_items.last.id.to_s } - ] - }, - { - id: story_two.id, - items: [ - { id: story_two.set_items.first.id.to_s }, - { id: story_two.set_items.last.id.to_s } - ] - } - ] - } + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: story_one.set_items.last.id.to_s } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } expect(story_one.reload.set_items.count).to eq 2 expect(story_two.reload.set_items.count).to eq 2 end - + it 'returns a success response' do post :multiple_remove, - params: { - api_key: user.api_key, - user_key: user.api_key, - stories: [ - { - id: story_one.id, - items: [ - { id: story_one.set_items.first.id.to_s }, - { id: story_one.set_items.last.id.to_s } - ] - }, - { - id: story_two.id, - items: [ - { id: story_two.set_items.first.id.to_s }, - { id: story_two.set_items.last.id.to_s } - ] - } - ] - } + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: story_one.set_items.last.id.to_s } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } expect(response.status).to eq 204 end end - context 'invalid' do + context 'invalid' do it 'returns an error for stories that do not belong to the user' do expect(story_one.reload.set_items.count).to eq 4 expect(story_two.reload.set_items.count).to eq 4 post :multiple_remove, - params: { - api_key: user_two.api_key, - user_key: user_two.api_key, - stories: [ - { - id: story_one.id, - items: [ - { id: story_one.set_items.first.id.to_s }, - { id: story_one.set_items.last.id.to_s } - ] - }, - { - id: story_two.id, - items: [ - { id: story_two.set_items.first.id.to_s }, - { id: story_two.set_items.last.id.to_s } - ] - } - ] - } + params: { + api_key: user_two.api_key, + user_key: user_two.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: story_one.set_items.last.id.to_s } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } expect(response.status).to eq 401 - expect(JSON.parse(response.body)['errors']).to eq 'Provided user key is not authorized to access this story' + expect(JSON.parse(response.body)['errors']).to eq 'Provided user key is not authorized to access this story' expect(story_one.reload.set_items.count).to eq 4 expect(story_two.reload.set_items.count).to eq 4 end @@ -605,29 +604,29 @@ module SupplejackApi expect(story_two.reload.set_items.count).to eq 4 post :multiple_remove, - params: { - api_key: user.api_key, - user_key: user.api_key, - stories: [ - { - id: story_one.id, - items: [ - { id: story_one.set_items.first.id.to_s }, - { id: 'a' } - ] - }, - { - id: story_two.id, - items: [ - { id: story_two.set_items.first.id.to_s }, - { id: story_two.set_items.last.id.to_s } - ] - } - ] - } + params: { + api_key: user.api_key, + user_key: user.api_key, + stories: [ + { + id: story_one.id, + items: [ + { id: story_one.set_items.first.id.to_s }, + { id: 'a' } + ] + }, + { + id: story_two.id, + items: [ + { id: story_two.set_items.first.id.to_s }, + { id: story_two.set_items.last.id.to_s } + ] + } + ] + } expect(response.status).to eq 404 - expect(JSON.parse(response.body)['errors']).to eq 'Record with ID a was not found' + expect(JSON.parse(response.body)['errors']).to eq 'Record with ID a was not found' expect(story_one.reload.set_items.count).to eq 3 expect(story_two.reload.set_items.count).to eq 4 end From 9fd4a4ff68784fd89a49b32652fed9d02666e56c Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 14:31:45 +1300 Subject: [PATCH 07/10] Move multiple actions to their own concern --- .../concerns/stories/multiple.rb | 67 +++++++++++++++++++ .../supplejack_api/stories_controller.rb | 62 +---------------- 2 files changed, 68 insertions(+), 61 deletions(-) create mode 100644 app/controllers/supplejack_api/concerns/stories/multiple.rb diff --git a/app/controllers/supplejack_api/concerns/stories/multiple.rb b/app/controllers/supplejack_api/concerns/stories/multiple.rb new file mode 100644 index 000000000..2e77a1df2 --- /dev/null +++ b/app/controllers/supplejack_api/concerns/stories/multiple.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module SupplejackApi + module Concerns + module Stories + module Multiple + def multiple_add + stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| + set = SupplejackApi::UserSet.custom_find(story['id']) + return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set + + authorize(set) + stories_array.push(set) + end + + changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes_array| + set = stories.find { |s| s.id.to_s == story_params['id'] } + + item_ids = story_params['items'].each_with_object([]) do |item, ids| + item = set.set_items.build(item) + + return render_error_with(item.errors.messages.values.join(', '), :bad_request) unless item.valid? + + ids.push(item.id) + end + + set.save! + + changes_array.push({ + story_id: story_params['id'], + item_ids: item_ids + }) + end + + render json: changes + end + + def multiple_remove + stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| + set = SupplejackApi::UserSet.custom_find(story['id']) + return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set + + authorize(set) + stories_array.push(set) + end + + multiple_stories_params['stories'].each do |story_params| + set = stories.find { |s| s.id.to_s == story_params['id'] } + + story_params['items'].each do |item| + set_item = set.set_items.find_by_id(item[:id]) + + return render json: { errors: I18n.t('errors.record_not_found', id: item[:id]) }, + status: :not_found unless set_item + + set_item.destroy! + end + + set.save! + end + + head :no_content + end + end + end + end +end diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index 780685df6..2ebb4d55a 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true -# rubocop:disable Metrics/ClassLength module SupplejackApi class StoriesController < SupplejackApplicationController include Pundit include Concerns::IgnoreMetrics + include Concerns::Stories::Multiple rescue_from Pundit::NotAuthorizedError, with: :user_not_authorized @@ -70,64 +70,6 @@ def reposition_items end end - def multiple_add - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| - set = SupplejackApi::UserSet.custom_find(story['id']) - return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set - - authorize(set) - stories_array.push(set) - end - - changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes_array| - set = stories.find { |s| s.id.to_s == story_params['id'] } - - item_ids = story_params['items'].each_with_object([]) do |item, ids| - item = set.set_items.build(item) - - return render_error_with(item.errors.messages.values.join(', '), :bad_request) unless item.valid? - - ids.push(item.id) - end - - set.save! - - changes_array.push({ - story_id: story_params['id'], - item_ids: item_ids - }) - end - - render json: changes - end - - def multiple_remove - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| - set = SupplejackApi::UserSet.custom_find(story['id']) - return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set - - authorize(set) - stories_array.push(set) - end - - multiple_stories_params['stories'].each do |story_params| - set = stories.find { |s| s.id.to_s == story_params['id'] } - - story_params['items'].each do |item| - set_item = set.set_items.find_by_id(item[:id]) - - return render json: { errors: I18n.t('errors.record_not_found', id: item[:id]) }, - status: :not_found unless set_item - - set_item.destroy! - end - - set.save! - end - - head :no_content - end - def multiple_stories_params params.permit(:api_key, stories: [:id, @@ -174,5 +116,3 @@ def create_story_record_views end end end - -# rubocop:enable Metrics/ClassLength From 77488917b662662f11278c682fe2096e294f376a Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 14:32:11 +1300 Subject: [PATCH 08/10] rubocopping --- .../concerns/stories/multiple.rb | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/app/controllers/supplejack_api/concerns/stories/multiple.rb b/app/controllers/supplejack_api/concerns/stories/multiple.rb index 2e77a1df2..ac15e8870 100644 --- a/app/controllers/supplejack_api/concerns/stories/multiple.rb +++ b/app/controllers/supplejack_api/concerns/stories/multiple.rb @@ -8,57 +8,57 @@ def multiple_add stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set - + authorize(set) stories_array.push(set) end - + changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes_array| set = stories.find { |s| s.id.to_s == story_params['id'] } - + item_ids = story_params['items'].each_with_object([]) do |item, ids| item = set.set_items.build(item) - + return render_error_with(item.errors.messages.values.join(', '), :bad_request) unless item.valid? - + ids.push(item.id) end - + set.save! - + changes_array.push({ - story_id: story_params['id'], - item_ids: item_ids - }) + story_id: story_params['id'], + item_ids: item_ids + }) end - + render json: changes end - + def multiple_remove stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set - + authorize(set) stories_array.push(set) end - + multiple_stories_params['stories'].each do |story_params| set = stories.find { |s| s.id.to_s == story_params['id'] } - + story_params['items'].each do |item| set_item = set.set_items.find_by_id(item[:id]) - + return render json: { errors: I18n.t('errors.record_not_found', id: item[:id]) }, status: :not_found unless set_item - + set_item.destroy! end - + set.save! end - + head :no_content end end From 21f363652a2e87846fc0fed9c8728afe18a8cb1b Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Wed, 20 Oct 2021 14:34:00 +1300 Subject: [PATCH 09/10] rename for clarity --- .../concerns/stories/multiple.rb | 18 ++++++++++++++---- .../supplejack_api/stories_controller.rb | 8 -------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/app/controllers/supplejack_api/concerns/stories/multiple.rb b/app/controllers/supplejack_api/concerns/stories/multiple.rb index ac15e8870..4cc487dbe 100644 --- a/app/controllers/supplejack_api/concerns/stories/multiple.rb +++ b/app/controllers/supplejack_api/concerns/stories/multiple.rb @@ -5,7 +5,7 @@ module Concerns module Stories module Multiple def multiple_add - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| + stories = multiple_params['stories'].each_with_object([]) do |story, stories_array| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set @@ -13,7 +13,7 @@ def multiple_add stories_array.push(set) end - changes = multiple_stories_params['stories'].each_with_object([]) do |story_params, changes_array| + changes = multiple_params['stories'].each_with_object([]) do |story_params, changes_array| set = stories.find { |s| s.id.to_s == story_params['id'] } item_ids = story_params['items'].each_with_object([]) do |item, ids| @@ -36,7 +36,7 @@ def multiple_add end def multiple_remove - stories = multiple_stories_params['stories'].each_with_object([]) do |story, stories_array| + stories = multiple_params['stories'].each_with_object([]) do |story, stories_array| set = SupplejackApi::UserSet.custom_find(story['id']) return render_error_with(I18n.t('errors.story_not_found', id: story['id']), :not_found) unless set @@ -44,7 +44,7 @@ def multiple_remove stories_array.push(set) end - multiple_stories_params['stories'].each do |story_params| + multiple_params['stories'].each do |story_params| set = stories.find { |s| s.id.to_s == story_params['id'] } story_params['items'].each do |item| @@ -61,6 +61,16 @@ def multiple_remove head :no_content end + + private + + def multiple_params + params.permit(:api_key, + stories: [:id, + { items: [:id, :position, :type, :sub_type, :image_url, + :display_collection, :category, :meta, :record_id, + { content: %i[value image_url display_collection category] }] }]) + end end end end diff --git a/app/controllers/supplejack_api/stories_controller.rb b/app/controllers/supplejack_api/stories_controller.rb index 2ebb4d55a..a9ede0cce 100644 --- a/app/controllers/supplejack_api/stories_controller.rb +++ b/app/controllers/supplejack_api/stories_controller.rb @@ -70,14 +70,6 @@ def reposition_items end end - def multiple_stories_params - params.permit(:api_key, - stories: [:id, - { items: [:id, :position, :type, :sub_type, :image_url, - :display_collection, :category, :meta, :record_id, - { content: %i[value image_url display_collection category] }] }]) - end - def story_params fields = [:name, :description, :privacy, :copyright, :cover_thumbnail, { tags: [], subjects: [] }] From 7c1982d4a01b18d8463d5c269e8f8b18fced33bd Mon Sep 17 00:00:00 2001 From: Richard Matthews Date: Fri, 22 Oct 2021 13:41:35 +1300 Subject: [PATCH 10/10] Add specs for multiple_add? and multiple_remove? policies --- .../supplejack_api/user_set_policy_spec.rb | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/spec/policies/supplejack_api/user_set_policy_spec.rb b/spec/policies/supplejack_api/user_set_policy_spec.rb index 44907c6e1..78e965bbb 100644 --- a/spec/policies/supplejack_api/user_set_policy_spec.rb +++ b/spec/policies/supplejack_api/user_set_policy_spec.rb @@ -74,4 +74,56 @@ end end end + + permissions :multiple_add? do + context 'when user is the owner of the story' do + let(:user) { story.user } + + it 'grants access' do + expect(policy).to permit(user, story) + end + end + + context 'when user is an admin' do + let(:admin) { create(:admin_user) } + + it 'grants access' do + expect(policy).to permit(admin, story) + end + end + + context 'when user is not and admin or owner of story' do + let(:user) { create(:user) } + + it 'denies access' do + expect(policy).not_to permit(user, story) + end + end + end + + permissions :multiple_remove? do + context 'when user is the owner of the story' do + let(:user) { story.user } + + it 'grants access' do + expect(policy).to permit(user, story) + end + end + + context 'when user is an admin' do + let(:admin) { create(:admin_user) } + + it 'grants access' do + expect(policy).to permit(admin, story) + end + end + + context 'when user is not and admin or owner of story' do + let(:user) { create(:user) } + + it 'denies access' do + expect(policy).not_to permit(user, story) + end + end + end end