Merge pull request #15581 from opf/fix/new-section-title

Use "New section" as placeholder, empty title as the hidden/builtin title

modules/meeting/app/components/meeting_sections/header_component.html.erb

@@ -9,7 +9,7 @@
         end
         grid.with_area(:content, tag: :span) do
           render(Primer::Beta::Text.new(font_weight: :bold, mr: 2)) do
-            @meeting_section.title
+            @meeting_section.title.presence || I18n.t("meeting_section.untitled_title")
           end
         end
         if @meeting_section.agenda_items_sum_duration_in_minutes > 0

modules/meeting/app/components/meeting_sections/show_component.html.erb

@@ -3,7 +3,7 @@
     render(Primer::Beta::BorderBox.new(mt: 3, data: drag_and_drop_target_config)) do |component|
       if render_section_wrapper?
         component.with_header(font_weight: :bold, pl: 0) do
-          render(MeetingSections::HeaderComponent.new(meeting_section: @meeting_section))
+          render(MeetingSections::HeaderComponent.new(meeting_section: @meeting_section, state: @state))
         end
       end
       if render_new_button_in_section?

modules/meeting/app/components/meeting_sections/show_component.rb

@@ -34,7 +34,8 @@ class ShowComponent < ApplicationComponent
 
     with_collection_parameter :meeting_section
 
-    def initialize(meeting_section:, first_and_last: [], form_hidden: true, form_type: :simple, insert_target_modified: true)
+    def initialize(meeting_section:, first_and_last: [], form_hidden: true, form_type: :simple, insert_target_modified: true,
+                   force_wrapper: false, state: :show)
       super
 
       @meeting = meeting_section.meeting
@@ -44,6 +45,8 @@ def initialize(meeting_section:, first_and_last: [], form_hidden: true, form_typ
       @form_hidden = form_hidden
       @form_type = form_type
       @insert_target_modified = insert_target_modified
+      @force_wrapper = force_wrapper
+      @state = state
     end
 
     private
@@ -65,8 +68,7 @@ def editable?
     end
 
     def render_section_wrapper?
-      # true
-      !@meeting_section.untitled? || @meeting.sections.count > 1
+      @force_wrapper || !@meeting_section.untitled? || @meeting.sections.count > 1
     end
 
     def render_new_button_in_section?

modules/meeting/app/contracts/meeting_sections/create_contract.rb

@@ -28,7 +28,15 @@
 
 module MeetingSections
   class CreateContract < BaseContract
-    validate :user_allowed_to_add, :validate_meeting_existence
+    # Note:
+    # the CreateContract is currently only called internally in the create action which substitutes the new action
+    # of the controller. The contract is not used in the context of a form.
+    # Only the UpdateContract is used alongside a user facing form.
+    # Thus we're not validating for title presence here, which enables us to create a section without a title which is required for
+    # the current UX implementation
+
+    validate :user_allowed_to_add,
+             :validate_meeting_existence
 
     def self.assignable_meetings(user)
       StructuredMeeting

modules/meeting/app/contracts/meeting_sections/move_contract.rb

@@ -0,0 +1,48 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2024 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+module MeetingSections
+  class MoveContract < BaseContract
+    validate :user_allowed_to_edit
+
+    # We allow an empty title internally via create to mark an untitled/implicit section
+    # Moving/Dragging them needs to be possible as well
+    # this we're not validating the title here in contrast to the UpdateContract
+    # validates :title, presence: true
+
+    # Meeting agenda items can currently be only edited
+    # through the project permission :manage_agendas
+    # When MeetingRole becomes available, agenda items will
+    # be edited through meeting permissions :manage_agendas
+    def user_allowed_to_edit
+      unless user.allowed_in_project?(:manage_agendas, model.project)
+        errors.add :base, :error_unauthorized
+      end
+    end
+  end
+end

modules/meeting/app/contracts/meeting_sections/update_contract.rb

@@ -30,6 +30,10 @@ module MeetingSections
   class UpdateContract < BaseContract
     validate :user_allowed_to_edit
 
+    # We allow an empty title internally via create to mark an untitled/implicit section
+    # but users should not be able to update it with an empty title through this contract
+    validates :title, presence: true
+
     # Meeting agenda items can currently be only edited
     # through the project permission :manage_agendas
     # When MeetingRole becomes available, agenda items will

modules/meeting/app/controllers/concerns/meetings/agenda_component_streams.rb

@@ -326,12 +326,15 @@ def update_section_header_via_turbo_stream(meeting_section: @meeting_section, st
         )
       end
 
-      def update_section_via_turbo_stream(meeting_section: @meeting_section, form_hidden: true, form_type: :simple)
+      def update_section_via_turbo_stream(meeting_section: @meeting_section, form_hidden: true, form_type: :simple,
+                                          force_wrapper: false, state: :show)
         update_via_turbo_stream(
           component: MeetingSections::ShowComponent.new(
             meeting_section:,
             form_type:,
-            form_hidden:
+            form_hidden:,
+            force_wrapper:,
+            state:
           )
         )
       end

modules/meeting/app/controllers/meeting_sections_controller.rb

@@ -42,16 +42,15 @@ def create
       .new(user: current_user)
       .call(
         {
-          meeting_id: @meeting.id,
-          title: t("meeting_section.default_title")
+          meeting_id: @meeting.id
         }
       )
 
     @meeting_section = call.result
 
     if call.success?
       add_section_via_turbo_stream
-      update_section_header_via_turbo_stream(state: :edit)
+      update_section_via_turbo_stream(force_wrapper: true, state: :edit)
       # update the section header of the previously last section in order to ensure the action menu move options are updated
       update_section_header_via_turbo_stream(meeting_section: @meeting.sections.last(2).first) if @meeting.sections.count > 1
       update_new_button_via_turbo_stream(disabled: true)
@@ -74,8 +73,8 @@ def edit
   end
 
   def cancel_edit
-    if @meeting_section.has_default_title? && @meeting_section.agenda_items.empty?
-      # if the section has the default title and no agenda items, we can safely delete it
+    if @meeting_section.untitled? && @meeting_section.agenda_items.empty?
+      # if the section is untitled and no agenda items, we can safely delete it
       destroy and return
     else
       update_section_header_via_turbo_stream(state: :show)
@@ -125,7 +124,7 @@ def destroy
 
   def drop
     call = ::MeetingSections::UpdateService
-      .new(user: current_user, model: @meeting_section)
+      .new(user: current_user, model: @meeting_section, contract_class: MeetingSections::MoveContract)
       .call(position: params[:position].to_i)
 
     if call.success?
@@ -145,7 +144,7 @@ def drop
 
   def move
     call = ::MeetingSections::UpdateService
-      .new(user: current_user, model: @meeting_section)
+      .new(user: current_user, model: @meeting_section, contract_class: MeetingSections::MoveContract)
       .call(move_to: params[:move_to]&.to_sym)
 
     if call.success?

modules/meeting/app/forms/meeting_section/title.rb

@@ -30,9 +30,8 @@ class MeetingSection::Title < ApplicationForm
   form do |meeting_section_form|
     meeting_section_form.text_field(
       name: :title,
-      placeholder: Meeting.human_attribute_name(:title),
+      placeholder: I18n.t("meeting_section.placeholder_title"),
       label: Meeting.human_attribute_name(:title),
-      value: init_value(meeting_section_form),
       visually_hide_label: true,
       required: true,
       autofocus: true,
@@ -42,14 +41,4 @@ class MeetingSection::Title < ApplicationForm
       }
     )
   end
