diff --git a/apps/src/templates/teacherDashboard/teacherSectionsRedux.js b/apps/src/templates/teacherDashboard/teacherSectionsRedux.js index df79033dfcced..8e7659ca96e14 100644 --- a/apps/src/templates/teacherDashboard/teacherSectionsRedux.js +++ b/apps/src/templates/teacherDashboard/teacherSectionsRedux.js @@ -161,10 +161,13 @@ export const finishEditingSection = () => (dispatch, getState) => { dispatch({type: EDIT_SECTION_REQUEST}); const state = getState().teacherSections; const section = state.sectionBeingEdited; + + const dataUrl = isAddingSection(state) ? '/dashboardapi/sections' : `/sections/${section.id}`; + const httpMethod = isAddingSection(state) ? 'POST' : 'PATCH'; return new Promise((resolve, reject) => { $.ajax({ - url: isAddingSection(state) ? '/dashboardapi/sections' : `/v2/sections/${section.id}/update`, - method: 'POST', + url: dataUrl, + method: httpMethod, contentType: 'application/json;charset=UTF-8', data: JSON.stringify(serverSectionFromSection(section)), }).done(result => { diff --git a/apps/test/unit/templates/teacherDashboard/teacherSectionsReduxTest.js b/apps/test/unit/templates/teacherDashboard/teacherSectionsReduxTest.js index c42f1e881bae1..b1f33a643728a 100644 --- a/apps/test/unit/templates/teacherDashboard/teacherSectionsReduxTest.js +++ b/apps/test/unit/templates/teacherDashboard/teacherSectionsReduxTest.js @@ -704,7 +704,7 @@ describe('teacherSectionsRedux', () => { store.dispatch(editSectionProperties({grade: 'K'})); // Set up matching server response - server.respondWith('POST', `/v2/sections/${sectionId}/update`, + server.respondWith('PATCH', `/sections/${sectionId}`, successResponse({grade: 'K'})); store.dispatch(finishEditingSection()); @@ -779,7 +779,7 @@ describe('teacherSectionsRedux', () => { it('sets and clears saveInProgress', () => { const sectionId = 12; server.autoRespond = true; - server.respondWith('POST', `/v2/sections/${sectionId}/update`, + server.respondWith('PATCH', `/sections/${sectionId}`, successResponse(sectionId)); expect(isSaveInProgress(getState())).to.be.false; @@ -794,7 +794,7 @@ describe('teacherSectionsRedux', () => { it('updates an edited section in the section map on success', () => { const sectionId = 12; server.autoRespond = true; - server.respondWith('POST', `/v2/sections/${sectionId}/update`, + server.respondWith('PATCH', `/sections/${sectionId}`, successResponse(sectionId, {login_type: 'word'})); expect(state().sections[sectionId].loginType).to.equal('picture'); diff --git a/dashboard/app/controllers/sections_controller.rb b/dashboard/app/controllers/sections_controller.rb index 6e4d4f35bfe7a..1c3bff4554e5b 100644 --- a/dashboard/app/controllers/sections_controller.rb +++ b/dashboard/app/controllers/sections_controller.rb @@ -7,14 +7,17 @@ def show @secret_pictures = SecretPicture.all.shuffle end - # Allows you to update a section's course_id. Clears any assigned script_id - # in the process + # Allows you to update a section. Clears any assigned script_id in the process def update section = Section.find(params[:id]) authorize! :manage, section course_id = params[:course_id] - script_id = params[:script_id] + + # This endpoint needs to satisfy two endpoint formats for getting script_id + # This should be updated soon to always expect params[:script_id] + script_id = params[:script][:id] if params[:script] + script_id ||= params[:script_id] if script_id script = Script.get_from_cache(script_id) @@ -24,13 +27,24 @@ def update course_id ||= script.course.try(:id) end - section.update!(course_id: course_id, script_id: script_id) + # TODO: (madelynkasula) refactor to use strong params + fields = {} + fields[:course_id] = course_id + fields[:script_id] = script_id + fields[:name] = params[:name] unless params[:name].nil_or_empty? + fields[:login_type] = params[:login_type] if Section.valid_login_type?(params[:login_type]) + fields[:grade] = params[:grade] if Section.valid_grade?(params[:grade]) + fields[:stage_extras] = params[:stage_extras] unless params[:stage_extras].nil? + fields[:pairing_allowed] = params[:pairing_allowed] unless params[:pairing_allowed].nil? + fields[:hidden] = params[:hidden] unless params[:hidden].nil? + + section.update!(fields) if script_id section.students.each do |student| student.assign_script(script) end end - render json: {} + render json: section.summarize end def log_in diff --git a/dashboard/test/controllers/sections_controller_test.rb b/dashboard/test/controllers/sections_controller_test.rb index 75a19bdffb181..d191ff5a7b6c7 100644 --- a/dashboard/test/controllers/sections_controller_test.rb +++ b/dashboard/test/controllers/sections_controller_test.rb @@ -160,15 +160,40 @@ class SectionsControllerTest < ActionController::TestCase test "update: can update section you own" do sign_in @teacher - section_with_script = create(:section, user: @teacher, script_id: Script.flappy_script.id) + section_with_script = create( + :section, + user: @teacher, + script_id: Script.flappy_script.id, + login_type: Section::LOGIN_TYPE_WORD, + grade: "1", + stage_extras: true, + pairing_allowed: false, + hidden: true + ) + post :update, params: { id: section_with_script.id, - course_id: @course.id + course_id: @course.id, + name: "My Section", + login_type: Section::LOGIN_TYPE_PICTURE, + grade: "K", + stage_extras: false, + pairing_allowed: true, + hidden: false } assert_response :success - section_with_script.reload + + # Cannot use section_with_script.reload because login_type has changed + section_with_script = Section.find(section_with_script.id) + assert_equal(@course.id, section_with_script.course_id) - assert_nil section_with_script.script_id + assert_nil(section_with_script.script_id) + assert_equal("My Section", section_with_script.name) + assert_equal(Section::LOGIN_TYPE_PICTURE, section_with_script.login_type) + assert_equal("K", section_with_script.grade) + assert_equal(false, section_with_script.stage_extras) + assert_equal(true, section_with_script.pairing_allowed) + assert_equal(false, section_with_script.hidden) end test "update: cannot update section you dont own" do @@ -241,4 +266,16 @@ class SectionsControllerTest < ActionController::TestCase assert_not_nil UserScript.find_by(script: Script.artist_script, user: student) end + + test "update: can set script from nested script param" do + sign_in @teacher + section = create(:section, user: @teacher, script_id: Script.flappy_script.id) + post :update, as: :json, params: { + id: section.id, + script: {id: @script_in_course.id} + } + assert_response :success + section.reload + assert_equal(@script_in_course.id, section.script_id) + end end diff --git a/pegasus/routes/v2_section_routes.rb b/pegasus/routes/v2_section_routes.rb index d1f2e2b898847..b76a20986f349 100644 --- a/pegasus/routes/v2_section_routes.rb +++ b/pegasus/routes/v2_section_routes.rb @@ -86,7 +86,14 @@ call(env.merge('REQUEST_METHOD' => 'DELETE', 'PATH_INFO' => "/v2/sections/#{id}")) end +# DEPRECATED: Use PATCH /sections/ instead patch '/v2/sections/:id' do |id| + # Notify Honeybadger to determine if this endpoint is still used anywhere + Honeybadger.notify( + error_class: "DeprecatedEndpointWarning", + error_message: 'Deprecated endpoint PATCH /v2/sections/:id called unexpectedly', + ) + only_for 'code.org' dont_cache unsupported_media_type! unless payload = request.json_body