Skip to content

Allow Experience CS admins to manage starter/public projects #552

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

Merged
merged 10 commits into from
May 28, 2025
6 changes: 3 additions & 3 deletions 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 Expand Up @@ -80,8 +80,8 @@ def load_projects
end

def project_params
if school_owner?
# A school owner must specify who the project user is.
if school_owner? || current_user&.experience_cs_admin?
# A school owner or an Experience CS admin must specify who the project user is.
base_params
else
# A school teacher may only create projects they own.
Expand Down
2 changes: 1 addition & 1 deletion app/graphql/mutations/create_project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def resolve(**input)
components: input[:components]&.map(&:to_h)
)

response = Project::Create.call(project_hash:)
response = Project::Create.call(project_hash:, current_user: context[:current_user])
raise GraphQL::ExecutionError, response[:error] unless response.success?

{ project: response[:project] }
Expand Down
8 changes: 8 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,12 @@ 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)
return unless user&.experience_cs_admin?

can %i[read create update destroy], Project, user_id: nil
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
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
10 changes: 5 additions & 5 deletions lib/concepts/project/operations/create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
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] = build_project(project_hash, current_user)
response[:project].save!
response
rescue StandardError => e
Expand All @@ -16,9 +16,9 @@ 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)
project_hash[:identifier] = PhraseIdentifier.generate unless current_user&.experience_cs_admin?
new_project = Project.new(project_hash.except(:components))
new_project.components.build(project_hash[:components])
new_project
end
Expand Down
7 changes: 4 additions & 3 deletions spec/concepts/project/create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
require 'rails_helper'

RSpec.describe Project::Create, type: :unit do
subject(:create_project) { described_class.call(project_hash:) }
subject(:create_project) { described_class.call(project_hash:, current_user:) }

let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' }
let(:current_user) { create(:user) }
let(:user_id) { current_user.id }

before do
mock_phrase_generation
Expand All @@ -16,7 +17,7 @@
let(:project_hash) { ActionController::Parameters.new({}).merge(user_id:) }

context 'with valid content' do
subject(:create_project_with_content) { described_class.call(project_hash:) }
subject(:create_project_with_content) { described_class.call(project_hash:, current_user:) }

let(:project_hash) do
{
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
80 changes: 74 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,24 @@
}
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 even if another identifier is specified' do
params_with_identifier = { project: { identifier: 'test-identifier', components: [] } }
post('/api/projects', headers:, params: params_with_identifier)
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 Expand Up @@ -200,4 +208,64 @@
expect(response).to have_http_status(:forbidden)
end
end

context 'when an Experience CS admin creates a starter Scratch project' do
let(:experience_cs_admin) { create(:experience_cs_admin_user) }
let(:params) do
{
project: {
identifier: 'test-project',
name: 'Test Project',
locale: 'fr',
project_type: Project::Types::SCRATCH,
user_id: nil,
components: []
}
}
end

before do
authenticated_in_hydra_as(experience_cs_admin)
end

it 'responds 201 Created' do
post('/api/projects', headers:, params:)
expect(response).to have_http_status(:created)
end

it 'sets the project identifier to the specified (not the generated) value' do
post('/api/projects', headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:identifier]).to eq('test-project')
end

it 'sets the project name to the specified value' do
post('/api/projects', headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:name]).to eq('Test Project')
end

it 'sets the project locale to the specified value' do
post('/api/projects', headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:locale]).to eq('fr')
end

it 'sets the project type to the specified value' do
post('/api/projects', headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:project_type]).to eq(Project::Types::SCRATCH)
end

it 'sets the project user_id to the specified value (i.e. nil to represent a public project)' do
post('/api/projects', headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:user_id]).to be_nil
end
end
end
27 changes: 26 additions & 1 deletion spec/features/project/updating_a_project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
end

let(:headers) { { Authorization: UserProfileMock::TOKEN } }
let!(:project) { create(:project, name: 'Test Project', user_id: owner.id) }
let(:project_type) { Project::Types::PYTHON }
let(:user_id) { owner.id }
let!(:project) { create(:project, name: 'Test Project', user_id:, locale: 'en', project_type:) }
let(:owner) { create(:owner, school:) }
let(:school) { create(:school) }

Expand Down Expand Up @@ -53,4 +55,27 @@
put("/api/projects/#{project.id}", params:)
expect(response).to have_http_status(:unauthorized)
end

context 'when an Experience CS admin creates a starter Scratch project' do
let(:experience_cs_admin) { create(:experience_cs_admin_user) }
let(:user_id) { nil }
let(:project_type) { Project::Types::SCRATCH }
let(:params) { { project: { name: 'Test Project' } } }

before do
authenticated_in_hydra_as(experience_cs_admin)
end

it 'responds 200 OK' do
put("/api/projects/#{project.identifier}?project_type=scratch", headers:, params:)
expect(response).to have_http_status(:success)
end

it 'sets the project name to the specified value' do
put("/api/projects/#{project.identifier}?project_type=scratch", headers:, params:)
data = JSON.parse(response.body, symbolize_names: true)

expect(data[:name]).to eq('Test Project')
end
end
end
26 changes: 26 additions & 0 deletions spec/models/ability_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,32 @@
end
end

context 'with an experience-cs admin' do
let(:user) { build(:experience_cs_admin_user, id: user_id) }
let(:another_project) { build(:project) }

context 'with a starter project' do
it { is_expected.to be_able_to(:read, starter_project) }
it { is_expected.to be_able_to(:create, starter_project) }
it { is_expected.to be_able_to(:update, starter_project) }
it { is_expected.to be_able_to(:destroy, starter_project) }
end

context 'with own project' do
it { is_expected.to be_able_to(:read, project) }
it { is_expected.to be_able_to(:create, project) }
it { is_expected.to be_able_to(:update, project) }
it { is_expected.to be_able_to(:destroy, project) }
end

context 'with another user\'s project' do
it { is_expected.not_to be_able_to(:read, another_project) }
it { is_expected.not_to be_able_to(:create, another_project) }
it { is_expected.not_to be_able_to(:update, another_project) }
it { is_expected.not_to be_able_to(:destroy, another_project) }
end
end

# rubocop:disable RSpec/MultipleMemoizedHelpers
context "with a teacher's project where the lesson is visible to students" do
let(:user) { create(:user) }
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(:experience_cs_admin_user)
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
23 changes: 23 additions & 0 deletions spec/requests/projects/destroy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,29 @@
expect(response).to have_http_status(:forbidden)
end
end

context 'when an Experience CS admin destroys a starter Scratch project' do
let(:project) do
create(
:project, {
project_type: Project::Types::SCRATCH,
user_id: nil,
locale: 'en'
}
)
end
let(:experience_cs_admin) { create(:experience_cs_admin_user) }

before do
authenticated_in_hydra_as(experience_cs_admin)
end

it 'deletes the project' do
expect do
delete("/api/projects/#{project.identifier}?project_type=scratch", headers:)
end.to change(Project.unscoped, :count).by(-1)
end
end
end

context 'when no token is given' do
Expand Down
3 changes: 2 additions & 1 deletion spec/support/user_profile_mock.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ def user_to_hash(user, user_type, id_field = :id)
id_field => user_type ? "#{user_type}:#{user.id}" : user.id,
name: user.name,
email: user.email,
username: user.username
username: user.username,
roles: user.roles
}
end

Expand Down