-
-  private
-
-  def init_value(meeting_section_form)
-    if meeting_section_form.builder.object.has_default_title? || meeting_section_form.builder.object.untitled?
-      "" # will be set to "Untitled" in the model if left empty
-    else
-      meeting_section_form.builder.object.title
-    end
-  end
 end

modules/meeting/app/models/meeting_agenda_item.rb

@@ -78,7 +78,7 @@ def add_to_latest_meeting_section
       meeting_section = meeting.sections.order(position: :asc).last
 
       if meeting_section.nil?
-        meeting_section = meeting.sections.build(title: "Untitled")
+        meeting_section = meeting.sections.build(title: "")
       end
 
       self.meeting_section = meeting_section

modules/meeting/app/models/meeting_section.rb

@@ -34,10 +34,6 @@ class MeetingSection < ApplicationRecord
   has_many :agenda_items, dependent: :destroy, class_name: "MeetingAgendaItem"
   has_one :project, through: :meeting
 
-  validates :title, presence: true
-
-  before_validation :set_default_title
-
   after_save :trigger_meeting_agenda_item_time_slots_calculation, if: Proc.new { |section|
     section.position_previously_changed?
   }
@@ -50,18 +46,8 @@ def trigger_meeting_agenda_item_time_slots_calculation
     meeting.calculate_agenda_item_time_slots
   end
 
-  def set_default_title
-    if title.blank?
-      self.title = I18n.t("meeting_section.untitled_title")
-    end
-  end
-
-  def has_default_title?
-    title == I18n.t("meeting_section.default_title")
-  end
-
   def untitled?
-    title == I18n.t("meeting_section.untitled_title")
+    title.blank?
   end
 
   def editable?

modules/meeting/config/locales/en.yml

@@ -143,9 +143,9 @@ en:
     copied: "Copied from Meeting #%{id}"
 
   meeting_section:
