feat: reorder module contents with move buttons

app/controllers/admin/content_modules/positions_controller.rb

@@ -0,0 +1,13 @@
+class Admin::ContentModules::PositionsController < AdminController
+  def update
+    content_module = ContentModule.find(params.expect(:content_module_id))
+
+    case params.expect(:direction)
+    when "up" then content_module.move_up
+    when "down" then content_module.move_down
+    else return head :unprocessable_entity
+    end
+
+    redirect_to admin_content_modules_path(program_id: content_module.program_id), status: :see_other
+  end
+end

app/controllers/admin/content_modules_controller.rb

@@ -51,6 +51,6 @@ def set_content_module
     end
 
     def content_module_params
-      params.expect(content_module: [ :name, :program_id, :level, :position ])
+      params.expect(content_module: [ :name, :program_id, :level ])
     end
 end

app/controllers/admin/contents/positions_controller.rb

@@ -0,0 +1,13 @@
+class Admin::Contents::PositionsController < AdminController
+  def update
+    content = Content.find(params.expect(:content_id))
+
+    case params.expect(:direction)
+    when "up" then content.move_up
+    when "down" then content.move_down
+    else return head :unprocessable_entity
+    end
+
+    redirect_to edit_admin_content_module_path(content.content_module), status: :see_other
+  end
+end

app/models/concerns/ordered.rb

@@ -1,7 +1,40 @@
+# Orders records by an integer +position+ column and lets them swap places with a neighbor.
+#
+# A model that wants to move records must define +position_siblings+, the relation of
+# records it moves among (for example, the contents of one module).
 module Ordered
   extend ActiveSupport::Concern
 
   included do
     scope :ordered, -> { order(:position) }
   end
+
+  def move_up
+    swap_position_with position_siblings.where(position: ...position).order(position: :desc).first
+  end
+
+  def move_down
+    swap_position_with position_siblings.where("position > ?", position).order(:position).first
+  end
+
+  private
+
+  def position_siblings
+    raise NotImplementedError, "#{self.class} must define #position_siblings to be moved"
+  end
+
+  def swap_position_with(neighbor)
+    return unless neighbor
+
+    transaction do
+      own_position = position
+      update!(position: neighbor.position)
+      neighbor.update!(position: own_position)
+    end
+  end
+
+  # Models opt in with +before_create :append_to_siblings+.
+  def append_to_siblings
+    self.position ||= (position_siblings.maximum(:position) || 0) + 1
+  end
 end

app/models/content.rb

@@ -7,40 +7,18 @@ class Content < ApplicationRecord
 
   validates :contentable_id, uniqueness: { scope: [ :content_module_id, :contentable_type ] }
 
-  before_create :append_to_content_module
+  before_create :append_to_siblings
 
   # Ensure the associated contentable record is destroyed if allowed before destroying this content.
   # This allows us to have dependent: :destroy behavior for contentable records.
   before_destroy :destroy_owned_record
 
-  def move_up
-    swap_position_with siblings.where(position: ...position).order(position: :desc).first
-  end
-
-  def move_down
-    swap_position_with siblings.where("position > ?", position).order(:position).first
-  end
-
   private
 
-  def siblings
+  def position_siblings
     content_module.contents.unscope(:order)
   end
 
-  def swap_position_with(neighbor)
-    return unless neighbor
-
-    transaction do
-      own_position = position
-      update!(position: neighbor.position)
-      neighbor.update!(position: own_position)
-    end
-  end
-
-  def append_to_content_module
-    self.position = (siblings.maximum(:position) || 0) + 1
-  end
-
   def destroy_owned_record
     contentable.destroy if contentable.destroy_with_attachable?
   end

app/models/content_module.rb

@@ -16,4 +16,20 @@ class ContentModule < ApplicationRecord
   enum :level, { basic: "basic", moderate: "moderate", advanced: "advanced" }, validate: true
 
   validates :name, :level, presence: true
