diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index f44710faf..b7112e4d7 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -27,7 +27,7 @@ def show end def create - result = Project::Create.call(project_hash: project_params) + result = Project::Create.call(project_hash: project_params, current_user:) if result.success? @project = result[:project] diff --git a/app/models/ability.rb b/app/models/ability.rb index bca1f2dfa..1660892d9 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -14,6 +14,8 @@ def initialize(user) define_school_teacher_abilities(user:, school:) if user.school_teacher?(school) define_school_owner_abilities(school:) if user.school_owner?(school) end + + define_experience_cs_admin_abilities(user) end private @@ -100,6 +102,10 @@ def define_school_student_abilities(user:, school:) can(%i[show_finished set_finished], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id) end + def define_experience_cs_admin_abilities(user) + can :create, :public_project if user.experience_cs_admin? + end + def school_teacher_can_manage_lesson?(user:, school:, lesson:) is_my_lesson = lesson.school_id == school.id && lesson.user_id == user.id is_my_class = lesson.school_class&.teacher_ids&.include?(user.id) diff --git a/app/models/project.rb b/app/models/project.rb index ebe956559..3b7f589c5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -7,6 +7,8 @@ module Types SCRATCH = 'scratch' end + attr_accessor :skip_identifier_generation + belongs_to :school, optional: true belongs_to :lesson, optional: true belongs_to :parent, optional: true, class_name: :Project, foreign_key: :remixed_from_id, inverse_of: :remixes @@ -20,7 +22,7 @@ module Types accepts_nested_attributes_for :components - before_validation :check_unique_not_null, on: :create + before_validation :generate_identifier, on: :create, unless: :skip_identifier_generation before_validation :create_school_project_if_needed validates :identifier, presence: true, uniqueness: { scope: :locale } @@ -81,7 +83,7 @@ def media private - def check_unique_not_null + def generate_identifier self.identifier ||= PhraseIdentifier.generate end diff --git a/app/models/user.rb b/app/models/user.rb index a7d90a6be..386ab2615 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -51,7 +51,15 @@ def student? end def admin? - (roles&.to_s&.split(',')&.map(&:strip) || []).include?('editor-admin') + parsed_roles.include?('editor-admin') + end + + def experience_cs_admin? + parsed_roles.include?('experience-cs-admin') + end + + def parsed_roles + roles&.to_s&.split(',')&.map(&:strip) || [] end def ==(other) diff --git a/lib/concepts/project/operations/create.rb b/lib/concepts/project/operations/create.rb index 625cdba85..7e40da299 100644 --- a/lib/concepts/project/operations/create.rb +++ b/lib/concepts/project/operations/create.rb @@ -3,10 +3,13 @@ class Project class Create class << self - def call(project_hash:) + def call(project_hash:, current_user:) response = OperationResponse.new - response[:project] = build_project(project_hash) - response[:project].save! + + project = build_project(project_hash, current_user) + project.save! + + response[:project] = project response rescue StandardError => e Sentry.capture_exception(e) @@ -16,9 +19,8 @@ def call(project_hash:) private - def build_project(project_hash) - identifier = PhraseIdentifier.generate - new_project = Project.new(project_hash.except(:components).merge(identifier:)) + def build_project(project_hash, _current_user) + new_project = Project.new(project_hash.except(:components)) new_project.components.build(project_hash[:components]) new_project end diff --git a/spec/concepts/project/create_spec.rb b/spec/concepts/project/create_spec.rb index 2af9426f1..4619a4b79 100644 --- a/spec/concepts/project/create_spec.rb +++ b/spec/concepts/project/create_spec.rb @@ -3,21 +3,17 @@ require 'rails_helper' RSpec.describe Project::Create, type: :unit do - subject(:create_project) { described_class.call(project_hash:) } - - let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } + describe '.call' do + subject(:create_project) { described_class.call(project_hash:, current_user:) } - before do - mock_phrase_generation - ActionController::Parameters.permit_all_parameters = true - end + let(:current_user) { build(:user) } + let(:user_id) { current_user.id } - describe '.call' do - let(:project_hash) { ActionController::Parameters.new({}).merge(user_id:) } + before do + mock_phrase_generation + end context 'with valid content' do - subject(:create_project_with_content) { described_class.call(project_hash:) } - let(:project_hash) do { project_type: Project::Types::PYTHON, @@ -32,16 +28,18 @@ end it 'returns success' do - expect(create_project_with_content.success?).to be(true) + expect(create_project.success?).to be(true) end it 'returns project with correct component content' do - new_project = create_project_with_content[:project] + new_project = create_project[:project] expect(new_project.components.first.content).to eq('print("hello world")') end end context 'when creation fails' do + let(:project_hash) { { user_id: } } + before do mock_project = instance_double(Project) allow(mock_project).to receive(:components).and_raise('Some error') diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 473db5ec7..d7470f672 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -11,6 +11,10 @@ roles { 'editor-admin' } end + factory :experience_cs_admin_user do + roles { 'experience-cs-admin' } + end + factory :student do email { nil } username { Faker::Internet.username } diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index bbcd47836..dbd876540 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -3,16 +3,11 @@ require 'rails_helper' RSpec.describe 'Creating a project', type: :request do - before do - authenticated_in_hydra_as(teacher) - mock_phrase_generation - end - + let(:generated_identifier) { 'word1-word2-word3' } let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:teacher) { create(:teacher, school:) } let(:school) { create(:school) } let(:owner) { create(:owner, school:) } - let(:params) do { project: { @@ -24,11 +19,23 @@ } end + before do + authenticated_in_hydra_as(teacher) + mock_phrase_generation(generated_identifier) + end + it 'responds 201 Created' do post('/api/projects', headers:, params:) expect(response).to have_http_status(:created) end + it 'generates an identifier for the project' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:identifier]).to eq(generated_identifier) + end + it 'responds with the project JSON' do post('/api/projects', headers:, params:) data = JSON.parse(response.body, symbolize_names: true) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 39624d64e..9d7a40aea 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -29,6 +29,8 @@ it { is_expected.not_to be_able_to(:update, project) } it { is_expected.not_to be_able_to(:destroy, project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with a standard user' do @@ -56,6 +58,8 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with a teacher' do @@ -83,6 +87,8 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with an owner' do @@ -110,6 +116,8 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with a student' do @@ -137,6 +145,14 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } + end + + context 'with an experience-cs admin' do + let(:user) { build(:experience_cs_admin_user) } + + it { is_expected.to be_able_to(:create, :public_project) } end # rubocop:disable RSpec/MultipleMemoizedHelpers diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6102ac091..ab56c8471 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -154,12 +154,19 @@ end end - describe 'check_unique_not_null' do - let(:saved_project) { create(:project) } - + describe 'generate_identifier' do it 'generates an identifier if nil' do - unsaved_project = build(:project, identifier: nil) - expect { unsaved_project.valid? }.to change { unsaved_project.identifier.nil? }.from(true).to(false) + project = build(:project, identifier: nil) + project.valid? + expect(project.identifier).not_to be_nil + end + + context 'when skip_identifier_generation is true' do + it 'does not generate an identifier if nil' do + project = build(:project, identifier: nil, skip_identifier_generation: true) + project.valid? + expect(project.identifier).to be_nil + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 103e90ada..7cc065aa6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -277,6 +277,28 @@ end end + describe '#parsed_roles' do + it 'returns array of role names when roles is set to comma-separated string' do + user = build(:user, roles: 'role-1,role-2') + expect(user.parsed_roles).to eq(%w[role-1 role-2]) + end + + it 'strips leading & trailing spaces from role names' do + user = build(:user, roles: ' role-1 , role-2 ') + expect(user.parsed_roles).to eq(%w[role-1 role-2]) + end + + it 'returns empty array when roles is set to empty string' do + user = build(:user, roles: '') + expect(user.parsed_roles).to eq([]) + end + + it 'returns empty array when roles is set to nil' do + user = build(:user, roles: nil) + expect(user.parsed_roles).to eq([]) + end + end + describe '#admin?' do it 'returns true if the user has the editor-admin role in Hydra' do user = build(:user, roles: 'editor-admin') @@ -287,15 +309,17 @@ user = build(:user, roles: 'another-editor-admin') expect(user).not_to be_admin end + end - it 'returns false if roles are empty in Hydra' do - user = build(:user, roles: '') - expect(user).not_to be_admin + describe '#experience_cs_admin?' do + it 'returns true if the user has the experience-cs-admin role in Hydra' do + user = build(:user, roles: 'experience-cs-admin') + expect(user).to be_experience_cs_admin end - it 'returns false if roles are nil in Hydra' do - user = build(:user, roles: nil) - expect(user).not_to be_admin + it 'returns false if the user does not have the experience-cs-admin role in Hydra' do + user = build(:user, roles: 'another-admin') + expect(user).not_to be_experience_cs_admin end end