-
Notifications
You must be signed in to change notification settings - Fork 9
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
Copy courses between providers in Support console #4750
Changes from 10 commits
fd0dcc0
a7eaf83
3db0161
75eb78d
6f006a7
df1b10b
8561c06
07eaba3
afcf251
14639b8
ffaf095
075b4b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
# frozen_string_literal: true | ||
|
||
module Support | ||
module Providers | ||
class CopyCoursesController < SupportController | ||
before_action :recruitment_cycle | ||
|
||
def new | ||
@target_provider = Provider.find(params[:provider_id]) | ||
@copy_courses_form = CopyCoursesForm.new(@target_provider) | ||
end | ||
|
||
def create | ||
@target_provider = Provider.find(params[:provider_id]) | ||
@copy_courses_form = CopyCoursesForm.new(@target_provider, recruitment_cycle.providers.find_by(provider_code: params[:course][:autocompleted_provider_code])) | ||
|
||
if @copy_courses_form.valid? | ||
sites_copy_to_course = params[:schools] ? Sites::CopyToCourseService : ->(*) {} | ||
|
||
copier = ::Courses::CopyToProviderService.new(sites_copy_to_course:, enrichments_copy_to_course: Enrichments::CopyToCourseService.new, force: true) | ||
|
||
Provider.transaction do | ||
@copy_courses_form.provider.courses.map do |course| | ||
copier.execute(course:, new_provider: @copy_courses_form.target_provider) | ||
end | ||
end | ||
|
||
redirect_to support_recruitment_cycle_provider_path(recruitment_cycle.year, @copy_courses_form.target_provider.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any indication everything went sucessfully? Should we add one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Screencast.from.10-12-24.10.47.38.webmThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll change it to redirect to the courses tab too |
||
else | ||
render :new | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# frozen_string_literal: true | ||
|
||
module Support | ||
class CopyCoursesForm | ||
include ActiveModel::Model | ||
|
||
attr_reader :target_provider, :provider | ||
|
||
def initialize(target_provider, provider = nil) | ||
@target_provider = target_provider | ||
@provider = provider | ||
end | ||
|
||
validates :target_provider, :provider, presence: true | ||
validate :different_providers | ||
|
||
def different_providers | ||
errors.add(:provider, message: 'Choose different providers') if target_provider == provider | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
<%= render PageTitle.new(title: "support.providers.copy_courses.new", has_errors: @copy_courses_form.errors.present?) %> | ||
|
||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-two-thirds-from-desktop"> | ||
<div> | ||
<%= form_with model: @copy_courses_form, url: support_recruitment_cycle_provider_copy_courses_path(@recruitment_cycle.year, @target_provider) do |form| %> | ||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-full"> | ||
<%= form.govuk_error_summary %> | ||
</div> | ||
</div> | ||
<h1 class="govuk-heading-l"><%= @target_provider.provider_name %></h1> | ||
<h2 class="govuk-heading-m">Copy courses from:</h2> | ||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-full"> | ||
<div class="govuk-form-group"> | ||
<%= form.govuk_text_field :provider, | ||
id: "provider", | ||
value: params[:query], | ||
class: "govuk-input", | ||
data: { qa: "provider-search" }, | ||
label: -> { tag.div("Search for an organisation to copy courses from:") + tag.div("Enter the name or provider code", class: %w[govuk-hint govuk-!-display-inline]) } %> | ||
<div id="provider-autocomplete"></div> | ||
</div> | ||
</div> | ||
</div> | ||
|
||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-two-thirds"> | ||
<div class="govuk-form-group"> | ||
<%= form.govuk_check_box :schools, true, false, label: { text: "Copy placement schools?" }, hint: { text: tag.div("Any placement schools that are not already linked to the target provider will not be copied", class: %w[govuk-hint govuk-!-font-size-16]) }, multiple: false %> | ||
</div> | ||
</div> | ||
</div> | ||
|
||
<div class="govuk-grid-row"> | ||
<div class="govuk-grid-column-two-thirds"> | ||
<%= form.govuk_submit "Copy courses" %> | ||
</div> | ||
</div> | ||
<% end %> | ||
</div> | ||
</div> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
|
||
module Support | ||
describe CopyCoursesForm, type: :model do | ||
subject { described_class.new(target_provider, provider) } | ||
|
||
let(:provider) { create(:provider) } | ||
|
||
describe 'validations' do | ||
let(:target_provider) { provider } | ||
|
||
context 'unique provider' do | ||
it 'is not valid' do | ||
expect(subject).not_to be_valid | ||
expect(subject.errors.messages[:provider]).to include('Choose different providers') | ||
end | ||
end | ||
|
||
context 'provider must be present' do | ||
let(:target_provider) { nil } | ||
|
||
it 'is not valid' do | ||
expect(subject).not_to be_valid | ||
expect(subject.errors.messages[:target_provider]).to include("Provider can't be blank") | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'rails_helper' | ||
|
||
RSpec.describe 'Support::CopyCourses' do | ||
include DfESignInUserHelper | ||
let(:user) { create(:user, :admin) } | ||
let(:source_provider) { create(:provider, courses: create_list(:course, 3, :with_full_time_sites)) } | ||
let!(:target_provider) { create(:provider, sites: source_provider.courses.flat_map(&:sites)) } | ||
let!(:year) { find_or_create(:recruitment_cycle).year } | ||
|
||
before { host! URI(Settings.base_url).host } | ||
|
||
describe 'GET new' do | ||
it 'responds with 200' do | ||
login_user(user) | ||
get "/support/2025/providers/#{source_provider.id}/copy_courses/new" | ||
expect(response).to have_http_status(:ok) | ||
end | ||
end | ||
|
||
describe 'POST create' do | ||
context 'with schools' do | ||
it 'copies the courses with schools' do | ||
login_user(user) | ||
post "/support/#{year}/providers/#{target_provider.id}/copy_courses", params: { 'course[autocompleted_provider_code]' => source_provider.provider_code, schools: '1' } | ||
expect(target_provider.reload.courses.length).to eq(3) | ||
courses = target_provider.reload.courses | ||
expect(courses.flat_map(&:sites).length).to eq(3) | ||
end | ||
end | ||
|
||
context 'without schools' do | ||
it 'copies the course without schools' do | ||
login_user(user) | ||
post "/support/#{year}/providers/#{target_provider.id}/copy_courses", params: { 'course[autocompleted_provider_code]' => source_provider.provider_code } | ||
expect(target_provider.reload.courses.length).to eq(3) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'spec_helper' | ||
|
||
RSpec.describe 'Support', service: :publish, skip: 'can not get chromedriver working on Alpine' do | ||
include DfESignInUserHelper | ||
|
||
let(:courses) do | ||
[ | ||
create(:course, :unpublished, :with_full_time_sites), | ||
create(:course, :published, :with_full_time_sites), | ||
create(:course, :withdrawn, :with_full_time_sites) | ||
] | ||
end | ||
let!(:source_provider) { create(:provider, provider_name: 'Source Provider', courses:) } | ||
let!(:target_provider) { create(:provider, provider_name: 'Target Provider') } | ||
let(:user) { create(:user, :admin) } | ||
|
||
before do | ||
sign_in_system_test(user:) | ||
end | ||
|
||
it 'copy courses from one provider to another', :js do | ||
visit '/support' | ||
click_on 'Target Provider' | ||
click_on 'Courses' | ||
click_on 'Copy Courses' | ||
autocomplete = page.find('input#provider') | ||
autocomplete.set(source_provider.provider_code) | ||
sleep 3 | ||
li = page.find('ul#provider__listbox li', visible: false) | ||
li.click | ||
page.find_by_id('schools-true-field', visible: false).click | ||
click_on 'Copy courses' | ||
click_on 'Courses' | ||
courses.map(&:name).each do |course_name| | ||
expect(page).to have_content(course_name) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the PR description it mentions "Copy all courses or some courses" but here is all courses. Can we remove the "some courses" from description just to avoid confusion?
I assume the copying specific courses would be done on a later interaction when we will have a support ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've not implemented "some courses" because unwanted courses can be deleted