Skip to content

Allow ExperienceCS admins to create public projects v2 #551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
6 changes: 6 additions & 0 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 4 additions & 2 deletions app/models/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 }
Expand Down Expand Up @@ -81,7 +83,7 @@ def media

private

def check_unique_not_null
def generate_identifier
self.identifier ||= PhraseIdentifier.generate
end

Expand Down
10 changes: 9 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 8 additions & 6 deletions lib/concepts/project/operations/create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
24 changes: 11 additions & 13 deletions spec/concepts/project/create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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')
Expand Down
4 changes: 4 additions & 0 deletions spec/factories/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
19 changes: 13 additions & 6 deletions spec/features/project/creating_a_project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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)
Expand Down
16 changes: 16 additions & 0 deletions spec/models/ability_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
17 changes: 12 additions & 5 deletions spec/models/project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
36 changes: 30 additions & 6 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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

Expand Down