diff --git a/app/concepts/project/operation/update.rb b/app/concepts/project/operation/update.rb new file mode 100644 index 000000000..d210d321f --- /dev/null +++ b/app/concepts/project/operation/update.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +class Project + module Operation + class Update + require 'operation_response' + + def self.call(params, project) + response = setup_response(project) + + setup_deletions(response, params) + update_project_attributes(response, params) + update_component_attributes(response, params) + persist_changes(response) + + response + end + + class << self + private + + def setup_response(project) + response = OperationResponse.new + response[:project] = project + response + end + + def setup_deletions(response, params) + existing_component_ids = response[:project].components.pluck(:id) + updated_component_ids = params[:components].pluck(:id) + response[:component_ids_to_delete] = existing_component_ids - updated_component_ids + + validate_deletions(response) + end + + def validate_deletions(response) + default_component_id = response[:project].components.find_by(default: true)&.id + return unless response[:component_ids_to_delete]&.include?(default_component_id) + + response[:error] = I18n.t 'errors.project.editing.delete_default_component' + end + + def update_project_attributes(response, params) + return if response[:error] + + response[:project].assign_attributes(params.slice(:name)) + end + + def update_component_attributes(response, params) + return if response[:error] + + params[:components].each do |component_params| + if component_params[:id].present? + component = response[:project].components.select { |c| c.id == component_params[:id] }.first + component.assign_attributes(component_params) + else + response[:project].components.build(component_params) + end + end + end + + def persist_changes(response) + return if response[:error] + + ActiveRecord::Base.transaction do + response[:project].save! + response[:project].components.where(id: response[:component_ids_to_delete]).destroy_all + rescue StandardError + # TODO: log error? + response[:error] = 'Error persisting changes' + end + end + end + end + end +end diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index f773d8b66..0de3c02c5 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -3,33 +3,34 @@ module Api class ProjectsController < ApiController require 'phrase_identifier' - before_action :require_oauth_user, only: %i[update] + before_action :load_project + load_and_authorize_resource def show - @project = Project.find_by!(identifier: params[:id]) render :show, formats: [:json] end def update - @project = Project.find_by!(identifier: params[:id]) - authorize! :update, @project + result = Project::Operation::Update.call(project_params, @project) - components = project_params[:components] - components.each do |comp_params| - component = Component.find(comp_params[:id]) - component.update(comp_params) + if result.success? + render :show, formats: [:json] + else + render json: { error: result[:error] }, status: :bad_request end - head :ok end private + def load_project + @project = Project.find_by!(identifier: params[:id]) + end + def project_params params.require(:project) - .permit(:identifier, - :type, - components: %i[id name extension content]) + .permit(:name, + components: %i[id name extension content index]) end end end diff --git a/app/models/ability.rb b/app/models/ability.rb index 438d12901..fa724636c 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -4,6 +4,8 @@ class Ability include CanCan::Ability def initialize(user) + can :read, Project + return if user.blank? can :update, Project, user_id: user diff --git a/app/models/component.rb b/app/models/component.rb index f8b53364f..eb8eb2dc8 100644 --- a/app/models/component.rb +++ b/app/models/component.rb @@ -2,4 +2,17 @@ class Component < ApplicationRecord belongs_to :project + validates :name, presence: true + validates :extension, presence: true + validates :index, presence: true, uniqueness: { scope: :project_id } + validate :default_component_protected_properties, on: :update + + private + + def default_component_protected_properties + return unless default? + + errors.add(:name, I18n.t('errors.project.editing.change_default_name')) if name_changed? + errors.add(:extension, I18n.t('errors.project.editing.change_default_extension')) if extension_changed? + end end diff --git a/app/models/project.rb b/app/models/project.rb index 8830427c6..094d3c7d5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -8,6 +8,7 @@ class Project < ApplicationRecord belongs_to :parent, class_name: 'Project', foreign_key: 'remixed_from_id', optional: true, inverse_of: :children has_many :components, -> { order(:index) }, dependent: :destroy, inverse_of: :project has_many :children, class_name: 'Project', foreign_key: 'remixed_from_id', dependent: :nullify, inverse_of: :parent + accepts_nested_attributes_for :components private diff --git a/app/views/api/projects/show.json.jbuilder b/app/views/api/projects/show.json.jbuilder index a3929b9af..da085ad97 100644 --- a/app/views/api/projects/show.json.jbuilder +++ b/app/views/api/projects/show.json.jbuilder @@ -4,4 +4,4 @@ json.call(@project, :identifier, :project_type, :name, :user_id) json.parent(@project.parent, :name, :identifier) if @project.parent -json.components @project.components, :id, :name, :extension, :content +json.components @project.components, :id, :name, :extension, :content, :index diff --git a/config/locales/en.yml b/config/locales/en.yml index 8ca56fc74..d5379a51c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1,33 +1,7 @@ -# Files in the config/locales directory are used for internationalization -# and are automatically loaded by Rails. If you want to use locales other -# than English, add the necessary files in this directory. -# -# To use the locales, use `I18n.t`: -# -# I18n.t "hello" -# -# In views, this is aliased to just `t`: -# -# <%= t("hello") %> -# -# To use a different locale, set it with `I18n.locale`: -# -# I18n.locale = :es -# -# This would use the information in config/locales/es.yml. -# -# The following keys must be escaped otherwise they will not be retrieved by -# the default I18n backend: -# -# true, false, on, off, yes, no -# -# Instead, surround them with single quotes. -# -# en: -# "true": "foo" -# -# To learn more, please read the Rails Internationalization guide -# available at https://guides.rubyonrails.org/i18n.html. - en: - hello: "Hello world" + errors: + project: + editing: + delete_default_component: 'Cannot delete default file' + change_default_name: 'Cannot amend default file name' + change_default_extension: 'Cannot amend default file extension' diff --git a/config/routes.rb b/config/routes.rb index 0b07b8978..aa0d8afc9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,11 +1,6 @@ # frozen_string_literal: true Rails.application.routes.draw do - # Define your application routes per the DSL in https://guides.rubyonrails.org/routing.html - - # Defines the root path route ("/") - # root "articles#index" - namespace :api do resource :default_project, only: %i[show create] do get '/html', to: 'default_projects#html' diff --git a/db/migrate/20220307144811_allow_nil_content_on_components.rb b/db/migrate/20220307144811_allow_nil_content_on_components.rb new file mode 100644 index 000000000..b1a233764 --- /dev/null +++ b/db/migrate/20220307144811_allow_nil_content_on_components.rb @@ -0,0 +1,9 @@ +class AllowNilContentOnComponents < ActiveRecord::Migration[7.0] + def up + change_column :components, :content, :string, null: true + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20220308115005_add_default_flag_to_components.rb b/db/migrate/20220308115005_add_default_flag_to_components.rb new file mode 100644 index 000000000..9a2dce29d --- /dev/null +++ b/db/migrate/20220308115005_add_default_flag_to_components.rb @@ -0,0 +1,5 @@ +class AddDefaultFlagToComponents < ActiveRecord::Migration[7.0] + def change + add_column :components, :default, :boolean, null: false, default: false + end +end diff --git a/db/migrate/20220310120419_add_unique_index_components.rb b/db/migrate/20220310120419_add_unique_index_components.rb new file mode 100644 index 000000000..78d958ec0 --- /dev/null +++ b/db/migrate/20220310120419_add_unique_index_components.rb @@ -0,0 +1,5 @@ +class AddUniqueIndexComponents < ActiveRecord::Migration[7.0] + def change + add_index :components, [:index, :project_id], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 7ebc5b8a1..8d5dbec3b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_02_28_094815) do +ActiveRecord::Schema.define(version: 2022_03_10_120419) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -20,10 +20,12 @@ t.uuid "project_id" t.string "name", null: false t.string "extension", null: false - t.text "content", null: false + t.string "content" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false t.integer "index" + t.boolean "default", default: false, null: false + t.index ["index", "project_id"], name: "index_components_on_index_and_project_id", unique: true t.index ["project_id"], name: "index_components_on_project_id" end diff --git a/lib/tasks/project_components/hello_world_example/project_config.yml b/lib/tasks/project_components/hello_world_example/project_config.yml index 0d29c4267..0fa21cbad 100644 --- a/lib/tasks/project_components/hello_world_example/project_config.yml +++ b/lib/tasks/project_components/hello_world_example/project_config.yml @@ -5,11 +5,14 @@ COMPONENTS: extension: "py" location: "main.py" index: 0 + default: true - name: "emoji" extension: "py" location: "emoji.py" index: 1 + default: false - name: "noemoji" extension: "py" location: "noemoji.py" - index: 2 \ No newline at end of file + index: 2 + default: false diff --git a/lib/tasks/project_components/hello_world_starter/project_config.yml b/lib/tasks/project_components/hello_world_starter/project_config.yml index 8e1bc82ce..ae1bd5a8d 100644 --- a/lib/tasks/project_components/hello_world_starter/project_config.yml +++ b/lib/tasks/project_components/hello_world_starter/project_config.yml @@ -5,11 +5,14 @@ COMPONENTS: extension: "py" location: "main.py" index: 0 + default: true - name: "emoji" extension: "py" location: "emoji.py" index: 1 + default: false - name: "noemoji" extension: "py" location: "noemoji.py" - index: 2 \ No newline at end of file + index: 2 + default: false diff --git a/lib/tasks/project_components/target_practice_example/project_config.yml b/lib/tasks/project_components/target_practice_example/project_config.yml index b221ea73c..3fbd1fd5d 100644 --- a/lib/tasks/project_components/target_practice_example/project_config.yml +++ b/lib/tasks/project_components/target_practice_example/project_config.yml @@ -4,4 +4,5 @@ COMPONENTS: - name: "main" extension: "py" location: "main.py" - index: 0 \ No newline at end of file + index: 0 + default: true diff --git a/lib/tasks/project_components/target_practice_starter/project_config.yml b/lib/tasks/project_components/target_practice_starter/project_config.yml index 12de0e7f3..0ea0f287b 100644 --- a/lib/tasks/project_components/target_practice_starter/project_config.yml +++ b/lib/tasks/project_components/target_practice_starter/project_config.yml @@ -4,4 +4,5 @@ COMPONENTS: - name: "main" extension: "py" location: "main.py" - index: 0 \ No newline at end of file + index: 0 + default: true diff --git a/lib/tasks/projects.rake b/lib/tasks/projects.rake index 0e17816ff..3a5e7d33c 100644 --- a/lib/tasks/projects.rake +++ b/lib/tasks/projects.rake @@ -15,7 +15,9 @@ namespace :projects do extension = component['extension'] code = File.read(File.dirname(__FILE__) + "/project_components/#{dir}/#{component['location']}") index = component['index'] - project_component = Component.new(name: name, extension: extension, content: code, index: index) + default = component['default'] + project_component = Component.new(name: name, extension: extension, content: code, index: index, + default: default) new_project.components << project_component end new_project.save diff --git a/spec/concepts/project/operation/update_default_component_spec.rb b/spec/concepts/project/operation/update_default_component_spec.rb new file mode 100644 index 000000000..7d888c323 --- /dev/null +++ b/spec/concepts/project/operation/update_default_component_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Project::Operation::Update, type: :unit do + subject(:update) { described_class.call(project_params, project) } + + let!(:project) { create(:project, :with_default_component) } + let(:default_component) { project.components.first } + + describe '.call' do + context 'when default file is removed' do + let(:project_params) do + { + name: 'updated project name', + components: [] + } + end + + it 'returns failure? true' do + expect(update.failure?).to eq(true) + end + + it 'returns error message' do + expect(update[:error]).to eq(I18n.t('errors.project.editing.delete_default_component')) + end + + it 'does not delete the default component' do + expect { update }.not_to change(Component, :count) + end + + it 'does not update project' do + expect { update }.not_to change { project.reload.name } + end + end + + context 'when default file properties are changed' do + let(:default_component_params) do + default_component.attributes.symbolize_keys.slice( + :id, + :name, + :content, + :extension, + :index + ) + end + + let(:project_params) do + { + name: 'updated project name', + components: [default_component_params] + } + end + + it 'does not update file name' do + default_component_params[:name] = 'Updated name' + expect { update }.not_to change { default_component.reload.name } + end + + it 'does not update file extension' do + default_component_params[:extension] = 'txt' + expect { update }.not_to change { default_component.reload.extension } + end + + it 'does not update project' do + default_component_params[:name] = 'Updated name' + expect { update }.not_to change { project.reload.name } + end + end + end +end diff --git a/spec/concepts/project/operation/update_delete_components_spec.rb b/spec/concepts/project/operation/update_delete_components_spec.rb new file mode 100644 index 000000000..b33ec43d7 --- /dev/null +++ b/spec/concepts/project/operation/update_delete_components_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Project::Operation::Update, type: :unit do + subject(:update) { described_class.call(project_params, project) } + + let!(:project) { create(:project, :with_default_component, :with_components) } + let(:component_to_delete) { project.components.last } + let(:default_component) { project.components.first } + + let(:default_component_params) do + default_component.attributes.symbolize_keys.slice( + :id, + :name, + :content, + :extension, + :index + ) + end + + describe '.call' do + context 'when an existing component has been removed' do + let(:project_params) do + { + name: 'updated project name', + components: [default_component_params] + } + end + + it 'deletes a component' do + expect { update }.to change(Component, :count).by(-1) + end + + it 'deletes the correct component' do + update + expect(Component.find_by(id: component_to_delete.id)).to eq(nil) + end + end + end +end diff --git a/spec/concepts/project/operation/update_invalid_spec.rb b/spec/concepts/project/operation/update_invalid_spec.rb new file mode 100644 index 000000000..2725c1fac --- /dev/null +++ b/spec/concepts/project/operation/update_invalid_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Project::Operation::Update, type: :unit do + subject(:update) do + params = { + name: 'updated project name', + components: [default_component_params, edited_component_params, new_component_params] + } + described_class.call(params, project) + end + + let!(:project) { create(:project, :with_default_component, :with_components, component_count: 2) } + let(:editable_component) { project.components.last } + let(:default_component) { project.components.first } + + let(:edited_component_params) do + { + id: editable_component.id, + name: nil, + content: 'updated content', + extension: 'py', + index: 5 + } + end + + describe '.call' do + context 'when updated project component is invalid' do + it 'returns failure? true' do + expect(update.failure?).to eq(true) + end + + it 'does not amend any project properties' do + expect { update }.not_to change { project.reload.name } + end + + it 'does not amend any component properties' do + expect { update }.not_to change { component_properties_hash(editable_component.reload) } + end + + it 'does not create or delete components' do + expect { update }.not_to change(Component, :count) + end + end + end + + def component_properties_hash(component) + component.attributes.symbolize_keys.slice(:name, :content, :extension, :index) + end + + def default_component_params + default_component.attributes.symbolize_keys.slice( + :id, + :name, + :content, + :extension, + :index + ) + end + + def new_component_params + { + name: 'new component', + content: 'new component content', + extension: 'py', + index: 99 + } + end +end diff --git a/spec/concepts/project/operation/update_spec.rb b/spec/concepts/project/operation/update_spec.rb new file mode 100644 index 000000000..e815ccdd2 --- /dev/null +++ b/spec/concepts/project/operation/update_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Project::Operation::Update, type: :unit do + subject(:update) do + params = { + name: 'updated project name', + components: component_params + } + described_class.call(params, project) + end + + let!(:project) { create(:project, :with_default_component, :with_components) } + let(:editable_component) { project.components.last } + let(:default_component) { project.components.first } + + describe '.call' do + let(:edited_component_params) do + { + id: editable_component.id, + name: 'updated component name', + content: 'updated content', + extension: 'py', + index: 5 + } + end + + context 'when only amending components' do + let(:component_params) { [default_component_params, edited_component_params] } + + it 'returns success? true' do + expect(update.success?).to eq(true) + end + + it 'updates project properties' do + expect { update }.to change { project.reload.name }.to('updated project name') + end + + it 'updates component properties' do + expect { update } + .to change { component_properties_hash(editable_component.reload) } + .to(edited_component_params.slice(:name, :content, :extension, :index)) + end + end + + context 'when a new component has been added' do + let(:component_params) { [default_component_params, edited_component_params, new_component_params] } + + it 'creates a new component' do + expect { update }.to change(Component, :count).by(1) + end + + it 'creates component with correct properties' do + update + created_component = project.components.find_by(**new_component_params) + expect(created_component).not_to eq(nil) + end + end + end + + def component_properties_hash(component) + component.attributes.symbolize_keys.slice(:name, :content, :extension, :index) + end + + def default_component_params + default_component.attributes.symbolize_keys.slice( + :id, + :name, + :content, + :extension, + :index + ) + end + + def new_component_params + { + name: 'new component', + content: 'new component content', + extension: 'py', + index: 99 + } + end +end diff --git a/spec/factories/component.rb b/spec/factories/component.rb index 74c43dba1..20b3b7841 100644 --- a/spec/factories/component.rb +++ b/spec/factories/component.rb @@ -4,6 +4,14 @@ factory :component do name { Faker::Lorem.word } extension { 'py' } - content { Faker::Lorem.paragraph(sentence_count: 2) } + sequence(:index) { |n| n } + default { false } + project + + factory :default_python_component do + name { 'main' } + index { 0 } + default { true } + end end end diff --git a/spec/factories/project.rb b/spec/factories/project.rb index 7badbdcb8..fbcc39fd1 100644 --- a/spec/factories/project.rb +++ b/spec/factories/project.rb @@ -8,8 +8,20 @@ project_type { 'python' } trait :with_components do + transient do + component_count { 1 } + end + + after(:create) do |object, evaluator| + object.components << FactoryBot.create_list(:component, + evaluator.component_count, + project: object) + end + end + + trait :with_default_component do after(:create) do |object| - object.components = FactoryBot.create_list(:component, 2, project: object) + object.components << FactoryBot.create(:default_python_component, project: object) end end end diff --git a/spec/models/component_spec.rb b/spec/models/component_spec.rb index b041ddad8..9fd7812d9 100644 --- a/spec/models/component_spec.rb +++ b/spec/models/component_spec.rb @@ -3,5 +3,41 @@ require 'rails_helper' RSpec.describe Component, type: :model do + subject { build(:component) } + it { is_expected.to belong_to(:project) } + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_presence_of(:extension) } + it { is_expected.to validate_presence_of(:index) } + it { is_expected.to validate_uniqueness_of(:index).scoped_to(:project_id) } + + context 'when default component' do + let(:component) { create(:default_python_component) } + + describe 'validations' do + it 'returns valid? false when name changed' do + component.name = 'updated' + expect(component.valid?).to eq(false) + end + + it 'sets error message when name changed' do + component.name = 'updated' + component.valid? + expect(component.errors[:name]) + .to include(I18n.t('errors.project.editing.change_default_name')) + end + + it 'returns valid? false when extension changed' do + component.extension = 'txt' + expect(component.valid?).to eq(false) + end + + it 'sets error message when extension changed' do + component.extension = 'txt' + component.valid? + expect(component.errors[:extension]) + .to include(I18n.t('errors.project.editing.change_default_extension')) + end + end + end end diff --git a/spec/request/projects/update_spec.rb b/spec/request/projects/update_spec.rb index 0c97d046e..dcaaa422f 100644 --- a/spec/request/projects/update_spec.rb +++ b/spec/request/projects/update_spec.rb @@ -1,16 +1,32 @@ # frozen_string_literal: true require 'rails_helper' +require_relative '../../../lib/operation_response' RSpec.describe 'Project update requests', type: :request do let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } let(:project) { create(:project, user_id: user_id) } context 'when authed user is project creator' do - let(:project) { create(:project, user_id: user_id) } + let(:project) { create(:project, :with_default_component, user_id: user_id) } let!(:component) { create(:component, project: project) } - let(:changes) { { name: 'updated', extension: 'py', content: 'updated content' } } - let(:params) { { project: { components: [{ id: component.id, **changes }] } } } + let(:default_component_params) do + project.components.first.attributes.symbolize_keys.slice( + :id, + :name, + :content, + :extension, + :index + ) + end + + let(:params) do + { project: + { components: [ + default_component_params, + { id: component.id, name: 'updated', extension: 'py', content: 'updated component content' } + ] } } + end before do mock_oauth_user(user_id) @@ -21,10 +37,26 @@ expect(response.status).to eq(200) end - it 'updates component' do + it 'returns updated project json' do put "/api/projects/#{project.identifier}", params: params - updated = component.reload.attributes.symbolize_keys.slice(:name, :content, :extension) - expect(updated).to eq(changes) + expect(response.body).to include('updated component content') + end + + it 'calls update operation' do + mock_response = instance_double(OperationResponse) + allow(mock_response).to receive(:success?).and_return(true) + allow(Project::Operation::Update).to receive(:call).and_return(mock_response) + put "/api/projects/#{project.identifier}", params: params + expect(Project::Operation::Update).to have_received(:call) + end + + context 'when update is invalid' do + let(:params) { { project: { components: [] } } } + + it 'returns error response' do + put "/api/projects/#{project.identifier}", params: params + expect(response.status).to eq(400) + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 01f7c9744..5d227ba75 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -70,12 +70,12 @@ # # Many RSpec users commonly either run the entire suite or an individual # # file, and it's useful to allow more verbose output when running an # # individual spec file. - # if config.files_to_run.one? - # # Use the documentation formatter for detailed output, - # # unless a formatter has already been configured - # # (e.g. via a command-line flag). - # config.default_formatter = "doc" - # end + if config.files_to_run.one? + # Use the documentation formatter for detailed output, + # unless a formatter has already been configured + # (e.g. via a command-line flag). + config.default_formatter = 'doc' + end # # # Print the 10 slowest examples and example groups at the # # end of the spec run, to help surface which specs are running