+
+  before_create :append_to_siblings
+  before_update :append_to_siblings, if: -> { program_id_changed? || level_changed? }
+
+  private
+
+  # Modules are ordered within one program and level, the same grouping the admin index shows.
+  def position_siblings
+    ContentModule.where(program_id: program_id, level: level)
+  end
+
+  # A module that joins another program or level goes to the end of it.
+  def append_to_siblings
+    self.position = nil if persisted?
+    super
+  end
 end

app/views/admin/content_modules/_form.html.erb

@@ -22,9 +22,6 @@
     <%= form.select :level, ContentModule.levels.keys.map { |l| [ l.humanize, l ] },
           { include_blank: "Select a level" }, class: "select w-full" %>
 
-    <%= form.label :position, class: "label" %>
-    <%= form.number_field :position, class: "input w-full" %>
-
     <%= form.submit class: "btn btn-primary" %>
   </div>
 <% end %>

app/views/admin/content_modules/edit.html.erb

@@ -42,8 +42,9 @@
               </tr>
             </thead>
             <tbody>
-              <% @contents.each do |content| %>
-                <%= render "admin/contents/contentables/#{content.contentable_name}", content: content %>
+              <% @contents.each_with_index do |content, index| %>
+                <%= render "admin/contents/contentables/#{content.contentable_name}", content: content,
+                      first: index.zero?, last: index == @contents.size - 1 %>
               <% end %>
             </tbody>
           </table>

app/views/admin/content_modules/index.html.erb

@@ -34,12 +34,14 @@
             </tr>
           </thead>
           <tbody>
-            <% modules.each do |mod| %>
+            <% modules.each_with_index do |mod, index| %>
               <tr>
                 <td><%= mod.name %></td>
                 <td><%= mod.contents.size %></td>
                 <td>
                   <div class="flex gap-2">
+                    <%= render "admin/shared/move_buttons", url: admin_content_module_position_path(mod), title: mod.name,
+                          first: index.zero?, last: index == modules.size - 1 %>
                     <%= link_to "Edit", edit_admin_content_module_path(mod), class: "btn btn-sm btn-primary" %>
                     <%= button_to "Delete", admin_content_module_path(mod), method: :delete,
                           data: { "turbo-confirm": "Delete this module?" },

app/views/admin/contents/contentables/_game.html.erb

@@ -4,6 +4,7 @@
   <td>Game</td>
   <td>
     <div class="flex gap-2">
+      <%= render "admin/shared/move_buttons", url: admin_content_position_path(content), title: game.title, first: first, last: last %>
       <%= link_to "Edit", edit_admin_game_path(game), class: "btn btn-xs btn-primary" %>
       <%= button_to "Remove", admin_content_path(content), method: :delete,
             data: { "turbo-confirm": "Remove this game from the module?" },

app/views/admin/contents/contentables/_link.html.erb

@@ -4,6 +4,7 @@
   <td><%= link.link_type.humanize %></td>
   <td>
     <div class="flex gap-2">
+      <%= render "admin/shared/move_buttons", url: admin_content_position_path(content), title: link.title, first: first, last: last %>
       <%= link_to "Edit", edit_admin_link_path(link), class: "btn btn-xs btn-primary" %>
       <%= button_to "Delete", admin_content_path(content), method: :delete,
             data: { "turbo-confirm": "Delete this link?" },

app/views/admin/shared/_move_buttons.html.erb

@@ -0,0 +1,5 @@
+<%# locals: (url:, title:, first:, last:) %>
+<%= button_to "↑", url, method: :patch, params: { direction: "up" },
+      disabled: first, class: "btn btn-xs", aria: { label: "Move #{title} up" } %>
+<%= button_to "↓", url, method: :patch, params: { direction: "down" },
+      disabled: last, class: "btn btn-xs", aria: { label: "Move #{title} down" } %>

config/routes.rb

@@ -23,8 +23,11 @@
       post :csv_import, to: "csv#import"
     end
     resources :content_modules, except: [ :show ] do
+      resource :position, only: :update, module: :content_modules
       resources :links, shallow: true, except: %i[index show]
