From bce34ee9a1ea21bd8baa62ddb52183c2d00758fe Mon Sep 17 00:00:00 2001 From: ArayB Date: Thu, 17 Mar 2022 09:29:39 +0000 Subject: [PATCH 1/8] Create draft PR for #34 From cd8c70272975973d099175d3e792a51759edcd9c Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Thu, 17 Mar 2022 12:35:15 +0000 Subject: [PATCH 2/8] Use submitted project data when remixing Ensure that component information is taken from params when remixing a project. This allows any changes a user has made to a project before remixing to be persisted. --- .../project/operation/create_remix.rb | 28 +++-- .../api/projects/remixes_controller.rb | 12 +- app/controllers/api_controller.rb | 2 + .../project/operation/create_remix_spec.rb | 118 ++++++++++++------ spec/factories/component.rb | 1 + spec/request/projects/remix_spec.rb | 15 ++- 6 files changed, 121 insertions(+), 55 deletions(-) diff --git a/app/concepts/project/operation/create_remix.rb b/app/concepts/project/operation/create_remix.rb index 22a7c3818..02a0aceae 100644 --- a/app/concepts/project/operation/create_remix.rb +++ b/app/concepts/project/operation/create_remix.rb @@ -5,39 +5,41 @@ module Operation class CreateRemix require 'operation_response' - def self.call(params, user_id) + def self.call(params:, user_id:, original_project:) response = OperationResponse.new - validate_params(response, params, user_id) - return response if response.failure? - - remix_project(response, params, user_id) + validate_params(response, params, user_id, original_project) + remix_project(response, params, user_id, original_project) response end class << self private - def validate_params(response, params, user_id) - valid = params[:project_id].present? && user_id.present? + def validate_params(response, params, user_id, original_project) + valid = params[:identifier].present? && user_id.present? && original_project.present? response[:error] = 'Invalid parameters' unless valid end - def remix_project(response, params, user_id) - original_project = Project.find_by!(identifier: params[:project_id]) + def remix_project(response, params, user_id, original_project) + return if response[:error] - response[:project] = create_remix(original_project, user_id) + response[:project] = create_remix(original_project, params, user_id) response[:error] = 'Unable to create project' unless response[:project].save response end - def create_remix(original_project, user_id) - original_project.dup.tap do |proj| + def create_remix(original_project, params, user_id) + remix = original_project.dup.tap do |proj| proj.user_id = user_id - proj.components = original_project.components.map(&:dup) proj.remixed_from_id = original_project.id end + + params[:components].each do |x| + remix.components.build(x.slice(:name, :extension, :content, :index)) + end + remix end end end diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index f24a87963..4f3089194 100644 --- a/app/controllers/api/projects/remixes_controller.rb +++ b/app/controllers/api/projects/remixes_controller.rb @@ -4,9 +4,10 @@ module Api module Projects class RemixesController < ApiController before_action :require_oauth_user + before_action :load_project def create - result = Project::Operation::CreateRemix.call(remix_params, oauth_user_id) + result = Project::Operation::CreateRemix.call(params: remix_params, user_id: oauth_user_id, original_project: @project) if result.success? @project = result[:project] @@ -18,8 +19,15 @@ def create private + def load_project + @project = Project.find_by!(identifier: params[:project_id]) + end + def remix_params - params.permit(:project_id) + params.require(:project) + .permit(:name, + :identifier, + components: %i[id name extension content index]) end end end diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index fecf2da8c..2477c2768 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -26,4 +26,6 @@ def return404 def return401 head :unauthorized end + + alias_method :current_user, :oauth_user_id end diff --git a/spec/concepts/project/operation/create_remix_spec.rb b/spec/concepts/project/operation/create_remix_spec.rb index c2c58e221..f36b3d5ee 100644 --- a/spec/concepts/project/operation/create_remix_spec.rb +++ b/spec/concepts/project/operation/create_remix_spec.rb @@ -3,52 +3,85 @@ require 'rails_helper' RSpec.describe Project::Operation::CreateRemix, type: :unit do - subject(:create_remix) { described_class.call(params, user_id) } + subject(:create_remix) { described_class.call(params: remix_params, user_id: user_id, original_project: original_project) } let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } let!(:original_project) { create(:project, :with_components) } + let(:remix_params) do + component = original_project.components.first + { + name: original_project.name, + identifier: original_project.identifier, + components: [ + { + id: component.id, + name: component.name, + extension: component.extension, + content: 'some updated component content', + index: component.index + } + ] + } + end before do mock_phrase_generation end describe '.call' do - context 'when all params valid' do - let(:params) { { project_id: original_project.identifier } } + let(:params) { { project_id: original_project.identifier } } - it 'returns success' do - result = create_remix - expect(result.success?).to eq(true) - end + it 'returns success' do + result = create_remix + expect(result.success?).to eq(true) + end - it 'creates new project' do - expect { create_remix }.to change(Project, :count).by(1) - end + it 'creates new project' do + expect { create_remix }.to change(Project, :count).by(1) + end - it 'assigns a new identifer to new project' do - result = create_remix - remixed_project = result[:project] - expect(remixed_project.identifier).not_to eq(original_project.identifier) - end + it 'assigns a new identifer to new project' do + result = create_remix + remixed_project = result[:project] + expect(remixed_project.identifier).not_to eq(original_project.identifier) + end - it 'assigns user_id to new project' do - remixed_project = create_remix[:project] - expect(remixed_project.user_id).to eq(user_id) - end + it 'assigns user_id to new project' do + remixed_project = create_remix[:project] + expect(remixed_project.user_id).to eq(user_id) + end - it 'duplicates properties on new project' do - remixed_project = create_remix[:project] + it 'duplicates properties on new project' do + remixed_project = create_remix[:project] + + remixed_attrs = remixed_project.attributes.symbolize_keys.slice(:name, :project_type) + original_attrs = original_project.attributes.symbolize_keys.slice(:name, :project_type) + expect(remixed_attrs).to eq(original_attrs) + end + + it 'creates new components' do + expect { create_remix }.to change(Component, :count).by(1) + end - remixed_attrs = remixed_project.attributes.symbolize_keys.slice(:name, :project_type) - original_attrs = original_project.attributes.symbolize_keys.slice(:name, :project_type) - expect(remixed_attrs).to eq(original_attrs) + it 'persists changes made to submitted components' do + remixed_project = create_remix[:project] + expect(remixed_project.components.first.content).to eq('some updated component content') + end + + context 'when a new component has been added before remixing' do + let(:new_component_params) { { name: 'added_component', extension: 'py', content: 'some added component content', index: 9999 } } + + before do + remix_params[:components] << new_component_params end - it 'duplicates project components' do - remixed_props_array = component_array_props(create_remix[:project].components) - original_props_array = component_array_props(original_project.components) + it 'creates all components' do + expect { create_remix }.to change(Component, :count).by(2) + end - expect(remixed_props_array).to match_array(original_props_array) + it 'persists the new component' do + remixed_project = create_remix[:project] + expect(remixed_project.components.last.attributes.symbolize_keys).to include(new_component_params) end end @@ -65,16 +98,27 @@ expect { create_remix }.not_to change(Project, :count) end end - end - def component_array_props(components) - components.map do |x| - { - name: x.name, - content: x.content, - extension: x.extension, - index: x.index - } + context 'when original project is not present' do + subject(:create_remix) { described_class.call(params: remix_params, user_id: user_id, original_project: nil) } + + it 'returns failure' do + result = create_remix + expect(result.failure?).to eq(true) + end + + it 'does not create new project' do + expect { create_remix }.not_to change(Project, :count) + end end end + + def component_props(component) + { + name: component.name, + content: component.content, + extension: component.extension, + index: component.index + } + end end diff --git a/spec/factories/component.rb b/spec/factories/component.rb index 20b3b7841..debe18f42 100644 --- a/spec/factories/component.rb +++ b/spec/factories/component.rb @@ -6,6 +6,7 @@ extension { 'py' } sequence(:index) { |n| n } default { false } + content { Faker::Lorem.paragraph } project factory :default_python_component do diff --git a/spec/request/projects/remix_spec.rb b/spec/request/projects/remix_spec.rb index bb8a28fc7..306f494b1 100644 --- a/spec/request/projects/remix_spec.rb +++ b/spec/request/projects/remix_spec.rb @@ -12,18 +12,27 @@ end context 'when auth is correct' do + let(:project_params) do + { + name: original_project.name, + identifier: original_project.identifier, + components: [] + } + end + before do - mock_oauth_user + mock_oauth_user(user_id) end it 'returns success response' do - post "/api/projects/#{original_project.identifier}/remix" + post "/api/projects/#{original_project.identifier}/remix", params: { project: project_params } expect(response.status).to eq(200) end it 'returns 404 response if invalid project' do - post '/api/projects/no-such-project/remix' + project_params[:identifier] = 'no-such-project' + post '/api/projects/no-such-project/remix', params: { project: project_params } expect(response.status).to eq(404) end From c24957dd3f9d9e341ee3f2d9085655d028d03fbd Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Thu, 17 Mar 2022 14:10:02 +0000 Subject: [PATCH 3/8] Fix rubocop issues --- app/controllers/api/projects/remixes_controller.rb | 3 ++- app/controllers/api_controller.rb | 7 +------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index 4f3089194..cb5f7caf0 100644 --- a/app/controllers/api/projects/remixes_controller.rb +++ b/app/controllers/api/projects/remixes_controller.rb @@ -7,7 +7,8 @@ class RemixesController < ApiController before_action :load_project def create - result = Project::Operation::CreateRemix.call(params: remix_params, user_id: oauth_user_id, original_project: @project) + result = Project::Operation::CreateRemix.call(params: remix_params, user_id: oauth_user_id, + original_project: @project) if result.success? @project = result[:project] diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 2477c2768..a7dc4f927 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -14,11 +14,6 @@ def require_oauth_user head :unauthorized unless oauth_user_id end - def current_user - # current_user is required by CanCanCan - oauth_user_id - end - def return404 head :not_found end @@ -27,5 +22,5 @@ def return401 head :unauthorized end - alias_method :current_user, :oauth_user_id + alias current_user oauth_user_id end From 633935f392d4e3a0f12348448b2a40e2e05f916e Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Thu, 17 Mar 2022 14:43:35 +0000 Subject: [PATCH 4/8] Alias method --- app/controllers/api_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index a7dc4f927..ca107512d 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -3,6 +3,8 @@ class ApiController < ActionController::API include OauthUser + alias current_user oauth_user_id + unless Rails.application.config.consider_all_requests_local rescue_from ActiveRecord::RecordNotFound, with: -> { return404 } rescue_from CanCan::AccessDenied, with: -> { return401 } @@ -21,6 +23,4 @@ def return404 def return401 head :unauthorized end - - alias current_user oauth_user_id end From 2da23b9b6c1e0f13f37b092e4b9f4bdcb3e82774 Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Thu, 17 Mar 2022 15:30:01 +0000 Subject: [PATCH 5/8] Remove alias as it does not work Run tests in random order --- app/controllers/api_controller.rb | 7 +++++-- spec/spec_helper.rb | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index ca107512d..fecf2da8c 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -3,8 +3,6 @@ class ApiController < ActionController::API include OauthUser - alias current_user oauth_user_id - unless Rails.application.config.consider_all_requests_local rescue_from ActiveRecord::RecordNotFound, with: -> { return404 } rescue_from CanCan::AccessDenied, with: -> { return401 } @@ -16,6 +14,11 @@ def require_oauth_user head :unauthorized unless oauth_user_id end + def current_user + # current_user is required by CanCanCan + oauth_user_id + end + def return404 head :not_found end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 5d227ba75..7db67711e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -86,7 +86,7 @@ # # order dependency and want to debug it, you can fix the order by providing # # the seed, which is printed after each run. # # --seed 1234 - # config.order = :random + config.order = :random # # # Seed global randomization in this process using the `--seed` CLI option. # # Setting this allows you to use `--seed` to deterministically reproduce From 951072ccc4aefac6d0dafdbe53ec69a0b9bd2ddf Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Thu, 17 Mar 2022 17:08:01 +0000 Subject: [PATCH 6/8] Better test coverage --- .../project/operation/create_remix.rb | 18 ++++----- config/locales/en.yml | 3 ++ .../project/operation/create_remix_spec.rb | 26 +++++++++++++ spec/request/projects/remix_spec.rb | 37 +++++++++++++++---- 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/app/concepts/project/operation/create_remix.rb b/app/concepts/project/operation/create_remix.rb index 02a0aceae..ac4cfdeb5 100644 --- a/app/concepts/project/operation/create_remix.rb +++ b/app/concepts/project/operation/create_remix.rb @@ -5,20 +5,20 @@ module Operation class CreateRemix require 'operation_response' - def self.call(params:, user_id:, original_project:) - response = OperationResponse.new + class << self + def call(params:, user_id:, original_project:) + response = OperationResponse.new - validate_params(response, params, user_id, original_project) - remix_project(response, params, user_id, original_project) - response - end + validate_params(response, params, user_id, original_project) + remix_project(response, params, user_id, original_project) + response + end - class << self private def validate_params(response, params, user_id, original_project) valid = params[:identifier].present? && user_id.present? && original_project.present? - response[:error] = 'Invalid parameters' unless valid + response[:error] = I18n.t('errors.project.remixing.invalid_params') unless valid end def remix_project(response, params, user_id, original_project) @@ -26,7 +26,7 @@ def remix_project(response, params, user_id, original_project) response[:project] = create_remix(original_project, params, user_id) - response[:error] = 'Unable to create project' unless response[:project].save + response[:error] = I18n.t('errors.project.remixing.cannot_save') unless response[:project].save response end diff --git a/config/locales/en.yml b/config/locales/en.yml index d5379a51c..70e5074e6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -5,3 +5,6 @@ en: delete_default_component: 'Cannot delete default file' change_default_name: 'Cannot amend default file name' change_default_extension: 'Cannot amend default file extension' + remixing: + invalid_params: 'Invalid parameters' + cannot_save: 'Cannot create project remix' diff --git a/spec/concepts/project/operation/create_remix_spec.rb b/spec/concepts/project/operation/create_remix_spec.rb index f36b3d5ee..4bc05f711 100644 --- a/spec/concepts/project/operation/create_remix_spec.rb +++ b/spec/concepts/project/operation/create_remix_spec.rb @@ -94,6 +94,11 @@ expect(result.failure?).to eq(true) end + it 'returns error message' do + result = create_remix + expect(result[:error]).to eq(I18n.t('errors.project.remixing.invalid_params')) + end + it 'does not create new project' do expect { create_remix }.not_to change(Project, :count) end @@ -107,10 +112,31 @@ expect(result.failure?).to eq(true) end + it 'returns error message' do + result = create_remix + expect(result[:error]).to eq(I18n.t('errors.project.remixing.invalid_params')) + end + it 'does not create new project' do expect { create_remix }.not_to change(Project, :count) end end + + context 'when project components are invalid' do + let(:invalid_component_params) { { name: 'added_component', extension: 'py', content: '' } } + + before do + remix_params[:components] << invalid_component_params + end + + it 'returns failure' do + expect(create_remix.failure?).to eq(true) + end + + it 'sets error message' do + expect(create_remix[:error]).to eq(I18n.t('errors.project.remixing.cannot_save')) + end + end end def component_props(component) diff --git a/spec/request/projects/remix_spec.rb b/spec/request/projects/remix_spec.rb index 306f494b1..430d43e93 100644 --- a/spec/request/projects/remix_spec.rb +++ b/spec/request/projects/remix_spec.rb @@ -1,10 +1,18 @@ # frozen_string_literal: true require 'rails_helper' +require_relative '../../../lib/operation_response' RSpec.describe 'Remix requests', type: :request do let!(:original_project) { create(:project) } let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } + let(:project_params) do + { + name: original_project.name, + identifier: original_project.identifier, + components: [] + } + end describe 'create' do before do @@ -12,14 +20,6 @@ end context 'when auth is correct' do - let(:project_params) do - { - name: original_project.name, - identifier: original_project.identifier, - components: [] - } - end - before do mock_oauth_user(user_id) end @@ -38,6 +38,27 @@ end end + context 'when project can not be saved' do + before do + mock_oauth_user(user_id) + error_response = OperationResponse.new + error_response[:error] = 'Something went wrong' + allow(Project::Operation::CreateRemix).to receive(:call).and_return(error_response) + end + + it 'returns 400' do + post "/api/projects/#{original_project.identifier}/remix", params: { project: project_params } + + expect(response.status).to eq(400) + end + + it 'returns error message' do + post "/api/projects/#{original_project.identifier}/remix", params: { project: project_params } + + expect(response.body).to eq({ error: 'Something went wrong' }.to_json) + end + end + context 'when auth is invalid' do it 'returns unauthorized' do post "/api/projects/#{original_project.identifier}/remix" From afb7fe4a382d0b7ce3fe0008e5ee247f2df48743 Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Mon, 21 Mar 2022 08:44:07 +0000 Subject: [PATCH 7/8] Use named params in public call to operation --- app/concepts/project/operation/update.rb | 2 +- app/controllers/api/projects_controller.rb | 2 +- .../concepts/project/operation/update_default_component_spec.rb | 2 +- .../concepts/project/operation/update_delete_components_spec.rb | 2 +- spec/concepts/project/operation/update_invalid_spec.rb | 2 +- spec/concepts/project/operation/update_spec.rb | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/concepts/project/operation/update.rb b/app/concepts/project/operation/update.rb index d210d321f..cfde67f70 100644 --- a/app/concepts/project/operation/update.rb +++ b/app/concepts/project/operation/update.rb @@ -5,7 +5,7 @@ module Operation class Update require 'operation_response' - def self.call(params, project) + def self.call(params:, project:) response = setup_response(project) setup_deletions(response, params) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index 0de3c02c5..e3f15ee1f 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -12,7 +12,7 @@ def show end def update - result = Project::Operation::Update.call(project_params, @project) + result = Project::Operation::Update.call(params: project_params, project: @project) if result.success? render :show, formats: [:json] diff --git a/spec/concepts/project/operation/update_default_component_spec.rb b/spec/concepts/project/operation/update_default_component_spec.rb index 7d888c323..4c55207bc 100644 --- a/spec/concepts/project/operation/update_default_component_spec.rb +++ b/spec/concepts/project/operation/update_default_component_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Project::Operation::Update, type: :unit do - subject(:update) { described_class.call(project_params, project) } + subject(:update) { described_class.call(params: project_params, project: project) } let!(:project) { create(:project, :with_default_component) } let(:default_component) { project.components.first } diff --git a/spec/concepts/project/operation/update_delete_components_spec.rb b/spec/concepts/project/operation/update_delete_components_spec.rb index b33ec43d7..ad08160f3 100644 --- a/spec/concepts/project/operation/update_delete_components_spec.rb +++ b/spec/concepts/project/operation/update_delete_components_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Project::Operation::Update, type: :unit do - subject(:update) { described_class.call(project_params, project) } + subject(:update) { described_class.call(params: project_params, project: project) } let!(:project) { create(:project, :with_default_component, :with_components) } let(:component_to_delete) { project.components.last } diff --git a/spec/concepts/project/operation/update_invalid_spec.rb b/spec/concepts/project/operation/update_invalid_spec.rb index 2725c1fac..449faa2fd 100644 --- a/spec/concepts/project/operation/update_invalid_spec.rb +++ b/spec/concepts/project/operation/update_invalid_spec.rb @@ -8,7 +8,7 @@ name: 'updated project name', components: [default_component_params, edited_component_params, new_component_params] } - described_class.call(params, project) + described_class.call(params: params, project: project) end let!(:project) { create(:project, :with_default_component, :with_components, component_count: 2) } diff --git a/spec/concepts/project/operation/update_spec.rb b/spec/concepts/project/operation/update_spec.rb index e815ccdd2..a0ea8a655 100644 --- a/spec/concepts/project/operation/update_spec.rb +++ b/spec/concepts/project/operation/update_spec.rb @@ -8,7 +8,7 @@ name: 'updated project name', components: component_params } - described_class.call(params, project) + described_class.call(params: params, project: project) end let!(:project) { create(:project, :with_default_component, :with_components) } From a76b4e1d7fca6cf9536391c631619a03112b6efd Mon Sep 17 00:00:00 2001 From: Ant Barnes Date: Mon, 21 Mar 2022 08:46:40 +0000 Subject: [PATCH 8/8] Memoize project load and remove before_action --- app/controllers/api/projects/remixes_controller.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index cb5f7caf0..cc57d69cf 100644 --- a/app/controllers/api/projects/remixes_controller.rb +++ b/app/controllers/api/projects/remixes_controller.rb @@ -4,11 +4,11 @@ module Api module Projects class RemixesController < ApiController before_action :require_oauth_user - before_action :load_project def create - result = Project::Operation::CreateRemix.call(params: remix_params, user_id: oauth_user_id, - original_project: @project) + result = Project::Operation::CreateRemix.call(params: remix_params, + user_id: oauth_user_id, + original_project: project) if result.success? @project = result[:project] @@ -20,8 +20,8 @@ def create private - def load_project - @project = Project.find_by!(identifier: params[:project_id]) + def project + @project ||= Project.find_by!(identifier: params[:project_id]) end def remix_params