From 8da27764dbdba34332dc39cd378ad51a5aac672b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Thu, 22 Feb 2024 21:42:23 +0100 Subject: [PATCH 1/2] Copy meeting agenda items --- .../app/controllers/meetings_controller.rb | 46 ++++++--- .../meeting/app/models/meeting_agenda_item.rb | 4 + .../meeting/app/models/structured_meeting.rb | 7 +- .../meeting/app/views/meetings/_form.html.erb | 22 ++++- .../meeting/app/views/meetings/new.html.erb | 12 ++- modules/meeting/config/locales/en.yml | 4 + .../spec/features/meetings_copy_spec.rb | 2 - .../structured_meeting_crud_spec.rb | 4 +- .../meeting/spec/requests/meetings_spec.rb | 99 +++++++++++++++++++ 9 files changed, 174 insertions(+), 26 deletions(-) create mode 100644 modules/meeting/spec/requests/meetings_spec.rb diff --git a/modules/meeting/app/controllers/meetings_controller.rb b/modules/meeting/app/controllers/meetings_controller.rb index f01cac053956..50e605ef11a8 100644 --- a/modules/meeting/app/controllers/meetings_controller.rb +++ b/modules/meeting/app/controllers/meetings_controller.rb @@ -31,6 +31,7 @@ class MeetingsController < ApplicationController before_action :find_optional_project, only: %i[index new create] before_action :build_meeting, only: %i[new create] before_action :find_meeting, except: %i[index new create] + before_action :find_copy_from_meeting, only: %i[create] before_action :convert_params, only: %i[create update update_participants] before_action :authorize, except: %i[index new create update_title update_details update_participants change_state] before_action :authorize_global, only: %i[index new create update_title update_details update_participants change_state] @@ -73,13 +74,8 @@ def create @meeting.participants.clear # Start with a clean set of participants @meeting.participants_attributes = @converted_params.delete(:participants_attributes) @meeting.attributes = @converted_params - if params[:copied_from_meeting_id].present? && params[:copied_meeting_agenda_text].present? - @meeting.agenda = MeetingAgenda.new( - text: params[:copied_meeting_agenda_text], - journal_notes: I18n.t('meeting.copied', id: params[:copied_from_meeting_id]) - ) - @meeting.agenda.author = User.current - end + copy_meeting_agenda + if @meeting.save text = I18n.t(:notice_successful_create) if User.current.time_zone.nil? @@ -90,7 +86,7 @@ def create redirect_to action: 'show', id: @meeting else - render template: 'meetings/new', project_id: @project + render template: 'meetings/new', project_id: @project, locals: { copy_from: @copy_from } end end @@ -101,10 +97,9 @@ def new; end end def copy - params[:copied_from_meeting_id] = @meeting.id - params[:copied_meeting_agenda_text] = @meeting.agenda.text if @meeting.agenda.present? + copy_from = @meeting @meeting = @meeting.copy(author: User.current) - render action: 'new', project_id: @project, locals: { copy: true } + render action: 'new', project_id: @project, locals: { copy_from: } end def destroy @@ -274,7 +269,8 @@ def set_time_zone(&) end def build_meeting - @meeting = Meeting.new + cls = meeting_type(params.dig(:meeting, :type)).constantize + @meeting = cls.new @meeting.project = @project @meeting.author = User.current end @@ -299,7 +295,6 @@ def convert_params # instance variable. @converted_params = meeting_params.to_h - @converted_params[:type] = meeting_type(@converted_params[:type]) @converted_params[:duration] = @converted_params[:duration].to_hours if @converted_params[:duration].present? # Force defaults on participants @converted_params[:participants_attributes] ||= {} @@ -308,7 +303,7 @@ def convert_params def meeting_params if params[:meeting].present? - params.require(:meeting).permit(:title, :location, :start_time, :type, + params.require(:meeting).permit(:title, :location, :start_time, :duration, :start_date, :start_time_hour, participants_attributes: %i[email name invited attended user user_id meeting id]) end @@ -330,4 +325,27 @@ def meeting_type(given_type) 'Meeting' end end + + def find_copy_from_meeting + return unless params[:copied_from_meeting_id] + + @copy_from = Meeting.visible.find(params[:copied_from_meeting_id]) + rescue ActiveRecord::RecordNotFound + render_404 + end + + def copy_meeting_agenda + return unless params[:copy_agenda] == '1' && @copy_from + + if @meeting.is_a?(StructuredMeeting) + @meeting.agenda_items_attributes = @copy_from.agenda_items.map(&:copy_attributes) + else + @meeting.agenda = MeetingAgenda.new( + author: current_user, + text: @copy_from.agenda&.text, + journal_notes: I18n.t('meeting.copied', id: params[:copied_from_meeting_id]) + ) + @meeting.agenda.author = current_user + end + end end diff --git a/modules/meeting/app/models/meeting_agenda_item.rb b/modules/meeting/app/models/meeting_agenda_item.rb index c0811dfc67b1..a3558eb8157b 100644 --- a/modules/meeting/app/models/meeting_agenda_item.rb +++ b/modules/meeting/app/models/meeting_agenda_item.rb @@ -85,4 +85,8 @@ def editable? def modifiable? !(meeting&.closed? || (deleted_work_package? && work_package_id.present?)) end + + def copy_attributes + attributes.except('id', 'meeting_id') + end end diff --git a/modules/meeting/app/models/structured_meeting.rb b/modules/meeting/app/models/structured_meeting.rb index d1fae09521ef..1d7dc58c49d7 100644 --- a/modules/meeting/app/models/structured_meeting.rb +++ b/modules/meeting/app/models/structured_meeting.rb @@ -27,7 +27,12 @@ #++ class StructuredMeeting < Meeting - has_many :agenda_items, dependent: :destroy, foreign_key: 'meeting_id', class_name: 'MeetingAgendaItem' + has_many :agenda_items, + dependent: :destroy, + foreign_key: 'meeting_id', + class_name: 'MeetingAgendaItem', + inverse_of: :meeting + accepts_nested_attributes_for :agenda_items # triggered by MeetingAgendaItem#after_create/after_destroy/after_save def calculate_agenda_item_time_slots diff --git a/modules/meeting/app/views/meetings/_form.html.erb b/modules/meeting/app/views/meetings/_form.html.erb index 2f93017842e4..c6898c1ada3a 100644 --- a/modules/meeting/app/views/meetings/_form.html.erb +++ b/modules/meeting/app/views/meetings/_form.html.erb @@ -35,7 +35,24 @@ See COPYRIGHT and LICENSE files for more details. <%= f.text_field :title, :required => true, :size => 60, container_class: '-wide' %> - <% if @meeting.new_record? %> + <% copy_from = local_assigns[:copy_from] %> + <% if copy_from.present? %> + <%= f.hidden_field :type, value: copy_from.is_a?(StructuredMeeting) ? 'dynamic' : 'classic' %> + <%= hidden_field_tag "copied_from_meeting_id", copy_from.id %> +
+ <%= styled_label_tag 'copy_agenda', t('meeting.copy.agenda') %> +
+ <%= styled_check_box_tag 'copy_agenda', + 1, + true, + no_label: true, + class: 'radio-button' %> +
+
+ <%= t('meeting.copy.agenda_text') %> +
+
+ <% elsif @meeting.new_record? %>
@@ -161,7 +178,4 @@ See COPYRIGHT and LICENSE files for more details.
<%= render partial: 'meetings/participants_section' %> - - <%= hidden_field_tag "copied_from_meeting_id", params[:copied_from_meeting_id] if params[:copied_from_meeting_id].present? %> - <%= hidden_field_tag "copied_meeting_agenda_text", params[:copied_meeting_agenda_text] if params[:copied_meeting_agenda_text].present? %> diff --git a/modules/meeting/app/views/meetings/new.html.erb b/modules/meeting/app/views/meetings/new.html.erb index 4c345bdc39d3..2c498f663402 100644 --- a/modules/meeting/app/views/meetings/new.html.erb +++ b/modules/meeting/app/views/meetings/new.html.erb @@ -26,9 +26,15 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. See COPYRIGHT and LICENSE files for more details. ++#%> +<% copy_from = local_assigns[:copy_from] %> +<% if copy_from %> + <% html_title t('meeting.copy.title', title: copy_from.title) %> + <%= toolbar title: t('meeting.copy.title', title: copy_from.title) %> +<% else %> + <% html_title t(:label_meeting_new) %> + <%= toolbar title: t(:label_meeting_new) %> +<% end %> -<% html_title t(:label_meeting_new) %> -<%= toolbar title: t(:label_meeting_new) %> <%= labelled_tabular_form_for @meeting, as: :meeting, url: {:controller => '/meetings', :action => 'create', :project_id => @project}, @@ -36,7 +42,7 @@ See COPYRIGHT and LICENSE files for more details. :data => { :controller => 'refresh-on-form-changes', 'refresh-on-form-changes-target': 'form', 'refresh-on-form-changes-turbo-stream-url-value': new_meeting_url }} do |f| -%> - <%= render :partial => 'form', :locals => { f:, copy: local_assigns[:copy] } %> + <%= render :partial => 'form', :locals => { f:, copy_from: } %> <%= styled_button_tag t(:button_create), class: '-highlight' %> <%= link_to t(:button_cancel), { :action => 'index', :project_id => @project }, class: 'button' %> diff --git a/modules/meeting/config/locales/en.yml b/modules/meeting/config/locales/en.yml index 58a3532f47af..ec4288efb3fd 100644 --- a/modules/meeting/config/locales/en.yml +++ b/modules/meeting/config/locales/en.yml @@ -113,6 +113,10 @@ en: meeting: + copy: + title: "Copy meeting %{title}" + agenda: "Copy agenda" + agenda_text: "Copy the agenda of the old meeting" email: open_meeting_link: "Open meeting" invited: diff --git a/modules/meeting/spec/features/meetings_copy_spec.rb b/modules/meeting/spec/features/meetings_copy_spec.rb index 48de9243edfa..0aac54cf3949 100644 --- a/modules/meeting/spec/features/meetings_copy_spec.rb +++ b/modules/meeting/spec/features/meetings_copy_spec.rb @@ -94,8 +94,6 @@ expect(page) .to have_field 'Time', with: start_time.strftime("%H:%M") - choose 'Classic' - click_button "Create" # Be on the new meeting's page with copied over attributes diff --git a/modules/meeting/spec/features/structured_meetings/structured_meeting_crud_spec.rb b/modules/meeting/spec/features/structured_meetings/structured_meeting_crud_spec.rb index 000556b07c42..fb33994db521 100644 --- a/modules/meeting/spec/features/structured_meetings/structured_meeting_crud_spec.rb +++ b/modules/meeting/spec/features/structured_meetings/structured_meeting_crud_spec.rb @@ -251,7 +251,7 @@ expect(page).to have_css('.flash', text: I18n.t('activerecord.errors.messages.error_conflict')) end - it 'can copy the meeting (empty)' do + it 'can copy the meeting' do show_page.expect_toast(message: 'Successful creation') # Can add and edit a single item @@ -270,7 +270,7 @@ click_button 'Create' - expect(page).to have_text 'Your meeting is empty' + show_page.expect_agenda_item title: 'My agenda item' new_meeting = StructuredMeeting.reorder(id: :asc).last expect(page).to have_current_path "/meetings/#{new_meeting.id}" end diff --git a/modules/meeting/spec/requests/meetings_spec.rb b/modules/meeting/spec/requests/meetings_spec.rb new file mode 100644 index 000000000000..1623a789ca01 --- /dev/null +++ b/modules/meeting/spec/requests/meetings_spec.rb @@ -0,0 +1,99 @@ +#-- 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. +#++ + +require 'spec_helper' + +RSpec.describe 'Meeting requests', + :skip_csrf, + type: :rails_request do + + shared_let(:project) { create(:project, enabled_module_names: %i[meetings]) } + shared_let(:user) { create(:user, member_with_permissions: { project => %i[view_meetings create_meetings] }) } + + before do + login_as user + end + + describe 'copy' do + let(:meeting) { create(:structured_meeting, project:) } + let(:base_params) do + { + copied_from_meeting_id: meeting.id, + project_id: project.id, + meeting: { + title: 'Copied meeting', + type: :dynamic + } + } + end + let(:params) { {} } + + subject do + post meetings_path(project), + params: base_params.merge(params) + + Meeting.find_by(title: 'Copied meeting') + end + + context 'when copying agenda items' do + let!(:agenda_item) { create(:meeting_agenda_item, meeting:, notes: '**foo**') } + let(:params) { { copy_agenda: '1' } } + + it 'copies the agenda items' do + subject + + expect(response).to be_redirect + + expect(subject).to be_present + expect(subject.agenda_items.count).to eq(1) + expect(subject.agenda_items.first.notes).to eq('**foo**') + end + end + + context 'when copying without additional params' do + it 'copies the meeting, but not the agenda' do + subject + + expect(response).to be_redirect + + expect(subject).to be_present + expect(subject.agenda_items).to be_empty + end + end + + context 'when meeting is not visible' do + let(:other_project) { create(:project) } + let(:meeting) { create(:meeting, project: other_project) } + + it 'renders a 404' do + subject + expect(response).to have_http_status(:not_found) + end + end + end +end From b8d14bf2bca9f0fae98c9035060fe098f3469686 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 26 Feb 2024 16:37:05 +0100 Subject: [PATCH 2/2] Skip _id validation if new_record meeting was passed --- modules/meeting/app/models/meeting_agenda_item.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/meeting/app/models/meeting_agenda_item.rb b/modules/meeting/app/models/meeting_agenda_item.rb index a3558eb8157b..1c0b4c519bdb 100644 --- a/modules/meeting/app/models/meeting_agenda_item.rb +++ b/modules/meeting/app/models/meeting_agenda_item.rb @@ -45,7 +45,11 @@ class MeetingAgendaItem < ApplicationRecord scope :with_includes_to_render, -> { includes(:author, :meeting) } - validates :meeting_id, presence: true + # The primer form depends on meeting_id being validated, even though Rails pattern would suggest + # to validate only :meeting. When copying meetings however, + # we build meetings and agenda items together, so meeting_id will stay empty. + # We can use loaded? to check if the meeting has been provided + validates :meeting_id, presence: true, unless: Proc.new { |item| item.association(:meeting).loaded? && item.meeting } validates :title, presence: true, if: Proc.new { |item| item.simple? } validates :work_package_id, presence: true, if: Proc.new { |item| item.work_package? }, on: :create validates :work_package_id,