-      resources :contents, shallow: true, only: %i[create destroy]
+      resources :contents, shallow: true, only: %i[create destroy] do
+        resource :position, only: :update, module: :contents
+      end
     end
     resources :classroom_modules, only: %i[update]
   end

db/seeds.rb

@@ -19,7 +19,7 @@ def maybe(&block)
 ContentModule.levels.keys.each do |level|
   programs.each do |program|
     2.times do |i|
-      mod = program.content_modules.create!(name: "#{program.name} #{level.humanize} Module #{i + 1}", level: level, position: i + 1)
+      mod = program.content_modules.create!(name: "#{program.name} #{level.humanize} Module #{i + 1}", level: level)
       # TODO: Update this when the delegated types migration is completed
       link = Link.new(title: "Survey #{i + 1}", url: "https://example.com/survey/#{program.id}-#{level}-#{i}", link_type: "survey", position: 1, content_module: mod)
       mod.contents.create! contentable: link

test/controllers/admin/content_modules/positions_controller_test.rb

@@ -0,0 +1,57 @@
+require "test_helper"
+
+class Admin::ContentModules::PositionsControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    @program = programs(:one)
+    @first = create_module("First")
+    @second = create_module("Second")
+    sign_in_as users(:one)
+  end
+
+  test "moves module down" do
+    patch admin_content_module_position_url(@first), params: { direction: "down" }
+
+    assert_redirected_to admin_content_modules_url(program_id: @program.id)
+    assert_equal [ @second.id, @first.id ], ordered_ids
+  end
+
+  test "moves module up" do
+    patch admin_content_module_position_url(@second), params: { direction: "up" }
+
+    assert_redirected_to admin_content_modules_url(program_id: @program.id)
+    assert_equal [ @second.id, @first.id ], ordered_ids
+  end
+
+  test "moving the first module up changes nothing" do
+    patch admin_content_module_position_url(@first), params: { direction: "up" }
+
+    assert_redirected_to admin_content_modules_url(program_id: @program.id)
+    assert_equal [ @first.id, @second.id ], ordered_ids
+  end
+
+  test "rejects an unknown direction" do
+    patch admin_content_module_position_url(@first), params: { direction: "sideways" }
+
+    assert_response :unprocessable_entity
+    assert_equal [ @first.id, @second.id ], ordered_ids
+  end
+
+  test "requires admin sign in" do
+    sign_out
+
+    patch admin_content_module_position_url(@first), params: { direction: "down" }
+
+    assert_redirected_to new_admin_session_url
+    assert_equal [ @first.id, @second.id ], ordered_ids
+  end
+
+  private
+
+  def create_module(name)
+    ContentModule.create!(program: @program, level: "advanced", name: name)
+  end
+
+  def ordered_ids
+    @program.content_modules.advanced.ordered.map(&:id)
+  end
+end

test/controllers/admin/content_modules_controller_test.rb

@@ -25,7 +25,7 @@ class Admin::ContentModulesControllerTest < ActionDispatch::IntegrationTest
   test "should create content module" do
     assert_difference "ContentModule.count" do
       post admin_content_modules_url, params: {
-        content_module: { name: "New Module", program_id: programs(:one).id, level: "moderate", position: 1 }
+        content_module: { name: "New Module", program_id: programs(:one).id, level: "moderate" }
       }
     end
     assert_redirected_to admin_content_modules_url
@@ -56,6 +56,15 @@ class Admin::ContentModulesControllerTest < ActionDispatch::IntegrationTest
     assert_select "table a[target=_blank][rel='noopener noreferrer']", text: "Health Survey ↗"
   end
 
+  test "edit disables moving the first row up and the last row down" do
+    get edit_admin_content_module_url(@content_module)
+
+    assert_select "tbody tr:first-child button[aria-label$='up'][disabled]", count: 1
+    assert_select "tbody tr:first-child button[aria-label$='down'][disabled]", count: 0
+    assert_select "tbody tr:last-child button[aria-label$='down'][disabled]", count: 1
+    assert_select "tbody tr:last-child button[aria-label$='up'][disabled]", count: 0
+  end
+
   test "edit offers only games not yet in the module" do
     get edit_admin_content_module_url(@content_module)
 
