feat: give contents ownership of module membership and order (#164)

* feat: give contents ownership of module membership and order

Links and games join modules only through contents, which append to the end of their siblings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix: updates

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: sean-dickinson

app/controllers/admin/content_modules_controller.rb

@@ -49,6 +49,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/games_controller.rb

@@ -6,8 +6,7 @@ def index
   end
 
   def new
-    @game = Game.new(content_module_id: params[:content_module_id])
-    @content_module = @game.content_module
+    @game = Game.new
   end
 
   def edit
@@ -18,7 +17,6 @@ def create
 
     respond_to do |format|
       if @game.save
-        Content.create!(content_module: @game.content_module, contentable: @game) if @game.content_module
         format.html { redirect_back(fallback_location: admin_games_path, notice: "Game was successfully created.") }
       else
         format.html { render :new, status: :unprocessable_content }
@@ -53,6 +51,6 @@ def set_game
     end
 
     def game_params
-      params.expect(game: [ :title, :slug, :content_module_id, :description ])
+      params.expect(game: [ :title, :slug, :description ])
     end
 end

app/controllers/admin/links_controller.rb

@@ -3,14 +3,15 @@ class Admin::LinksController < AdminController
   before_action :set_link, only: %i[edit update destroy]
 
   def new
-    @link = Link.new(content_module: @content_module)
+    @link = Link.new
   end
 
   def create
-    @link = Link.new(link_params.merge(content_module: @content_module))
+    @link = Link.new(link_params)
+
+    @link.build_content(content_module: @content_module)
 
     if @link.save
-      @content_module.contents.create!(contentable: @link)
       redirect_to edit_admin_content_module_path(@content_module), notice: "Link was successfully created."
     else
       render :new, status: :unprocessable_entity
@@ -29,8 +30,9 @@ def update
   end
 
   def destroy
+    content_module = @link.content_module
     @link.destroy!
-    redirect_to edit_admin_content_module_path(@link.content_module), notice: "Link was successfully deleted.", status: :see_other
+    redirect_to edit_admin_content_module_path(content_module), notice: "Link was successfully deleted.", status: :see_other
   end
 
   private
@@ -43,6 +45,6 @@ def set_link
     end
 
     def link_params
-      params.expect(link: [ :title, :url, :link_type, :position ])
+      params.expect(link: [ :title, :url, :link_type ])
     end
 end

app/models/concerns/contentable.rb

@@ -1,7 +1,10 @@
 module Contentable
   extend ActiveSupport::Concern
 
-  included do
-    has_one :content, as: :contentable
+  # Whether the content record owns this contentable, so the contentable is destroyed together with it.
+  #
+  # @return [Boolean]
+  def owned_by_content?
+    false
   end
 end

app/models/concerns/ordered.rb

@@ -1,7 +1,24 @@
+# Orders records by an integer +position+ column. New records go to the end of their siblings.
 module Ordered
   extend ActiveSupport::Concern
 
   included do
     scope :ordered, -> { order(:position) }
+
+    before_create :append_to_siblings
+  end
+
+  private
+
+  # This method is required to override. It is the relation of records this record is
+  # ordered among (for example, the contents of one module).
+  #
+  # @return [ActiveRecord::Relation]
+  def position_siblings
+    raise NotImplementedError, "#{self.class} must define #position_siblings to be ordered"
+  end
+
+  def append_to_siblings
+    self.position = (position_siblings.maximum(:position) || 0) + 1
   end
 end

app/models/content.rb

@@ -1,15 +1,26 @@
 class Content < ApplicationRecord
+  include Ordered
+
   belongs_to :content_module
 
   delegated_type :contentable, types: %w[Link Game]
 
+  validates :contentable_id, uniqueness: {
+    scope: [ :content_module_id, :contentable_type ],
+    message: "is already in this module"
+  }
+
   # 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
 
   private
 
+  def position_siblings
+    content_module.contents
+  end
+
   def destroy_owned_record
-    contentable.destroy if contentable.destroy_with_attachable?
+    contentable.destroy if contentable.owned_by_content?
   end
 end

app/models/content_module.rb

@@ -2,18 +2,23 @@ class ContentModule < ApplicationRecord
   include Ordered
 
   belongs_to :program
-  has_many :contents, dependent: :destroy
-  has_many :links, -> { ordered },
-    through: :contents,
-    source: :contentable,
-    source_type: "Link"
-  has_many :games,
-    through: :contents,
-    source: :contentable,
-    source_type: "Game"
+  has_many :contents, -> { ordered }, dependent: :destroy
+  has_many :games, through: :contents, source: :contentable, source_type: "Game"
+  has_many :links, through: :contents, source: :contentable, source_type: "Link"
   has_many :classroom_modules, dependent: :restrict_with_error
 
   enum :level, { basic: "basic", moderate: "moderate", advanced: "advanced" }, validate: true
 
   validates :name, :level, presence: true
+
+  # A module that joins another program or level goes to the end of it.
+  # TODO: Consider if we just don't allow these types of edits to avoid this hidden callback behavior
+  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
+    program.content_modules.where(level: level)
+  end
 end

app/models/game.rb

@@ -1,15 +1,12 @@
 class Game < ApplicationRecord
   include Contentable
 
-  belongs_to :content_module, optional: true
-  has_many :game_attempts, dependent: :destroy
-
+  has_many :contents, as: :contentable, dependent: :destroy
+  has_many :content_modules, through: :contents
   has_many :game_attempts, dependent: :destroy
 
   validates :title, :slug, presence: true
   validates :title, :slug, uniqueness: true
 
-  def destroy_with_attachable?
-    false
-  end
+  scope :available_for, ->(content_module) { where.not(id: content_module.games).order(:title) }
 end

app/models/link.rb

@@ -1,15 +1,16 @@
 class Link < ApplicationRecord
-  include Ordered
   include Contentable
 
-  belongs_to :content_module
+  # :delete skips Content callbacks to avoid a destroy loop
+  has_one :content, as: :contentable, dependent: :delete
+  has_one :content_module, through: :content
 
   enum :link_type, { survey: "survey" }, validate: true
 
   validates :title, :url, :link_type, presence: true
   validates :url, format: { with: /\Ahttps?:\/\/.+\z/i, message: "must start with http:// or https://" }, allow_blank: true
 
-  def destroy_with_attachable?
+  def owned_by_content?
     true
   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 %>