Merge pull request #22951 from code-dot-org/revert-22949-revert-22902…

…-update-sections-dashboard

Revert "Revert "Update PATCH /sections/:id in dashboard, deprecate pegasus endpoint""

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 => {

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');

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

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

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/<id> 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