+    untitled_title: "Untitled section"
     delete_confirmation: "Deleting the section will also delete all of its agenda items. Are you sure you want to do this?"
-    default_title: "New section"
-    untitled_title: "Untitled"
+    placeholder_title: "New section"
     empty_text: "Drag items here or create a new one"
 
   notice_successful_notification: "Notification sent successfully"

modules/meeting/db/migrate/20240516102219_fix_untitled_meetings.rb

@@ -0,0 +1,9 @@
+class FixUntitledMeetings < ActiveRecord::Migration[7.1]
+  def up
+    MeetingSection.where(title: "Untitled").update_all(title: "")
+  end
+
+  def down
+    MeetingSection.where(title: "").update_all(title: "Untitled")
+  end
+end

modules/meeting/spec/contracts/meeting_sections/create_contract_spec.rb

@@ -61,6 +61,16 @@
 
       it_behaves_like "contract is invalid", base: :error_unauthorized
     end
+
+    context "when :title is empty" do
+      before do
+        section.title = ""
+      end
+
+      # empty title allowed in create contract in contrast to update contract
+      # only used internally and not alongside a user facing form
+      it_behaves_like "contract is valid"
+    end
   end
 
   context "without permission" do

modules/meeting/spec/features/structured_meetings/structured_meeting_crud_spec.rb

@@ -330,6 +330,8 @@
 
         first_section = MeetingSection.find_by!(title: "First section")
 
+        meeting = first_section.meeting
+
         # edit the first section
         show_page.edit_section(first_section) do
           fill_in "Title", with: "Updated first section title"
@@ -357,31 +359,58 @@
         show_page.expect_section(title: "Updated first section title")
         show_page.expect_no_section(title: "Second section")
 
-        # add a section without a name
+        # add a section without a name is not possible
         show_page.add_section do
           click_on "Save"
+          expect(page).to have_text "Title can't be blank"
         end
 
-        ## "Untitled" is applied automatically as a name
-        show_page.expect_section(title: "Updated first section title")
-        show_page.expect_section(title: "Untitled")
-
         # remove the first section
         show_page.remove_section first_section
+        show_page.expect_no_section(title: "Updated first section title")
+
+        # now the meeting completely empty again
+
+        # add an item to the meeting
+        show_page.add_agenda_item do
+          fill_in "Title", with: "First item without explicit section"
+        end
+
+        # the agenda item is wrapped in an "untitled" section, but the section is not explicitly rendered
+        show_page.expect_no_section(title: "Untitled section")
+
+        # add a second section again
+        show_page.add_section do
+          fill_in "Title", with: "Second section"
+          click_on "Save"
+        end
+
+        ## the first section without a name is now explicitly rendered as "Untitled"
+        show_page.expect_section(title: "Untitled section")
+        show_page.expect_section(title: "Second section")
+
+        second_section = MeetingSection.find_by!(title: "Second section")
+
+        # remove the second section
+        show_page.remove_section second_section
 
         ## the last existing section is not explicitly rendered as a section as no name was specified for this section
         ## -> back to "no section mode"
-        show_page.expect_no_section(title: "Updated first section title")
-        show_page.expect_no_section(title: "Untitled")
+        show_page.expect_no_section(title: "Second section")
+        show_page.expect_no_section(title: "Untitled section")
+
+        # TBD: remove the agenda item again, the untitle section is not rendered explicitly and will not be removed
+        first_item = MeetingAgendaItem.find_by!(title: "First item without explicit section")
+        show_page.remove_agenda_item(first_item)
 
         # add a second section again
         show_page.add_section do
           fill_in "Title", with: "Second section"
           click_on "Save"
         end
 
-        ## the first section without a name is now explicitly rendered as "Untitled"
-        show_page.expect_section(title: "Untitled")
+        ## the first section without a name is now again explicitly rendered as "Untitled"
+        show_page.expect_section(title: "Untitled section")
         show_page.expect_section(title: "Second section")
 
         second_section = MeetingSection.find_by!(title: "Second section")
@@ -394,7 +423,7 @@
 
         show_page.expect_agenda_item_in_section title: "First item", section: second_section
 
-        first_section = MeetingSection.find_by!(title: "Untitled")
+        first_section = meeting.sections.first
 
         # add an item to the first section explicitly
         show_page.add_agenda_item_to_section(section: first_section) do
@@ -425,7 +454,7 @@
         end
 
         # only untitled secion is left -> will not be rendered explicitly as secion
-        show_page.expect_no_section(title: "Untitled")
+        show_page.expect_no_section(title: "Untitled section")
         show_page.expect_no_section(title: "Second section")
 
         expect { item_in_second_section.reload }.to raise_error(ActiveRecord::RecordNotFound)