@@ -77,6 +86,45 @@ class Admin::ContentModulesControllerTest < ActionDispatch::IntegrationTest
     assert_select "tbody tr", text: /#{@content_module.name}\s+#{@content_module.contents.count}/
   end
 
+  test "index disables moving the first module up and the last module down within a level" do
+    second = ContentModule.create!(program: programs(:one), level: "basic", name: "Second Basic")
+    ContentModule.create!(program: programs(:one), level: "moderate", name: "Only Moderate")
+
+    get admin_content_modules_url(program_id: programs(:one).id)
+
+    assert_select "section", text: /Basic/ do
+      assert_select "tbody tr:first-child button[aria-label$='up'][disabled]", count: 1
+      assert_select "tbody tr:first-child button[aria-label$='down'][disabled]", count: 0
+      assert_select "tbody tr:last-child button[aria-label='Move #{second.name} down'][disabled]", count: 1
+      assert_select "tbody tr:last-child button[aria-label$='up'][disabled]", count: 0
+    end
+    assert_select "section", text: /Moderate/ do
+      assert_select "tbody tr button[disabled]", count: 2
+    end
+  end
+
+  test "index move buttons patch the module position" do
+    get admin_content_modules_url(program_id: @content_module.program_id)
+
+    assert_select "form[action='#{admin_content_module_position_path(@content_module)}'] button[aria-label='Move #{@content_module.name} up']"
+  end
+
+  test "create appends the module and ignores a submitted position" do
+    ContentModule.create!(program: programs(:one), level: "moderate", name: "Existing")
+
+    post admin_content_modules_url, params: {
+      content_module: { name: "New Module", program_id: programs(:one).id, level: "moderate", position: 99 }
+    }
+
+    assert_equal 2, ContentModule.find_by!(name: "New Module").position
+  end
+
+  test "form has no position field" do
+    get new_admin_content_module_url
+
+    assert_select "input[name='content_module[position]']", count: 0
+  end
+
   test "should update content module" do
     patch admin_content_module_url(@content_module), params: {
       content_module: { name: "Updated Name" }

test/controllers/admin/contents/positions_controller_test.rb

@@ -0,0 +1,47 @@
+require "test_helper"
+
+class Admin::Contents::PositionsControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    @content_module = content_modules(:one)
+    @first = contents(:one)
+    @second = contents(:two)
+    sign_in_as users(:one)
+  end
+
+  test "moves content down" do
+    patch admin_content_position_url(@first), params: { direction: "down" }
+
+    assert_redirected_to edit_admin_content_module_url(@content_module)
+    assert_equal [ @second.id, @first.id ], @content_module.contents.reload.map(&:id)
+  end
+
+  test "moves content up" do
+    patch admin_content_position_url(@second), params: { direction: "up" }
+
+    assert_redirected_to edit_admin_content_module_url(@content_module)
+    assert_equal [ @second.id, @first.id ], @content_module.contents.reload.map(&:id)
+  end
+
+  test "moving the first content up changes nothing" do
+    patch admin_content_position_url(@first), params: { direction: "up" }
+
+    assert_redirected_to edit_admin_content_module_url(@content_module)
+    assert_equal [ @first.id, @second.id ], @content_module.contents.reload.map(&:id)
+  end
+
+  test "rejects an unknown direction" do
+    patch admin_content_position_url(@first), params: { direction: "sideways" }
+
+    assert_response :unprocessable_entity
+    assert_equal [ @first.id, @second.id ], @content_module.contents.reload.map(&:id)
+  end
+
+  test "requires admin sign in" do
+    sign_out
+
+    patch admin_content_position_url(@first), params: { direction: "down" }
+
+    assert_redirected_to new_admin_session_url
+    assert_equal [ @first.id, @second.id ], @content_module.contents.reload.map(&:id)
+  end
+end