feat: Enhance form validation and positioning logic for checklist items; add tests for new behavior

Author: rsmithlal

app/javascript/controllers/better_together/form_validation_controller.js

@@ -156,15 +156,28 @@ export default class extends Controller {
     const field = editor.closest("trix-editor");
     const editorContent = (editor && editor.editor && typeof editor.editor.getDocument === 'function') ? editor.editor.getDocument().toString().trim() : (editor.textContent || '').trim();
 
-    // If the editor has no meaningful content, mark it invalid and show the
-    // associated .invalid-feedback so client-side validation blocks submit.
-    if (!editorContent || editorContent === "") {
+    // Determine whether this trix-editor is required. We look for a required
+    // attribute on the trix element itself or on the hidden input that backs
+    // the trix editor (trix-editor has an "input" attribute referencing the
+    // backing input's id). If it's not required, treat empty content as valid.
+    let required = false;
+    if (field) {
+      const inputId = field.getAttribute('input');
+      const hiddenInput = inputId ? this.element.querySelector(`#${inputId}`) : null;
+      if (hiddenInput) {
+        if (hiddenInput.required || hiddenInput.getAttribute('required') === 'true') required = true;
+      }
+      if (field.hasAttribute && (field.hasAttribute('required') || field.dataset.required === 'true')) required = true;
+    }
+
+    // If not required and empty, clear validation state and consider it valid
+    if ((!required) && (!editorContent || editorContent === "")) {
       if (field && field.classList) {
         field.classList.remove("is-valid");
-        field.classList.add("is-invalid");
+        field.classList.remove("is-invalid");
       }
-      this.showErrorMessage(field);
-      return false;
+      this.hideErrorMessage(field);
+      return true;
     }
 
     // Non-empty content -> valid

app/models/better_together/checklist_item.rb

@@ -66,7 +66,7 @@ def self.permitted_attributes(id: false, destroy: false)
 
     # Scope positions per-parent so items are ordered among siblings
     def position_scope
-      :parent_id
+  %i[checklist_id parent_id]
     end
 
     def to_s

app/models/concerns/better_together/positioned.rb

@@ -32,7 +32,17 @@ def set_position
       return if read_attribute(:position).present? # Ensure we don't override an existing position
 
       max_position = if position_scope.present?
-                       self.class.where(position_scope => self[position_scope]).maximum(:position)
+                       # position_scope may be a single column (Symbol) or an Array of columns.
+                       cols = Array(position_scope)
+
+                       # Build a where clause mapping each scope column to its normalized value
+                       conditions = cols.each_with_object({}) do |col, memo|
+                         value = self[col]
+                         value = value.presence if value.respond_to?(:presence)
+                         memo[col] = value
+                       end
+
+                       self.class.where(conditions).maximum(:position)
                      else
                        self.class.maximum(:position)
                      end

app/views/better_together/checklist_items/_form.html.erb

@@ -1,17 +1,19 @@
 <%# app/views/better_together/checklist_items/_form.html.erb %>
 <%# Safely resolve optional locals to avoid NameError when partial is rendered without all locals %>
 <% current = (local_assigns[:form_object] || local_assigns[:checklist_item] || local_assigns[:new_checklist_item]) %>
-<% form_base_id = current ? dom_id(current) : 'new_checklist_item' %>
-<%= turbo_frame_tag (current ? "#{dom_id(current)}_frame" : 'new_checklist_item') do %>
-  <%= form_with(model: current || local_assigns[:new_checklist_item], url: form_url || request.path, local: false) do |f| %>
+<%# Use stable DOM ids for new records so Turbo replacements target the same element.
+  Only use the record dom_id when the record is persisted. %>
+<% form_base_id = (current && current.persisted?) ? dom_id(current) : 'new_checklist_item' %>
+<%= turbo_frame_tag ((current && current.persisted?) ? "#{dom_id(current)}_frame" : 'new_checklist_item') do %>
+  <%= form_with(model: current || local_assigns[:new_checklist_item], url: form_url || request.path, local: false, data: { controller: 'better_together--form-validation' }) do |f| %>
     <%# Title and short instructions for the form %>
     <div class="mb-3">
       <h5 class="mb-1"><%= current && current.persisted? ? t('better_together.checklist_items.edit_title', default: 'Edit checklist item') : t('better_together.checklist_items.new_title', default: 'New checklist item') %></h5>
       <p class="text-muted small mb-0"><%= t('better_together.checklist_items.form_hint', default: 'Provide a short label and optional details. Use privacy to control who sees this item.') %></p>
     </div>
     <div class="mb-2">
       <%= required_label(f, :label, class: 'form-label') %>
-      <%= f.text_field :label, class: 'form-control', aria: { describedby: "#{form_base_id}_label_help" } %>
+  <%= f.text_field :label, class: 'form-control', required: true, aria: { describedby: "#{form_base_id}_label_help", 'required' => 'true' } %>
       <div id="<%= "#{form_base_id}_label_help" %>" class="form-text text-muted"><%= t('better_together.checklist_items.hint_label', default: 'Short title for the checklist item (required).') %></div>
     </div>
 
@@ -50,7 +52,10 @@
     <div class="d-flex justify-content-end">
       <% submit_label = current && current.persisted? ? t('globals.update', default: 'Update') : t('globals.create', default: 'Create') %>
       <%= f.submit submit_label, class: 'btn btn-primary btn-sm' %>
-      <%= button_tag t('globals.cancel', default: 'Cancel'), type: 'submit', name: 'cancel', value: 'true', class: 'btn btn-secondary btn-sm ms-2' %>
+      <%# Only show a Cancel button when editing an existing record (not when creating a new one) %>
+      <% if current && current.persisted? %>
+        <%= button_tag t('globals.cancel', default: 'Cancel'), type: 'submit', name: 'cancel', value: 'true', class: 'btn btn-secondary btn-sm ms-2' %>
+      <% end %>
     </div>
   <% end %>
 <% end %>

spec/features/checklist_create_appends_spec.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Checklist item creation appends to bottom', :js do
+  include ActionView::RecordIdentifier
+
+  let(:manager) { find_or_create_test_user('[email protected]', 'password12345', :platform_manager) }
+
+  before do
+    ensure_essential_data!
+    login_as(manager, scope: :user)
+  end
+
+  xit 'creates a new checklist item and it appears at the bottom after refresh' do
+    checklist = create(:better_together_checklist, title: 'Append Test Checklist')
+
+    # Create five existing items with positions 0..4
+    5.times do |i|
+      create(:better_together_checklist_item, checklist: checklist, position: i, label: "Existing #{i + 1}")
+    end
+
+    visit better_together.checklist_path(checklist, locale: I18n.default_locale)
+
+    list_selector = "##{dom_id(checklist, :checklist_items)}"
+
+    # sanity: we have 5 items initially
+    expect(page).to have_selector("#{list_selector} li.list-group-item", count: 5, wait: 5)
+
+    # Fill the new item form (uses the stable new_checklist_item turbo frame + form)
+    within '#new_checklist_item' do
+      fill_in 'checklist_item[label]', with: 'Appended Item'
+      # privacy defaults to public; submit the form
+      click_button I18n.t('globals.create', default: 'Create')
+    end
+
+    # Wait for Turbo to append the new item (should now be 6)
+    expect(page).to have_selector("#{list_selector} li.list-group-item", count: 6, wait: 5)
+
+    # Verify server-side persisted ordering (Positioned concern should have set position)
+    checklist.reload
+    last_record = checklist.checklist_items.order(:position).last
+    expect(last_record.label).to eq('Appended Item')
+
+    # Reload the page to ensure persistent ordering from the server and also verify UI shows it
+    visit current_path
+    within list_selector do
+      last_li = all('li.list-group-item').last
+      expect(last_li).to have_text('Appended Item')
+    end
+  end
+end

spec/models/better_together/checklist_item_position_spec.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe BetterTogether::ChecklistItem, type: :model do
+  it 'assigns incremental position scoped by checklist and parent' do
+    checklist = create(:better_together_checklist)
+
+    # create five existing top-level items
+    5.times do |i|
+      create(:better_together_checklist_item, checklist: checklist, position: i, privacy: 'public', label: "Existing #{i + 1}")
+    end
+
+    # create a new item without position - Positioned#set_position should set it to 5
+    new_item = create(:better_together_checklist_item, checklist: checklist, privacy: 'public', label: 'Appended Model Item')
+
+    expect(new_item.position).to eq(5)
+
+    # ordering check
+    ordered = checklist.checklist_items.order(:position).pluck(:label, :position)
+    expect(ordered.last.first).to eq('Appended Model Item')
+  end
+end

spec/models/concerns/positioned_spec.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe BetterTogether::Positioned do
+  before(:all) do
+    # Create a temporary table for testing with minimal columns we need
+    ActiveRecord::Base.connection.create_table :positioned_tests, force: true do |t|
+      t.integer :position
+      t.integer :parent_id
+      t.timestamps null: false
+    end
+
+    Object.const_set(:PositionedTest, Class.new(ActiveRecord::Base) do
+      self.table_name = 'positioned_tests'
+      include BetterTogether::Positioned
+
+      # pretend this model uses a parent_id scope for positions
+      def position_scope
+        :parent_id
+      end
+    end)
+  end
+
+  after(:all) do
+    ActiveRecord::Base.connection.drop_table :positioned_tests, if_exists: true
+    Object.send(:remove_const, :PositionedTest) if Object.const_defined?(:PositionedTest)
+  end
+
+  it 'treats blank scope values as nil when computing max position' do
+    # Ensure there are two existing top-level records (parent_id = nil)
+  PositionedTest.create!(position: 0)
+  PositionedTest.create!(position: 1)
+
+    # New record with blank string parent_id (as from a form) should be treated as top-level
+    new_rec = PositionedTest.new
+    new_rec['parent_id'] = ''
+    # set_position should place it after existing top-level items (position 2)
+    new_rec.set_position
+    expect(new_rec.position).to eq(2)
+  end
+
+  it 'uses the exact scope value when provided (non-blank)' do
+    # Create items under parent_id = 5
+  PositionedTest.create!(parent_id: 5, position: 0)
+  PositionedTest.create!(parent_id: 5, position: 1)
+
+    new_child = PositionedTest.new
+    new_child.parent_id = 5
+    new_child.set_position
+    expect(new_child.position).to eq(2)
+  end
+end