From ade749b373b1ea4d010f4be2f648b03fd37efae1 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Wed, 4 Jun 2025 16:50:51 +0200 Subject: [PATCH 001/122] My Time Tracking pages --- .../my/time_tracking/list_component.html.erb | 2 +- .../spec/features/my_time_tracking_spec.rb | 39 +++++++++++++-- .../pages/my_time_tracking/calendar_page.rb | 38 +++++++++++++++ .../pages/my_time_tracking/list_page.rb | 47 +++++++++++++++++++ 4 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 modules/costs/spec/support/pages/my_time_tracking/calendar_page.rb create mode 100644 modules/costs/spec/support/pages/my_time_tracking/list_page.rb diff --git a/modules/costs/app/components/my/time_tracking/list_component.html.erb b/modules/costs/app/components/my/time_tracking/list_component.html.erb index 06dfae607b9..96f15cc39c3 100644 --- a/modules/costs/app/components/my/time_tracking/list_component.html.erb +++ b/modules/costs/app/components/my/time_tracking/list_component.html.erb @@ -7,7 +7,7 @@ <%= flex_layout(classes: ["my-time-tracking-list-view"]) do |flex| %> <% range.each do |date| %> <% flex.with_row do %> - <%= render(Primer::OpenProject::CollapsibleSection.new(collapsed: collapsed?(date))) do |section| %> + <%= render(Primer::OpenProject::CollapsibleSection.new(test_selector: "section-#{date.iso8601}", collapsed: collapsed?(date))) do |section| %> <% section.with_title { date_title(date) } %> <% section.with_caption { date_caption(date) } %> <% section.with_additional_information do %> diff --git a/modules/costs/spec/features/my_time_tracking_spec.rb b/modules/costs/spec/features/my_time_tracking_spec.rb index c8b19270680..716be0a22dd 100644 --- a/modules/costs/spec/features/my_time_tracking_spec.rb +++ b/modules/costs/spec/features/my_time_tracking_spec.rb @@ -58,13 +58,44 @@ RSpec.describe "my time tracking", :js do end let!(:time_entry6) { create(:time_entry, user:, work_package: work_package2, spent_on: "2025-04-14", hours: 2.0) } + let(:allow_exact_time_tracking) { true } + let(:force_exact_time_tracking) { false } + + let(:calendar_page) { Pages::MyTimeTracking::CalendarPage.new } + let(:list_page) { Pages::MyTimeTracking::ListPage.new } + before do + allow(TimeEntry).to receive_messages( + can_track_start_and_end_time?: allow_exact_time_tracking, + must_track_start_and_end_time?: force_exact_time_tracking + ) + login_as user end - it "does something" do - allow(TimeEntry).to receive(:can_track_start_and_end_time?).and_return(true) - visit my_time_tracking_path(date: "2025-04-09", view_mode: "list") - # save_and_open_screenshot + around do |example| + travel_to "2025-04-09T12:00:00Z" do # rubocop:disable RSpecRails/TravelAround + example.run + end + end + + context "when requesting list view" do + before do + visit my_time_tracking_path(date: "2025-04-09", view_mode: "list", mode: "workweek") + end + + it "shows all dates of the current work week" do + list_page.expect_displays_day_section("2025-04-07", collapsed: true) + list_page.expect_displays_day_section("2025-04-08", collapsed: true) + list_page.expect_displays_day_section("2025-04-09", collapsed: false) + list_page.expect_displays_day_section("2025-04-10", collapsed: false) + list_page.expect_displays_day_section("2025-04-11", collapsed: false) + end + end + + context "when requesting calendar view" do # rubocop:disable RSpec/EmptyExampleGroup + before do + visit my_time_tracking_path(date: "2025-04-09", view_mode: "calendar", mode: "workweek") + end end end diff --git a/modules/costs/spec/support/pages/my_time_tracking/calendar_page.rb b/modules/costs/spec/support/pages/my_time_tracking/calendar_page.rb new file mode 100644 index 00000000000..c6f056c7bed --- /dev/null +++ b/modules/costs/spec/support/pages/my_time_tracking/calendar_page.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 "support/pages/page" + +module Pages + module MyTimeTracking + class CalendarPage < Page + end + end +end diff --git a/modules/costs/spec/support/pages/my_time_tracking/list_page.rb b/modules/costs/spec/support/pages/my_time_tracking/list_page.rb new file mode 100644 index 00000000000..fcae03edadc --- /dev/null +++ b/modules/costs/spec/support/pages/my_time_tracking/list_page.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 "support/pages/page" + +module Pages + module MyTimeTracking + class ListPage < Page + def expect_displays_day_section(date, collapsed: false) + selector = if collapsed + "collapsible-section[data-test-selector='section-#{date.to_date.iso8601}'][data-collapsed=true]" + else + "collapsible-section[data-test-selector='section-#{date.to_date.iso8601}']" + end + + expect(page).to have_css(selector) + end + end + end +end From 9ba4722805d22378b237d61690bc9f85cbd87c30 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 10 Jun 2025 09:36:47 +0200 Subject: [PATCH 002/122] working day handling --- .../spec/features/my_time_tracking_spec.rb | 36 ++++++++++++++----- .../pages/my_time_tracking/list_page.rb | 4 +++ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/modules/costs/spec/features/my_time_tracking_spec.rb b/modules/costs/spec/features/my_time_tracking_spec.rb index 716be0a22dd..d788cc018e4 100644 --- a/modules/costs/spec/features/my_time_tracking_spec.rb +++ b/modules/costs/spec/features/my_time_tracking_spec.rb @@ -80,16 +80,36 @@ RSpec.describe "my time tracking", :js do end context "when requesting list view" do - before do - visit my_time_tracking_path(date: "2025-04-09", view_mode: "list", mode: "workweek") + context "for a defined work week", with_settings: { working_days: [1, 3, 4, 5] } do + before do + visit my_time_tracking_path(date: "2025-04-09", view_mode: "list", mode: "workweek") + end + + it "shows all dates of the current work week" do + list_page.expect_displays_day_section("2025-04-07", collapsed: true) + list_page.expect_no_display_day_section("2025-04-08") + list_page.expect_displays_day_section("2025-04-09", collapsed: false) + list_page.expect_displays_day_section("2025-04-10", collapsed: false) + list_page.expect_displays_day_section("2025-04-11", collapsed: false) + list_page.expect_no_display_day_section("2025-04-12") + list_page.expect_no_display_day_section("2025-04-13") + end end - it "shows all dates of the current work week" do - list_page.expect_displays_day_section("2025-04-07", collapsed: true) - list_page.expect_displays_day_section("2025-04-08", collapsed: true) - list_page.expect_displays_day_section("2025-04-09", collapsed: false) - list_page.expect_displays_day_section("2025-04-10", collapsed: false) - list_page.expect_displays_day_section("2025-04-11", collapsed: false) + context "for a full week" do + before do + visit my_time_tracking_path(date: "2025-04-09", view_mode: "list", mode: "week") + end + + it "shows all dates of the current week" do + list_page.expect_displays_day_section("2025-04-07", collapsed: true) + list_page.expect_displays_day_section("2025-04-08", collapsed: true) + list_page.expect_displays_day_section("2025-04-09", collapsed: false) + list_page.expect_displays_day_section("2025-04-10", collapsed: false) + list_page.expect_displays_day_section("2025-04-11", collapsed: false) + list_page.expect_displays_day_section("2025-04-12", collapsed: false) + list_page.expect_displays_day_section("2025-04-13", collapsed: false) + end end end diff --git a/modules/costs/spec/support/pages/my_time_tracking/list_page.rb b/modules/costs/spec/support/pages/my_time_tracking/list_page.rb index fcae03edadc..ca0a51e7057 100644 --- a/modules/costs/spec/support/pages/my_time_tracking/list_page.rb +++ b/modules/costs/spec/support/pages/my_time_tracking/list_page.rb @@ -33,6 +33,10 @@ require "support/pages/page" module Pages module MyTimeTracking class ListPage < Page + def expect_no_display_day_section(date) + expect(page).to have_no_css("collapsible-section[data-test-selector='section-#{date.to_date.iso8601}']") + end + def expect_displays_day_section(date, collapsed: false) selector = if collapsed "collapsible-section[data-test-selector='section-#{date.to_date.iso8601}'][data-collapsed=true]" From 873fe7ba76c3d7866a98cdc72b865249498a88d2 Mon Sep 17 00:00:00 2001 From: as-op Date: Thu, 19 Jun 2025 15:02:12 +0200 Subject: [PATCH 003/122] [#64999] PDF export: workPackageValue macro for float values formats the number without a thousand separator https://community.openproject.org/work_packages/64999 --- config/locales/crowdin/de.yml | 2 +- config/locales/en.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/locales/crowdin/de.yml b/config/locales/crowdin/de.yml index 209870f4849..43fcf113b8b 100644 --- a/config/locales/crowdin/de.yml +++ b/config/locales/crowdin/de.yml @@ -3418,7 +3418,7 @@ de: #Default format for numbers number: format: - delimiter: "" + delimiter: "." precision: 2 separator: "," human: diff --git a/config/locales/en.yml b/config/locales/en.yml index 16f4f7e38a4..2d0d6715068 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3549,7 +3549,7 @@ en: # Default format for numbers number: format: - delimiter: "" + delimiter: "," precision: 3 separator: "." human: From a3e427b1a8f2a418a8f3b0d1b0fcd1805d7d9d05 Mon Sep 17 00:00:00 2001 From: as-op Date: Thu, 19 Jun 2025 15:16:05 +0200 Subject: [PATCH 004/122] adjust localized XLS expectation --- .../xls_export/work_package/exporter/xls_integration_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb b/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb index ad5ae1af501..7aebd1a016b 100644 --- a/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb +++ b/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb @@ -202,7 +202,7 @@ RSpec.describe XlsExport::WorkPackage::Exporter::XLS do expect(sheet.rows.size).to eq(4 + 1) cost_column = sheet.columns.last.to_a - %w[1 99.99 1000].each do |value| + %w[1 99.99 1,000].each do |value| expect(cost_column).to include(value) end end @@ -217,7 +217,7 @@ RSpec.describe XlsExport::WorkPackage::Exporter::XLS do expect(sheet.rows.size).to eq(4 + 1) cost_column = sheet.columns.last.to_a - %w[1 99,99 1000].each do |value| + %w[1 99,99 1.000].each do |value| expect(cost_column).to include(value) end end From a7e0dd3574b9514d2082207333d1d0ad7baf2363 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Thu, 19 Jun 2025 15:51:18 +0200 Subject: [PATCH 005/122] [#64615] add params validation for work package type updates - https://community.openproject.org/work_packages/64615 - added validation of copy_workflow_from param - added validation of project_ids param - restructured set_attributes to run multiple pre-contract validations --- .../copy_workflow_attribute_contract.rb | 51 +++++++++++++ .../update_projects_contract.rb | 35 +++++++++ .../work_packages/types/settings_form.rb | 7 +- .../set_attributes_service.rb | 73 +++++++++++++++---- config/locales/en.yml | 4 + .../copy_workflow_attribute_contract_spec.rb | 65 +++++++++++++++++ .../set_attributes_service_spec.rb | 61 +++++++++++++++- 7 files changed, 281 insertions(+), 15 deletions(-) create mode 100644 app/contracts/work_package_types/copy_workflow_attribute_contract.rb create mode 100644 app/contracts/work_package_types/update_projects_contract.rb create mode 100644 spec/services/work_package_types/copy_workflow_attribute_contract_spec.rb diff --git a/app/contracts/work_package_types/copy_workflow_attribute_contract.rb b/app/contracts/work_package_types/copy_workflow_attribute_contract.rb new file mode 100644 index 00000000000..515d7e1fa04 --- /dev/null +++ b/app/contracts/work_package_types/copy_workflow_attribute_contract.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 WorkPackageTypes + class CopyWorkflowAttributeContract < Dry::Validation::Contract + config.messages.backend = :i18n + + params do + optional(:copy_workflow_from).filled(:string) + end + + rule(:copy_workflow_from) do + next unless key? + + type = Type.find_by(id: value) + if type.nil? + key.failure(:not_found) + next + end + + key.failure(:workflow_missing) if type.workflows.empty? + end + end +end diff --git a/app/contracts/work_package_types/update_projects_contract.rb b/app/contracts/work_package_types/update_projects_contract.rb new file mode 100644 index 00000000000..8af223f7ecb --- /dev/null +++ b/app/contracts/work_package_types/update_projects_contract.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 WorkPackageTypes + class UpdateProjectsContract < BaseContract + attribute :projects + end +end diff --git a/app/forms/work_packages/types/settings_form.rb b/app/forms/work_packages/types/settings_form.rb index c7ab04ee73f..a6ab9bba160 100644 --- a/app/forms/work_packages/types/settings_form.rb +++ b/app/forms/work_packages/types/settings_form.rb @@ -52,7 +52,8 @@ module WorkPackages name: :copy_workflow_from, input_width: :medium, label: I18n.t(:label_copy_workflow_from), - include_blank: true + include_blank: true, + validation_message: validation_message_for(:copy_workflow_from) ) do |other_types| work_package_types.each do |type| other_types.option( @@ -106,6 +107,10 @@ module WorkPackages Type.all end + def validation_message_for(attribute) + model.errors.messages_for(attribute).to_sentence.presence + end + def prefilled_copy_workflow_from @builder.options[:copy_workflow_from] end diff --git a/app/services/work_package_types/set_attributes_service.rb b/app/services/work_package_types/set_attributes_service.rb index b8fc3e8077a..af0265ffb0f 100644 --- a/app/services/work_package_types/set_attributes_service.rb +++ b/app/services/work_package_types/set_attributes_service.rb @@ -30,42 +30,89 @@ module WorkPackageTypes class SetAttributesService < ::BaseServices::SetAttributes + include Dry::Monads[:result] + def initialize(user:, model:, contract_class:, contract_options: nil) super - @valid_pattern = true + @param_validations = {} end private def set_attributes(params) permitted = params.except(:copy_workflow_from) - @valid_pattern = check_patterns(permitted) - if @valid_pattern - super(permitted) - else - super(permitted.except(:patterns)) - end + check_patterns(permitted).or { |failures| @param_validations.update(failures) } + check_copy_workflow(params).or { |failure| @param_validations.update(failure) } + check_projects(permitted).or { |failure| @param_validations.update(failure) } + + super(permitted.except(*@param_validations.keys)) end def validate_and_result success, errors = validate(model, user, options: {}) - if @valid_pattern + if @param_validations.empty? ServiceResult.new(success:, errors:, result: model) else - errors.add(:patterns, :is_invalid) + @param_validations.each_pair do |key, error| + errors.add(key, error) + end + ServiceResult.failure(errors:, result: model) end end def check_patterns(params) - return true unless params.key?(:patterns) - return true if params.key?(:patterns) && params[:patterns].blank? + return Success() unless params.key?(:patterns) + return Success() if params.key?(:patterns) && params[:patterns].blank? - Types::Patterns::CollectionContract.new.call(params[:patterns]).success? + Types::Patterns::CollectionContract + .new + .call(params[:patterns]) + .to_monad + .or { |result| Failure({ patterns: validation_failure_to_message(result).join(", ") }) } rescue ArgumentError - false + Failure({ patterns: :is_invalid }) + end + + def check_copy_workflow(params) + return Success() unless params.key?(:copy_workflow_from) + + CopyWorkflowAttributeContract + .new + .call(params.slice(:copy_workflow_from)) + .to_monad + .or { |result| Failure({ copy_workflow_from: validation_failure_to_message(result).join(", ") }) } + end + + def check_projects(params) + return Success() unless params.key?(:project_ids) + + invalid_project_ids = params[:project_ids].reject { |id| Project.exists?(id) } + + if invalid_project_ids.empty? + Success() + else + Failure({ project_ids: "Projects with ids #{invalid_project_ids.join(', ')} do not exist." }) + end + end + + def validation_failure_to_message(result) + flatten_error_messages result.errors(full: true).to_h + end + + def flatten_error_messages(errors) + case errors + when String + [errors] + when Array + errors.flat_map { |e| flatten_error_messages(e) } + when Hash + errors.values.flat_map { |e| flatten_error_messages(e) } + else + [] + end end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index b4af5f34bc8..4b7eb34f944 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -318,6 +318,8 @@ en: greater_or_equal_zero: "must be greater or equal to 0." not_found: "not found." rules: + copy_workflow_from: + workflow_missing: "has no own workflow." item: root_item: "cannot be a root item." not_persisted: "must be an already existing item." @@ -328,6 +330,8 @@ en: parent: not_descendant: "must be a descendant of the hierarchy root." rules: + copy_workflow_from: "Type for workflow copy" + enabled: "Enabled" depth: "Depth" item: "Item" label: "Label" diff --git a/spec/services/work_package_types/copy_workflow_attribute_contract_spec.rb b/spec/services/work_package_types/copy_workflow_attribute_contract_spec.rb new file mode 100644 index 00000000000..6e7d2934fce --- /dev/null +++ b/spec/services/work_package_types/copy_workflow_attribute_contract_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 WorkPackageTypes::CopyWorkflowAttributeContract do + subject { described_class.new } + + context "if the given work package type id exists and has a workflow" do + let(:wp_type) { create(:type_with_workflow) } + let(:params) { { copy_workflow_from: wp_type.id.to_s } } + + it "is valid" do + expect(subject.call(params)).to be_success + end + end + + context "if the given work package type id does not exists" do + let(:params) { { copy_workflow_from: "NO" } } + + it "is invalid" do + result = subject.call(params) + expect(result).to be_failure + expect(result.errors(full: true)[:copy_workflow_from]).to match_array("Type for workflow copy not found.") + end + end + + context "if the given work package type id does not have a workflow" do + let(:wp_type) { create(:type) } + let(:params) { { copy_workflow_from: wp_type.id.to_s } } + + it "is invalid" do + result = subject.call(params) + expect(result).to be_failure + expect(result.errors(full: true)[:copy_workflow_from]).to match_array("Type for workflow copy has no own workflow.") + end + end +end diff --git a/spec/services/work_package_types/set_attributes_service_spec.rb b/spec/services/work_package_types/set_attributes_service_spec.rb index 3335b9cdf79..08c535061df 100644 --- a/spec/services/work_package_types/set_attributes_service_spec.rb +++ b/spec/services/work_package_types/set_attributes_service_spec.rb @@ -68,7 +68,7 @@ module WorkPackageTypes it "adds an error on the patterns attribute" do result = service.perform(params) - expect(result.errors.details).to eq(patterns: [{ error: :is_invalid }]) + expect(result.errors.details).to eq(patterns: [{ error: "Enabled is missing" }]) end it "does not override the already existing value on the model" do @@ -89,5 +89,64 @@ module WorkPackageTypes expect(model.patterns).to eq(Types::Patterns::Collection.empty) end end + + context "if copy workflow source type does not exist" do + let(:params) { { copy_workflow_from: "1337" } } + + it "fails" do + result = service.perform(params) + expect(result).to be_failure + end + + it "adds an error on the copy_workflow_from attribute" do + result = service.perform(params) + expect(result.errors.details).to eq(copy_workflow_from: [{ error: "Type for workflow copy not found." }]) + end + + it "does not override the already existing value on the model" do + service.perform(params) + expect(model).not_to be_changed + end + end + + context "if copy workflow source type does not have a workflow" do + let(:wp_type) { create(:type_bug) } + let(:params) { { copy_workflow_from: wp_type.id.to_s } } + + it "fails" do + result = service.perform(params) + expect(result).to be_failure + end + + it "adds an error on the copy_workflow_from attribute" do + result = service.perform(params) + expect(result.errors.details).to eq(copy_workflow_from: [{ error: "Type for workflow copy has no own workflow." }]) + end + + it "does not override the already existing value on the model" do + service.perform(params) + expect(model).not_to be_changed + end + end + + context "if project ids contain a not existent id" do + let(:project) { create(:project) } + let(:params) { { project_ids: [project.id.to_s, "1337"] } } + + it "fails" do + result = service.perform(params) + expect(result).to be_failure + end + + it "adds an error on the project_ids attribute" do + result = service.perform(params) + expect(result.errors.details).to eq(project_ids: [{ error: "Projects with ids 1337 do not exist." }]) + end + + it "does not override the already existing value on the model" do + service.perform(params) + expect(model).not_to be_changed + end + end end end From b4e7737347cec381c882bd86d5b7e8e64ba1ab07 Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Fri, 20 Jun 2025 14:27:33 +0200 Subject: [PATCH 006/122] [#64615] fixed parameter preparation in types controller --- app/controllers/types_controller.rb | 6 +++++- spec/services/work_package_types/create_service_spec.rb | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/controllers/types_controller.rb b/app/controllers/types_controller.rb index 5480472dae7..315d65c75c4 100644 --- a/app/controllers/types_controller.rb +++ b/app/controllers/types_controller.rb @@ -59,7 +59,11 @@ class TypesController < ApplicationController end def create - additional_params = { copy_workflow_from: params.dig(:type, :copy_workflow_from) } + additional_params = {} + if (value = params.dig(:type, :copy_workflow_from)) + additional_params[:copy_workflow_from] = value + end + service_call = WorkPackageTypes::CreateService .new(user: current_user) .call(permitted_type_params.merge(additional_params)) diff --git a/spec/services/work_package_types/create_service_spec.rb b/spec/services/work_package_types/create_service_spec.rb index d56eee625d8..bb2121fe752 100644 --- a/spec/services/work_package_types/create_service_spec.rb +++ b/spec/services/work_package_types/create_service_spec.rb @@ -43,7 +43,7 @@ RSpec.describe WorkPackageTypes::CreateService, type: :model do let(:params) do { name: "Order 66", - copy_workflow_from: existing_type.id, + copy_workflow_from: existing_type.id.to_s, is_milestone: false, is_in_roadmap: true, is_default: false From 8b7270e64f214aeb313627b7e94789ba003e95fa Mon Sep 17 00:00:00 2001 From: as-op Date: Mon, 23 Jun 2025 15:16:51 +0200 Subject: [PATCH 007/122] remove test with German locale, as this is not testing the exported values into XLS (they are not export formatted as string) --- .../exporter/xls_integration_spec.rb | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb b/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb index 7aebd1a016b..0c8d5534d0e 100644 --- a/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb +++ b/modules/xls_export/spec/models/xls_export/work_package/exporter/xls_integration_spec.rb @@ -207,22 +207,6 @@ RSpec.describe XlsExport::WorkPackage::Exporter::XLS do end end - context "with german locale" do - let(:current_user) { create(:admin, language: :de) } - - it "exports successfully the work packages with a cost column localized" do - I18n.with_locale :de do - sheet - end - - expect(sheet.rows.size).to eq(4 + 1) - cost_column = sheet.columns.last.to_a - %w[1 99,99 1.000].each do |value| - expect(cost_column).to include(value) - end - end - end - it "includes work" do expect(sheet.rows.size).to eq(4 + 1) From 4f634596449afa26a23b51b85d4fa13d5746b169 Mon Sep 17 00:00:00 2001 From: as-op Date: Mon, 23 Jun 2025 15:28:05 +0200 Subject: [PATCH 008/122] revert change to locale as this must be done in Crowdin --- config/locales/crowdin/de.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/crowdin/de.yml b/config/locales/crowdin/de.yml index 43fcf113b8b..209870f4849 100644 --- a/config/locales/crowdin/de.yml +++ b/config/locales/crowdin/de.yml @@ -3418,7 +3418,7 @@ de: #Default format for numbers number: format: - delimiter: "." + delimiter: "" precision: 2 separator: "," human: From cf2e6ce4b3951197215d6b1ebe66b9a595fa5c69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 23 Jun 2025 11:24:23 +0200 Subject: [PATCH 009/122] Reset default Rack::Timeout wait_timeout to 30, matching default --- config/constants/settings/definition.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/constants/settings/definition.rb b/config/constants/settings/definition.rb index afaf76c0ef3..3dd5f771cd9 100644 --- a/config/constants/settings/definition.rb +++ b/config/constants/settings/definition.rb @@ -1160,7 +1160,7 @@ module Settings default: { "workers" => 2, "timeout" => Rails.env.production? ? 120 : 0, - "wait_timeout" => 10, + "wait_timeout" => 30, "min_threads" => 4, "max_threads" => 16 }, From 05577ccf15478322e5b0392004b3c224634b2dd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 23 Jun 2025 11:23:11 +0200 Subject: [PATCH 010/122] Only suppress integrations, not rails logging --- .../timeout/suppress_internal_error_report_on_timeout.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/rack/timeout/suppress_internal_error_report_on_timeout.rb b/lib/rack/timeout/suppress_internal_error_report_on_timeout.rb index 558859117b1..33ad0d9a937 100644 --- a/lib/rack/timeout/suppress_internal_error_report_on_timeout.rb +++ b/lib/rack/timeout/suppress_internal_error_report_on_timeout.rb @@ -2,7 +2,10 @@ module Rack class Timeout module SuppressInternalErrorReportOnTimeout def op_handle_error(message_or_exception, context = {}) - return if respond_to?(:request) && request.env[Rack::Timeout::ENV_INFO_KEY].try(:state) == :timed_out + if respond_to?(:request) && request.env[Rack::Timeout::ENV_INFO_KEY].try(:state) == :timed_out + Rails.logger.error "Rack::Timeout: Receiving timeout exception: #{message_or_exception}" + return + end super end From 72d44b8ad50529c8918cc00608a120d669335227 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 24 Jun 2025 11:11:08 +0200 Subject: [PATCH 011/122] [65062] Fix infinite loop when getting automatically scheduled ancestors https://community.openproject.org/wp/65062 When there is a cycle in the hierarchy, like trying to set the root parent as a child of its own child, the scheduling dependency would loop when trying to find all the automatically scheduled ancestors of the work package. This is now fixed by marking the visited work packages. --- app/contracts/work_packages/base_contract.rb | 15 ++- .../work_packages/schedule_dependency.rb | 18 ++- .../work_packages/schedule_dependency_spec.rb | 113 ++++++++++++++++++ .../update_service_integration_spec.rb | 30 ++++- 4 files changed, 167 insertions(+), 9 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 65edad4d4a9..22689ac7108 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH @@ -304,11 +306,22 @@ module WorkPackages if model.parent_id_changed? && model.parent_id && errors.exclude?(:parent) && - WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty? + current_parent_unrelatable? + # closure_tree adds an error on :parent_id because of the cycle + # detection, and active_record sees the error when saving the children + # association and adds an error on :children as well. We need to remove + # them. + errors.delete(:parent_id) # remove the error added by closure_tree + errors.delete(:children) # remove the error added by active_record + # add our own error errors.add :parent, :cant_link_a_work_package_with_a_descendant end end + def current_parent_unrelatable? + WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty? + end + def validate_status_exists errors.add :status, :does_not_exist if model.status && !status_exists? end diff --git a/app/services/work_packages/schedule_dependency.rb b/app/services/work_packages/schedule_dependency.rb index 76f291a83a0..bac24732f6c 100644 --- a/app/services/work_packages/schedule_dependency.rb +++ b/app/services/work_packages/schedule_dependency.rb @@ -102,13 +102,21 @@ class WorkPackages::ScheduleDependency def automatically_scheduled_ancestors(work_package) @automatically_scheduled_ancestors ||= {} @automatically_scheduled_ancestors[work_package] ||= begin - parent = parent_of(work_package) + work_packages_to_process = [work_package] + result = [] + processed_ids = Set.new - if parent&.schedule_automatically? - [parent, *automatically_scheduled_ancestors(parent)] - else - [] + while current = work_packages_to_process.shift + processed_ids.add(current.id) + + parent = parent_of(current) + + if parent&.schedule_automatically? + result << parent unless parent.id == work_package.id + work_packages_to_process << parent unless processed_ids.include?(parent.id) + end end + result end end diff --git a/spec/services/work_packages/schedule_dependency_spec.rb b/spec/services/work_packages/schedule_dependency_spec.rb index 47dbb8ae8e8..c9fb9f3522d 100644 --- a/spec/services/work_packages/schedule_dependency_spec.rb +++ b/spec/services/work_packages/schedule_dependency_spec.rb @@ -75,4 +75,117 @@ RSpec.describe WorkPackages::ScheduleDependency do end end end + + describe "#automatically_scheduled_ancestors" do + shared_let(:work_package) { create(:work_package, subject: "work_package") } + let(:schedule_dependency) { described_class.new(work_package) } + + context "with no parent" do + it "returns an empty array" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty + end + end + + context "with a single automatically scheduled parent" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + parent | automatic + TABLE + + before do + work_package.update(parent:) + end + + it "returns the parent" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent) + end + end + + context "with a manually scheduled parent" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + parent | manual + TABLE + + before do + work_package.update(parent:) + end + + it "returns an empty array" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty + end + end + + context "with multiple levels of automatically scheduled ancestors" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + grandparent | automatic + parent | automatic + TABLE + + before do + work_package.update(parent:) + end + + it "returns all automatically scheduled ancestors" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent, grandparent) + end + end + + context "with mixed scheduling modes in the hierarchy" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + grandparent | automatic + parent | manual + TABLE + + before do + work_package.update(parent:) + end + + it "returns only automatically scheduled ancestors" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty + end + end + + context "with a cycle in the hierarchy" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + child | automatic + grandchild | manual + TABLE + + before do + child.update(parent: work_package) + work_package.update(schedule_manually: false) + + # Create the cycle: set the parent with a current child, but do not + # save, like when set by an update during a set attributes service call + work_package.parent = child + end + + it "handles the cycle gracefully and does not cause an infinite loop" do + expect { schedule_dependency.automatically_scheduled_ancestors(work_package) }.not_to raise_error + # Should return the parent but not get stuck in an infinite loop + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(child) + end + end + + context "with a complex hierarchy" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + great_grandparent | automatic + grandparent | manual + parent | automatic + TABLE + + before do + work_package.update(parent:) + end + + it "returns only automatically scheduled ancestors" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent) + end + end + end end diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index b86c09d06e1..7e53bd4277e 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -64,7 +64,7 @@ RSpec.describe WorkPackages::UpdateService, "integration", type: :model do set_factory_default(:user, user) end - shared_let(:work_package, refind: true, reload: false) do + let(:work_package) do create(:work_package, subject: "work_package") end @@ -1892,9 +1892,33 @@ RSpec.describe WorkPackages::UpdateService, "integration", type: :model do } end - # Bug #63499: this was causing an infinite loop when computing the future + # Bug #64973: this was causing an infinite loop when computing the future # dates of the predecessor. - it "displays an error about the inability to have multiple relations between the same work packages (Bug #63499)" do + it "displays an error about the inability to have multiple relations between the same work packages (Bug #64973)" do + expect(subject).to be_failure + + expect(subject.errors.attribute_names).to contain_exactly(:parent) + # the error message in this case is far from ideal + expect(subject.errors.details).to include(parent: [{ error: :cant_link_a_work_package_with_a_descendant }]) + end + end + + context "when a work package with a child and a grandchild is made a child of its child" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + work_package | automatic + child | automatic + grandchild | manual + TABLE + let(:attributes) do + { + parent: child + } + end + + # Bug #65062: this was causing an infinite loop when computing automatically + # scheduled ancestors of the updated work package. + it "displays an error about the inability to have multiple relations between the same work packages (Bug #65062)" do expect(subject).to be_failure expect(subject.errors.attribute_names).to contain_exactly(:parent) From 31573972b0e944ed261bdb24407f8203483898cf Mon Sep 17 00:00:00 2001 From: ulferts Date: Fri, 20 Jun 2025 17:24:52 +0200 Subject: [PATCH 012/122] use a BaseContracted service for finishing attachment upload Those services place a semaphore preventing race conditions on journal creation --- .../finish_direct_upload_service.rb | 142 ++++++++++++++++++ .../attachments/finish_direct_upload_job.rb | 119 +++------------ 2 files changed, 166 insertions(+), 95 deletions(-) create mode 100644 app/services/attachments/finish_direct_upload_service.rb diff --git a/app/services/attachments/finish_direct_upload_service.rb b/app/services/attachments/finish_direct_upload_service.rb new file mode 100644 index 00000000000..5eb3a15ed9d --- /dev/null +++ b/app/services/attachments/finish_direct_upload_service.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +# -- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 Attachments + class FinishDirectUploadService < BaseServices::BaseContracted + def initialize(user:, model:, contract_class: nil, contract_options: {}) + self.model = model + super(user:, contract_class:, contract_options:) + end + + protected + + alias_method :attachment, :model + + def service_context(send_notifications:, &) + # Overwriting the call to in_context to place the semaphore on the container and not on the attachment. + # Since the service will write a journal on the container, there should not be another + # process that modifies the container while this service is running. + in_context(attachment.container, send_notifications:, &) + end + + def validate_params(_params) + super.tap do |call| + validate_local_file_exists(call) + end + end + + def before_perform(*) + super.tap do + set_attachment_parameters + end + end + + def persist(call) + super.tap do + attachment.save! + end + end + + def after_perform(service_call) + super.tap do + journalize_container + attachment_created_event + schedule_jobs + end + end + + def validate_local_file_exists(call) + unless local_file + call.errors.add(:base, "File for attachment #{attachment.filename} was not uploaded.") + call.success = false + end + end + + def set_attachment_parameters + attachment.extend(OpenProject::ChangedBySystem) + attachment.change_by_system do + attachment.status = :uploaded + attachment.set_file_size local_file + attachment.set_content_type local_file + attachment.set_digest local_file + end + end + + def schedule_jobs + attachment.extract_fulltext + end + + def journalize_container + journable = attachment.container + + return unless journable&.class&.journaled? + + # Touching the journable will lead to the journal created next having its own timestamp. + # That timestamp will not adequately reflect the time the attachment was uploaded. This job + # right here might be executed way later than the time the attachment was uploaded. Ideally, + # the journals would be created bearing the time stamps of the attachment's created_at. + # This remains a TODO. + # But with the timestamp update in place as it is, at least the collapsing of aggregated journals + # from days before with the newly uploaded attachment is prevented. + touch_journable(journable) + + Journals::CreateService + .new(journable, attachment.author) + .call + end + + def touch_journable(journable) + # Not using touch here on purpose, + # as to avoid changing lock versions on the journables for this change + attributes = journable.send(:timestamp_attributes_for_update_in_model) + + timestamps = attributes.index_with { Time.current } + journable.update_columns(timestamps) if timestamps.any? + end + + def attachment_created_event + OpenProject::Notifications.send( + OpenProject::Events::ATTACHMENT_CREATED, + attachment: + ) + end + + def default_contract_class + ::Attachments::CreateContract + end + + def local_file + # An attachment is guaranteed to have a file. + # But if the attachment is nil the expression attachment&.file will be nil and attachment&.file.local_file + # will throw a NoMethodError: undefined method local_file' for nil:NilClass`. + attachment&.file&.local_file + end + end +end diff --git a/app/workers/attachments/finish_direct_upload_job.rb b/app/workers/attachments/finish_direct_upload_job.rb index 18865d4cc3d..0438b1b61e3 100644 --- a/app/workers/attachments/finish_direct_upload_job.rb +++ b/app/workers/attachments/finish_direct_upload_job.rb @@ -29,115 +29,44 @@ class Attachments::FinishDirectUploadJob < ApplicationJob queue_with_priority :high - def perform(attachment_id, whitelist: true) + def perform(attachment_id, allowlist: true) attachment = Attachment.pending_direct_upload.find_by(id: attachment_id) - # An attachment is guaranteed to have a file. - # But if the attachment is nil the expression attachment&.file will be nil and attachment&.file.local_file - # will throw a NoMethodError: undefined method local_file' for nil:NilClass`. - local_file = attachment && attachment.file.local_file - if local_file.nil? - return Rails.logger.error("File for attachment #{attachment_id} was not uploaded.") - end - - User.execute_as(attachment.author) do - attach_uploaded_file(attachment, local_file, whitelist) + Attachments::FinishDirectUploadService + .new(user: attachment.author, model: attachment, contract_options: derive_contract_options(allowlist)) + .call + .on_failure do |call| + destroy_attachment_and_log_errors(attachment, call.errors) end end private - def attach_uploaded_file(attachment, local_file, whitelist) - set_attributes_from_file(attachment, local_file) - validate_attachment(attachment, whitelist) - save_attachment(attachment) - journalize_container(attachment) - attachment_created_event(attachment) - schedule_jobs(attachment) - rescue StandardError => e - ::OpenProject.logger.error e + def destroy_attachment_and_log_errors(attachment, errors) attachment.destroy - ensure - FileUtils.rm_rf(local_file.path) + errors = errors.full_messages + + OpenProject.logger.error( + <<~MSG + Failed to finish attachment upload for: + * user: #{attachment.author} + * container: #{attachment.container} + * attachment file name: #{attachment.filename} + + Errors: + #{errors.join("\n")} + MSG + ) end - def set_attributes_from_file(attachment, local_file) - attachment.extend(OpenProject::ChangedBySystem) - attachment.change_by_system do - attachment.status = :uploaded - attachment.set_file_size local_file - attachment.set_content_type local_file - attachment.set_digest local_file - end - end - - def save_attachment(attachment) - attachment.save! if attachment.changed? - end - - def validate_attachment(attachment, whitelist) - contract = create_contract attachment, whitelist - - unless contract.valid? - errors = contract.errors.full_messages.join(", ") - raise "Failed to validate attachment #{attachment.id}: #{errors}" - end - end - - def create_contract(attachment, whitelist) - options = derive_contract_options(whitelist) - ::Attachments::CreateContract.new attachment, - attachment.author, - options: - end - - def schedule_jobs(attachment) - attachment.extract_fulltext - end - - def derive_contract_options(whitelist) - case whitelist + def derive_contract_options(allowlist) + case allowlist when false - { whitelist: [] } + { allowlist: [] } when Array - { whitelist: whitelist.map(&:to_s) } + { allowlist: allowlist.map(&:to_s) } else {} end end - - def journalize_container(attachment) - journable = attachment.container - - return unless journable&.class&.journaled? - - # Touching the journable will lead to the journal created next having its own timestamp. - # That timestamp will not adequately reflect the time the attachment was uploaded. This job - # right here might be executed way later than the time the attachment was uploaded. Ideally, - # the journals would be created bearing the time stamps of the attachment's created_at. - # This remains a TODO. - # But with the timestamp update in place as it is, at least the collapsing of aggregated journals - # from days before with the newly uploaded attachment is prevented. - touch_journable(journable) - - Journals::CreateService - .new(journable, attachment.author) - .call - end - - def touch_journable(journable) - # Not using touch here on purpose, - # as to avoid changing lock versions on the journables for this change - attributes = journable.send(:timestamp_attributes_for_update_in_model) - - timestamps = attributes.index_with { Time.now } - journable.update_columns(timestamps) if timestamps.any? - end - - def attachment_created_event(attachment) - OpenProject::Notifications.send( - OpenProject::Events::ATTACHMENT_CREATED, - attachment: - ) - end end From 0ecd09b06beac943bdfbae285737fcf2029d5deb Mon Sep 17 00:00:00 2001 From: ulferts Date: Fri, 20 Jun 2025 17:25:15 +0200 Subject: [PATCH 013/122] rename to allowlist --- app/contracts/attachments/create_contract.rb | 24 +++++++++---------- app/services/attachments/base_service.rb | 4 ++-- app/workers/backup_job.rb | 2 +- app/workers/exports/export_job.rb | 2 +- config/locales/en.yml | 2 +- .../controllers/bim/bcf/issues_controller.rb | 2 +- .../bim/ifc_models/ifc_models_controller.rb | 12 +++++----- modules/bim/app/models/bim/bcf/viewpoint.rb | 2 +- .../app/models/bim/ifc_models/ifc_model.rb | 2 +- .../bim/ifc_models/set_attributes_service.rb | 2 +- .../ifc_models/shared_contract_examples.rb | 4 ++-- .../work_packages/exports/export_job_spec.rb | 19 +++++++++++---- .../attachments/create_contract_spec.rb | 14 +++++------ .../attachment_resource_shared_examples.rb | 8 +++---- ...nish_direct_upload_job_integration_spec.rb | 6 ++--- .../work_packages/exports/export_job_spec.rb | 16 +++++++++---- 16 files changed, 68 insertions(+), 53 deletions(-) diff --git a/app/contracts/attachments/create_contract.rb b/app/contracts/attachments/create_contract.rb index 2ec0d41d3fe..a3be099f6bc 100644 --- a/app/contracts/attachments/create_contract.rb +++ b/app/contracts/attachments/create_contract.rb @@ -68,29 +68,29 @@ module Attachments end ## - # Validates the content type, if a whitelist is set + # Validates the content type, if a allowlist is set def validate_content_type - # If the whitelist is empty, assume all files are allowed + # If the allowlist is empty, assume all files are allowed # as before - unless matches_whitelist?(attachment_whitelist) - Rails.logger.info { "Uploaded file #{model.filename} with type #{model.content_type} does not match whitelist" } - errors.add :content_type, :not_whitelisted, value: model.content_type + unless matches_allowlist?(attachment_allowlist) + Rails.logger.info { "Uploaded file #{model.filename} with type #{model.content_type} does not match allowlist" } + errors.add :content_type, :not_allowlisted, value: model.content_type end end ## - # Get the user-defined whitelist or a custom whitelist + # Get the user-defined allowlist or a custom allowlist # defined for this invocation - def attachment_whitelist - Array(options.fetch(:whitelist, Setting.attachment_whitelist)) + def attachment_allowlist + Array(options.fetch(:allowlist, Setting.attachment_whitelist)) end ## - # Returns whether the attachment matches the whitelist - def matches_whitelist?(whitelist) - return true if whitelist.empty? + # Returns whether the attachment matches the allowlist + def matches_allowlist?(allowlist) + return true if allowlist.empty? - whitelist.include?(model.content_type) || whitelist.include?("*#{model.extension}") + allowlist.include?(model.content_type) || allowlist.include?("*#{model.extension}") end end end diff --git a/app/services/attachments/base_service.rb b/app/services/attachments/base_service.rb index 3ebf934fd84..fda62e662b3 100644 --- a/app/services/attachments/base_service.rb +++ b/app/services/attachments/base_service.rb @@ -36,8 +36,8 @@ module Attachments # @param whitelist A custom whitelist to validate with, or empty to disable validation # # Warning: When passing an empty whitelist, this results in no validations on the content type taking place. - def self.bypass_whitelist(user:, whitelist: []) - new(user:, contract_options: { whitelist: whitelist.map(&:to_s) }) + def self.bypass_allowlist(user:, allowlist: []) + new(user:, contract_options: { allowlist: allowlist.map(&:to_s) }) end end end diff --git a/app/workers/backup_job.rb b/app/workers/backup_job.rb index 69e75e75b58..9be533fb694 100644 --- a/app/workers/backup_job.rb +++ b/app/workers/backup_job.rb @@ -134,7 +134,7 @@ class BackupJob < ApplicationJob def store_backup(file_name, backup:, user:) File.open(file_name) do |file| call = Attachments::CreateService - .bypass_whitelist(user:) + .bypass_allowlist(user:) .call(container: backup, filename: file_name, file:, description: "OpenProject backup") call.on_success do diff --git a/app/workers/exports/export_job.rb b/app/workers/exports/export_job.rb index 95b28bc110e..fac92e52533 100644 --- a/app/workers/exports/export_job.rb +++ b/app/workers/exports/export_job.rb @@ -117,7 +117,7 @@ module Exports filename = clean_filename(export_result) call = Attachments::CreateService - .bypass_whitelist(user: User.current) + .bypass_allowlist(user: User.current) .call(container:, file:, filename:, description: "") call.on_success do diff --git a/config/locales/en.yml b/config/locales/en.yml index c97dc0b8886..4674703d554 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1286,7 +1286,7 @@ en: attributes: content_type: blank: "The content type of the file cannot be blank." - not_whitelisted: "The file was rejected by an automatic filter. '%{value}' is not whitelisted for upload." + not_allowlisted: "The file was rejected by an automatic filter. '%{value}' is not allowed for upload." format: "%{message}" capability: context: diff --git a/modules/bim/app/controllers/bim/bcf/issues_controller.rb b/modules/bim/app/controllers/bim/bcf/issues_controller.rb index 11949e96083..178b2a7cb77 100644 --- a/modules/bim/app/controllers/bim/bcf/issues_controller.rb +++ b/modules/bim/app/controllers/bim/bcf/issues_controller.rb @@ -215,7 +215,7 @@ module Bim def create_attachment filename = params[:bcf_file].original_filename call = Attachments::CreateService - .bypass_whitelist(user: current_user, whitelist: %w[application/zip]) + .bypass_allowlist(user: current_user, allowlist: %w[application/zip]) .call(file: params[:bcf_file], filename:, description: filename) diff --git a/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb b/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb index 32d47c39c97..af418af6e6b 100644 --- a/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb +++ b/modules/bim/app/controllers/bim/ifc_models/ifc_models_controller.rb @@ -49,6 +49,10 @@ module Bim .includes(:project, :uploader) end + def show + frontend_redirect params[:id].to_i + end + def new @ifc_model = @project.ifc_models.build prepare_form(@ifc_model) @@ -58,10 +62,6 @@ module Bim prepare_form(@ifc_model) end - def show - frontend_redirect params[:id].to_i - end - def defaults frontend_redirect @ifc_models.defaults.pluck(:id).uniq end @@ -116,7 +116,7 @@ module Bim if service_result.success? ::Attachments::FinishDirectUploadJob.perform_later attachment.id, - whitelist: false + allowlist: false flash[:notice] = if new_model t("ifc_models.flash_messages.upload_successful") @@ -183,7 +183,7 @@ module Bim return unless OpenProject::Configuration.direct_uploads? call = ::Attachments::PrepareUploadService - .bypass_whitelist(user: current_user) + .bypass_allowlist(user: current_user) .call(filename: "model.ifc", filesize: 0) call.on_failure { flash[:error] = call.message } diff --git a/modules/bim/app/models/bim/bcf/viewpoint.rb b/modules/bim/app/models/bim/bcf/viewpoint.rb index 668ed857a09..921a6ae134c 100644 --- a/modules/bim/app/models/bim/bcf/viewpoint.rb +++ b/modules/bim/app/models/bim/bcf/viewpoint.rb @@ -73,7 +73,7 @@ module Bim::Bcf def build_snapshot(file, user: User.current) ::Attachments::BuildService - .bypass_whitelist(user:) + .bypass_allowlist(user:) .call(file:, container: self, filename: file.original_filename, description: "snapshot") .result end diff --git a/modules/bim/app/models/bim/ifc_models/ifc_model.rb b/modules/bim/app/models/bim/ifc_models/ifc_model.rb index e878716a430..307d0b2b139 100644 --- a/modules/bim/app/models/bim/ifc_models/ifc_model.rb +++ b/modules/bim/app/models/bim/ifc_models/ifc_model.rb @@ -41,7 +41,7 @@ module Bim delete_attachment name filename = file.respond_to?(:original_filename) ? file.original_filename : File.basename(file.path) call = ::Attachments::CreateService - .bypass_whitelist(user: User.current) + .bypass_allowlist(user: User.current) .call(file:, container: self, filename:, description: name) call.on_failure { Rails.logger.error "Failed to add #{name} attachment: #{call.message}" } diff --git a/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb b/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb index 8e9cc0746d2..eb7c1e79f91 100644 --- a/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb +++ b/modules/bim/app/services/bim/ifc_models/set_attributes_service.rb @@ -78,7 +78,7 @@ module Bim def build_ifc_attachment(ifc_attachment) ::Attachments::BuildService - .bypass_whitelist(user:) + .bypass_allowlist(user:) .call(file: ifc_attachment, container: model, filename: ifc_attachment.original_filename, description: "ifc") .on_failure do |build_attachment_result| build_attachment_result.errors.each do |error| diff --git a/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb b/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb index 28db250bbab..50ab9b411cb 100644 --- a/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb +++ b/modules/bim/spec/contracts/ifc_models/shared_contract_examples.rb @@ -102,7 +102,7 @@ RSpec.shared_examples_for "ifc model contract" do let(:ifc_file) { FileHelpers.mock_uploaded_file name: "model.ifc", content_type: "application/binary", binary: true } let(:ifc_attachment) do Attachments::BuildService - .bypass_whitelist(user: current_user) + .bypass_allowlist(user: current_user) .call(file: ifc_file, filename: "model.ifc") .result end @@ -118,7 +118,7 @@ RSpec.shared_examples_for "ifc model contract" do end let(:ifc_attachment) do Attachments::BuildService - .bypass_whitelist(user: current_user) + .bypass_allowlist(user: current_user) .call(file: ifc_file, filename: "model.ifc") .result end diff --git a/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb b/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb index 491f18d6798..0909582fb4b 100644 --- a/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb +++ b/modules/bim/spec/workers/work_packages/exports/export_job_spec.rb @@ -66,15 +66,14 @@ RSpec.describe WorkPackages::ExportJob do service = double("attachments create service") - expect(Attachments::CreateService) - .to receive(:bypass_whitelist) - .with(user:) + allow(Attachments::CreateService) + .to receive(:bypass_allowlist) .and_return(service) - expect(Exports::CleanupOutdatedJob) + allow(Exports::CleanupOutdatedJob) .to receive(:perform_after_grace) - expect(service) + allow(service) .to(receive(:call)) .and_return(ServiceResult.success(result: attachment)) @@ -85,6 +84,16 @@ RSpec.describe WorkPackages::ExportJob do expect(subject.job_status).to be_present expect(subject.job_status[:status]).to eq "success" expect(subject.job_status[:payload]["download"]).to eq "/api/v3/attachments/1234/content" + + expect(Attachments::CreateService) + .to have_received(:bypass_allowlist) + .with(user:) + + expect(Exports::CleanupOutdatedJob) + .to have_received(:perform_after_grace) + + expect(service) + .to have_received(:call) end end end diff --git a/spec/contracts/attachments/create_contract_spec.rb b/spec/contracts/attachments/create_contract_spec.rb index e2e0367981a..723ab76823b 100644 --- a/spec/contracts/attachments/create_contract_spec.rb +++ b/spec/contracts/attachments/create_contract_spec.rb @@ -104,28 +104,28 @@ RSpec.describe Attachments::CreateContract do end end - context "with an empty whitelist", + context "with an empty allowlist", with_settings: { attachment_whitelist: %w[] } do it_behaves_like "contract is valid" end - context "with a matching mime whitelist", + context "with a matching mime allowlist", with_settings: { attachment_whitelist: %w[image/png] } do it_behaves_like "contract is valid" end - context "with a matching extension whitelist", + context "with a matching extension allowlist", with_settings: { attachment_whitelist: %w[*.png] } do it_behaves_like "contract is valid" end - context "with a non-matching whitelist", + context "with a non-matching allowlist", with_settings: { attachment_whitelist: %w[*.jpg image/jpeg] } do - it_behaves_like "contract is invalid", content_type: :not_whitelisted + it_behaves_like "contract is invalid", content_type: :not_allowlisted context "when disabling the whitelist check" do let(:contract_options) do - { whitelist: [] } + { allowlist: [] } end it_behaves_like "contract is valid" @@ -133,7 +133,7 @@ RSpec.describe Attachments::CreateContract do context "when overriding the whitelist" do let(:contract_options) do - { whitelist: %w[*.png] } + { allowlist: %w[*.png] } end it_behaves_like "contract is valid" diff --git a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb index 19be033f486..5391a8a539a 100644 --- a/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb +++ b/spec/requests/api/v3/attachments/attachment_resource_shared_examples.rb @@ -157,7 +157,7 @@ RSpec.shared_examples "it supports direct uploads" do end end - context "with an attachment whitelist", with_settings: { attachment_whitelist: ["text/csv"] } do + context "with an attachment allowlist", with_settings: { attachment_whitelist: ["text/csv"] } do context "with an allowed content type" do let(:metadata) { { fileName: "cats.csv", fileSize: file.size, contentType: "text/csv" } } @@ -171,14 +171,14 @@ RSpec.shared_examples "it supports direct uploads" do it "fails" do expect(subject.status).to eq 422 - expect(subject.body).to include "not whitelisted" + expect(subject.body).to include "'text/plain' is not allowed for upload." end end - context "with a non-specific content type not on the whitelist" do + context "with a non-specific content type not on the allowlist" do let(:metadata) { { fileName: "cats.bin", fileSize: file.size, contentType: "application/binary" } } - # the actual whitelist check will be performed in the FinishDirectUpload job in this case + # the actual allowlist check will be performed in the FinishDirectUpload job in this case it "still succeeds" do expect(subject.status).to eq 201 end diff --git a/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb b/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb index 08097ce47d1..2df7fb0b761 100644 --- a/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb +++ b/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb @@ -136,7 +136,7 @@ RSpec.describe Attachments::FinishDirectUploadJob, "integration", type: :job do it_behaves_like "adding a journal to the attachment in the name of the attachment's author" end - context "with an incompatible attachment whitelist", + context "with an incompatible attachment allowlist", with_settings: { attachment_whitelist: %w[image/png] } do let!(:container) { create(:work_package) } let!(:container_timestamp) { container.updated_at } @@ -156,9 +156,9 @@ RSpec.describe Attachments::FinishDirectUploadJob, "integration", type: :job do expect { pending_attachment.reload }.to raise_error(ActiveRecord::RecordNotFound) end - context "when the job is getting a whitelist override" do + context "when the job is getting a allowlist override" do it "Does save the attachment" do - job.perform(pending_attachment.id, whitelist: false) + job.perform(pending_attachment.id, allowlist: false) container.reload diff --git a/spec/workers/work_packages/exports/export_job_spec.rb b/spec/workers/work_packages/exports/export_job_spec.rb index 9480c7b2e28..114b492904b 100644 --- a/spec/workers/work_packages/exports/export_job_spec.rb +++ b/spec/workers/work_packages/exports/export_job_spec.rb @@ -68,15 +68,14 @@ RSpec.describe WorkPackages::ExportJob do let(:exporter_instance) { instance_double(exporter) } it "exports" do - expect(Attachments::CreateService) - .to receive(:bypass_whitelist) - .with(user:) + allow(Attachments::CreateService) + .to receive(:bypass_allowlist) .and_return(service) - expect(Exports::CleanupOutdatedJob) + allow(Exports::CleanupOutdatedJob) .to receive(:perform_after_grace) - expect(service) + allow(service) .to receive(:call) do |file:, **_args| expect(File.basename(file)) .to start_with "some_title" @@ -96,6 +95,13 @@ RSpec.describe WorkPackages::ExportJob do expect(subject.job_status.reference).to eq export expect(subject.job_status[:status]).to eq "success" expect(subject.job_status[:payload]["download"]).to eq "/api/v3/attachments/1234/content" + + expect(Attachments::CreateService) + .to have_received(:bypass_allowlist) + .with(user:) + + expect(Exports::CleanupOutdatedJob) + .to have_received(:perform_after_grace) end end From d66741ca8a755f012f6238c83f61aed212c49178 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 24 Jun 2025 11:38:16 +0200 Subject: [PATCH 014/122] [64203] Fix unclear error message on parent self-assignment https://community.openproject.org/wp/64203 When updating a work package to be parent of itself, `closure_tree` gem has hooks to update the work_package_hierarchies table and can detect cycles. When a cycle is detected, it adds a "You cannot add an ancestor as a descendant" error on `:parent_id`. The fix is to remove this error before providing our own error message. --- app/contracts/work_packages/base_contract.rb | 3 +++ spec/contracts/work_packages/base_contract_spec.rb | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 65edad4d4a9..366437f919b 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH @@ -289,6 +291,7 @@ module WorkPackages def validate_parent_not_self if model.parent == model + errors.delete(:parent_id) # remove the error added by closure_tree's cycle detection errors.add :parent, :cannot_be_self_assigned end end diff --git a/spec/contracts/work_packages/base_contract_spec.rb b/spec/contracts/work_packages/base_contract_spec.rb index 776775e118c..64e4daa5cee 100644 --- a/spec/contracts/work_packages/base_contract_spec.rb +++ b/spec/contracts/work_packages/base_contract_spec.rb @@ -1185,8 +1185,6 @@ RSpec.describe WorkPackages::BaseContract do subject do contract.validate - # while we do validate the parent - # the errors are still put on :base so that the messages can be reused contract.errors.symbols_for(:parent) end @@ -1196,6 +1194,7 @@ RSpec.describe WorkPackages::BaseContract do it "returns an error for the parent" do expect(subject) .to eq [:cannot_be_self_assigned] + expect(contract.errors.attribute_names).to contain_exactly(:parent) end end From 3ba3e8b74bd0f9c4460362d5687829ea9accfa21 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 24 Jun 2025 11:55:37 +0200 Subject: [PATCH 015/122] work in review comments --- .../attachments/finish_direct_upload_service.rb | 16 +++++++--------- .../attachments/finish_direct_upload_job.rb | 15 ++++++++++++--- .../finish_direct_upload_job_integration_spec.rb | 12 ++++++++++++ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/app/services/attachments/finish_direct_upload_service.rb b/app/services/attachments/finish_direct_upload_service.rb index 5eb3a15ed9d..545a9e10e54 100644 --- a/app/services/attachments/finish_direct_upload_service.rb +++ b/app/services/attachments/finish_direct_upload_service.rb @@ -59,8 +59,11 @@ module Attachments end def persist(call) - super.tap do - attachment.save! + super.tap do |service_result| + unless attachment.save + service_result.errors = attachment.errors + service_result.success = false + end end end @@ -83,9 +86,7 @@ module Attachments attachment.extend(OpenProject::ChangedBySystem) attachment.change_by_system do attachment.status = :uploaded - attachment.set_file_size local_file - attachment.set_content_type local_file - attachment.set_digest local_file + attachment.file = local_file end end @@ -133,10 +134,7 @@ module Attachments end def local_file - # An attachment is guaranteed to have a file. - # But if the attachment is nil the expression attachment&.file will be nil and attachment&.file.local_file - # will throw a NoMethodError: undefined method local_file' for nil:NilClass`. - attachment&.file&.local_file + attachment&.diskfile end end end diff --git a/app/workers/attachments/finish_direct_upload_job.rb b/app/workers/attachments/finish_direct_upload_job.rb index 0438b1b61e3..0c295ac4d34 100644 --- a/app/workers/attachments/finish_direct_upload_job.rb +++ b/app/workers/attachments/finish_direct_upload_job.rb @@ -32,6 +32,11 @@ class Attachments::FinishDirectUploadJob < ApplicationJob def perform(attachment_id, allowlist: true) attachment = Attachment.pending_direct_upload.find_by(id: attachment_id) + unless attachment + log_not_found(attachment_id) + return + end + Attachments::FinishDirectUploadService .new(user: attachment.author, model: attachment, contract_options: derive_contract_options(allowlist)) .call @@ -49,16 +54,20 @@ class Attachments::FinishDirectUploadJob < ApplicationJob OpenProject.logger.error( <<~MSG Failed to finish attachment upload for: - * user: #{attachment.author} - * container: #{attachment.container} + * user: #{attachment.author_id} - #{attachment.author.name} + * container: #{attachment.container_id} - #{attachment.container} * attachment file name: #{attachment.filename} Errors: - #{errors.join("\n")} + #{errors.join("\n ")} MSG ) end + def log_not_found(attachment_id) + OpenProject.logger.error("Attachment #{attachment_id} not found") + end + def derive_contract_options(allowlist) case allowlist when false diff --git a/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb b/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb index 2df7fb0b761..e6b0aef64e8 100644 --- a/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb +++ b/spec/workers/attachments/finish_direct_upload_job_integration_spec.rb @@ -136,6 +136,18 @@ RSpec.describe Attachments::FinishDirectUploadJob, "integration", type: :job do it_behaves_like "adding a journal to the attachment in the name of the attachment's author" end + context "for an attachment that has been removed in the meantime" do + let!(:container) { create(:work_package) } + + it "logs the error and resolves the job as done" do + allow(OpenProject.logger).to receive(:error) + + job.perform(pending_attachment.id + 1) + + expect(OpenProject.logger).to have_received(:error) + end + end + context "with an incompatible attachment allowlist", with_settings: { attachment_whitelist: %w[image/png] } do let!(:container) { create(:work_package) } From b5d6b6b2997938255c734c29e5cb9744e62e961a Mon Sep 17 00:00:00 2001 From: Eric Schubert Date: Tue, 24 Jun 2025 14:30:38 +0200 Subject: [PATCH 016/122] [#64615] removed result from set_attributes for easier flow --- .../set_attributes_service.rb | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/app/services/work_package_types/set_attributes_service.rb b/app/services/work_package_types/set_attributes_service.rb index af0265ffb0f..2b658ffb80c 100644 --- a/app/services/work_package_types/set_attributes_service.rb +++ b/app/services/work_package_types/set_attributes_service.rb @@ -30,8 +30,6 @@ module WorkPackageTypes class SetAttributesService < ::BaseServices::SetAttributes - include Dry::Monads[:result] - def initialize(user:, model:, contract_class:, contract_options: nil) super @param_validations = {} @@ -42,9 +40,9 @@ module WorkPackageTypes def set_attributes(params) permitted = params.except(:copy_workflow_from) - check_patterns(permitted).or { |failures| @param_validations.update(failures) } - check_copy_workflow(params).or { |failure| @param_validations.update(failure) } - check_projects(permitted).or { |failure| @param_validations.update(failure) } + check_patterns(permitted) + check_copy_workflow(params) + check_projects(permitted) super(permitted.except(*@param_validations.keys)) end @@ -64,37 +62,32 @@ module WorkPackageTypes end def check_patterns(params) - return Success() unless params.key?(:patterns) - return Success() if params.key?(:patterns) && params[:patterns].blank? + return unless params.key?(:patterns) + return if params.key?(:patterns) && params[:patterns].blank? - Types::Patterns::CollectionContract - .new - .call(params[:patterns]) - .to_monad - .or { |result| Failure({ patterns: validation_failure_to_message(result).join(", ") }) } + result = Types::Patterns::CollectionContract.new.call(params[:patterns]) + if result.failure? + @param_validations.update({ patterns: validation_failure_to_message(result).join(", ") }) + end rescue ArgumentError - Failure({ patterns: :is_invalid }) + @param_validations.update({ patterns: :is_invalid }) end def check_copy_workflow(params) - return Success() unless params.key?(:copy_workflow_from) + return unless params.key?(:copy_workflow_from) - CopyWorkflowAttributeContract - .new - .call(params.slice(:copy_workflow_from)) - .to_monad - .or { |result| Failure({ copy_workflow_from: validation_failure_to_message(result).join(", ") }) } + result = CopyWorkflowAttributeContract.new.call(params.slice(:copy_workflow_from)) + if result.failure? + @param_validations.update({ copy_workflow_from: validation_failure_to_message(result).join(", ") }) + end end def check_projects(params) - return Success() unless params.key?(:project_ids) + return unless params.key?(:project_ids) invalid_project_ids = params[:project_ids].reject { |id| Project.exists?(id) } - - if invalid_project_ids.empty? - Success() - else - Failure({ project_ids: "Projects with ids #{invalid_project_ids.join(', ')} do not exist." }) + unless invalid_project_ids.empty? + @param_validations.update({ project_ids: "Projects with ids #{invalid_project_ids.join(', ')} do not exist." }) end end From ba14469d1abe137073779132cb36cf60c853c56f Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Fri, 23 May 2025 15:23:35 +0200 Subject: [PATCH 017/122] Primerize SaaS teaser components --- .../banner_component.html.erb | 12 +++-- .../enterprise_edition/banner_component.rb | 5 ++ .../buy_now_button_component.rb | 48 +++++++++++++++++++ .../enterprise_edition/plan_for_feature.rb | 28 +++++++++-- config/locales/en.yml | 5 ++ 5 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 app/components/enterprise_edition/buy_now_button_component.rb diff --git a/app/components/enterprise_edition/banner_component.html.erb b/app/components/enterprise_edition/banner_component.html.erb index 623fe25cc4b..24df82dec02 100644 --- a/app/components/enterprise_edition/banner_component.html.erb +++ b/app/components/enterprise_edition/banner_component.html.erb @@ -42,10 +42,10 @@ flex.with_row(classes: "op-enterprise-banner--description") do concat render(Primer::Beta::Text.new(tag: :p)) { description } - unless large? - concat render(Primer::Beta::Text.new(tag: :p)) { plan_text } - end + unless large? || teaser? + concat render(Primer::Beta::Text.new(tag: :p)) { plan_text } end + end if features.present? flex.with_row do @@ -68,7 +68,10 @@ end end - flex.with_row(mt: 1) do + flex.with_row(mt: 1) do + if teaser? + render(EnterpriseEdition::BuyNowButtonComponent.new) + else render EnterpriseEdition::UpsellButtonsComponent.new( feature_key, justify_content: (large? ? :center : :flex_start) @@ -76,6 +79,7 @@ end end end + end if medium? grid.with_area(:image, **@image_arguments) diff --git a/app/components/enterprise_edition/banner_component.rb b/app/components/enterprise_edition/banner_component.rb index 80be876e196..864a0110720 100644 --- a/app/components/enterprise_edition/banner_component.rb +++ b/app/components/enterprise_edition/banner_component.rb @@ -107,6 +107,10 @@ module EnterpriseEdition @variant == :inline end + def teaser? + feature_key == :teaser + end + def wrapper_key "enterprise_banner_#{@dismiss_key}" end @@ -135,6 +139,7 @@ module EnterpriseEdition end def render? + return true if teaser? return true if @show_always return false if dismissed? return true if feature_available? && trial_feature? diff --git a/app/components/enterprise_edition/buy_now_button_component.rb b/app/components/enterprise_edition/buy_now_button_component.rb new file mode 100644 index 00000000000..6e371c4ba1d --- /dev/null +++ b/app/components/enterprise_edition/buy_now_button_component.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +#-- copyright + +# OpenProject is an open source project management software. +# Copyright (C) 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 EnterpriseEdition + class BuyNowButtonComponent < ApplicationComponent + include ApplicationHelper + include OpPrimer::ComponentHelpers + + def call + render(Primer::Beta::Button.new( + classes: "upsell-colored-background hidden-for-mobile", + tag: :a, + href: "/admin/subscriptions/new", + align_self: :center + )) do + I18n.t(:button_buy_now) + end + end + end +end diff --git a/app/components/enterprise_edition/plan_for_feature.rb b/app/components/enterprise_edition/plan_for_feature.rb index 6341e5e29d1..2d30dcbe304 100644 --- a/app/components/enterprise_edition/plan_for_feature.rb +++ b/app/components/enterprise_edition/plan_for_feature.rb @@ -37,15 +37,29 @@ module EnterpriseEdition attr_accessor :i18n_scope end + def teaser? + feature_key == :teaser + end + + def current_subscription + @current_subscription ||= Subscription.current + end + def title I18n.t(:title, scope: i18n_scope, default: default_title) end def default_title - I18n.t(feature_key, scope: :"ee.features") + teaser? ? teaser_title : I18n.t(feature_key, scope: :"ee.features") + end + + def teaser_title + I18n.t("ee.teaser.title", count: current_subscription.trial_days_left, trial_plan: current_subscription.enterprise_plan) end def description + return teaser_description if teaser? + @description || begin if I18n.exists?(:description_html, scope: i18n_scope) I18n.t(:description_html, scope: i18n_scope).html_safe @@ -63,6 +77,10 @@ module EnterpriseEdition ) end + def teaser_description + I18n.t("ee.teaser.description", trial_plan: plan_name(current_subscription.enterprise_plan)).html_safe + end + def features defined?(@features) || begin @features = I18n.t(:features, scope: i18n_scope, default: nil)&.values @@ -81,11 +99,13 @@ module EnterpriseEdition end def plan_text - plan_name = render(Primer::Beta::Text.new(font_weight: :bold, classes: "upsell-colored")) do + I18n.t("ee.upsell.plan_text_html", plan_name: plan_name(plan)).html_safe + end + + def plan_name(plan) + render(Primer::Beta::Text.new(font_weight: :bold, classes: "upsell-colored")) do I18n.t("ee.upsell.plan_name", plan: plan.capitalize) end - - I18n.t("ee.upsell.plan_text_html", plan_name:).html_safe end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 73fa5122ddd..3cd9526bdd0 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2216,6 +2216,11 @@ en: internal_comments_inline: title: "Write internal comments only a small group can see" description: " " + teaser: + title: + one: "One day left of %{trial_plan} trial token" + other: "%{count} days left of %{trial_plan} trial token" + description: "You have access to all %{trial_plan} features." trial: not_found: "You have requested a trial token, but that request is no longer available. Please try again." wait_for_confirmation: "We sent you an email to confirm your address in order to retrieve a trial token." From 57fa27dea9191cbde4f44526e1ffa9b7c534bb04 Mon Sep 17 00:00:00 2001 From: Henriette Darge Date: Tue, 27 May 2025 11:47:42 +0200 Subject: [PATCH 018/122] Change scroll implementation for main menu to sticky headers to avoid special solutions when something hooks into the sidemenu (e.g the EE banner) --- .../common/submenu_component.sass | 8 ++++++-- .../src/global_styles/layout/_main_menu.sass | 19 ++++--------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/app/components/open_project/common/submenu_component.sass b/app/components/open_project/common/submenu_component.sass index bba8cd5d7cf..bb30232b319 100644 --- a/app/components/open_project/common/submenu_component.sass +++ b/app/components/open_project/common/submenu_component.sass @@ -2,10 +2,14 @@ height: 100% display: flex flex-direction: column - overflow: hidden &--search - margin: 12px var(--main-menu-x-spacing) + position: sticky + // Header item + 10px padding + top: calc(var(--main-menu-item-height) + 10px) + z-index: 1 + background: var(--main-menu-bg-color) + padding: 12px var(--main-menu-x-spacing) color: var(--main-menu-font-color) display: flex flex-direction: column diff --git a/frontend/src/global_styles/layout/_main_menu.sass b/frontend/src/global_styles/layout/_main_menu.sass index d8489adb461..630ef50ab64 100644 --- a/frontend/src/global_styles/layout/_main_menu.sass +++ b/frontend/src/global_styles/layout/_main_menu.sass @@ -56,21 +56,6 @@ $arrow-left-width: 40px position: relative @include styled-scroll-bar - // Fixed heights to allow inner scrolling - .menu_root.closed, - .menu_root > li.open, - wp-query-select, - .searchable-menu, - .searchable-menu--search-container, - .main-menu--children > li.partial:only-child - height: 100% - - .main-menu--children - // 10px spacing - height: calc(100% - (var(--main-menu-item-height) + 10px)) - overflow: auto - @include styled-scroll-bar - a:not(.Button):focus color: var(--main-menu-font-color) @@ -188,6 +173,10 @@ $arrow-left-width: 40px @include main-menu-hover .main-menu--children-menu-header + position: sticky + top: 0 + z-index: 1 + background: var(--main-menu-bg-color) padding: 10px 10px 0 4px height: calc(var(--main-menu-item-height) + 10px) From e86d7a299b6baf4158b8e1632a7952ce6e37579b Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Mon, 16 Jun 2025 12:20:11 +0200 Subject: [PATCH 019/122] Update banner component to work with SaaS patch --- app/components/enterprise_edition/plan_for_feature.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/components/enterprise_edition/plan_for_feature.rb b/app/components/enterprise_edition/plan_for_feature.rb index 2d30dcbe304..5ec1f3244f8 100644 --- a/app/components/enterprise_edition/plan_for_feature.rb +++ b/app/components/enterprise_edition/plan_for_feature.rb @@ -41,8 +41,9 @@ module EnterpriseEdition feature_key == :teaser end + # Allow providing a subscription def current_subscription - @current_subscription ||= Subscription.current + nil end def title @@ -53,8 +54,9 @@ module EnterpriseEdition teaser? ? teaser_title : I18n.t(feature_key, scope: :"ee.features") end + # Allow providing a title based on the subscription def teaser_title - I18n.t("ee.teaser.title", count: current_subscription.trial_days_left, trial_plan: current_subscription.enterprise_plan) + nil end def description @@ -77,8 +79,9 @@ module EnterpriseEdition ) end + # Allow providing a description based on the subscription def teaser_description - I18n.t("ee.teaser.description", trial_plan: plan_name(current_subscription.enterprise_plan)).html_safe + nil end def features From 7f7292d33e68f9af78038158edd1ddd86cca62a2 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Mon, 16 Jun 2025 12:20:28 +0200 Subject: [PATCH 020/122] Fix teaser positioning --- frontend/src/global_styles/layout/_main_menu.sass | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/src/global_styles/layout/_main_menu.sass b/frontend/src/global_styles/layout/_main_menu.sass index 630ef50ab64..4adfe007903 100644 --- a/frontend/src/global_styles/layout/_main_menu.sass +++ b/frontend/src/global_styles/layout/_main_menu.sass @@ -51,6 +51,8 @@ $arrow-left-width: 40px background-color: var(--main-menu-bg-color) #menu-sidebar + display: flex + flex-direction: column +allow-vertical-scrolling height: calc(100vh - var(--header-height)) position: relative @@ -210,6 +212,8 @@ a.main-menu--parent-node:not(.Button) // logic for showing either parent or child menu .main-menu ul.menu_root + flex-grow: 1 + &.closed li display: none @@ -263,7 +267,7 @@ a.main-menu--parent-node:not(.Button) .main-menu--children display: none -#sidebar +#sidebar:has(> *) margin: 30px 0 0 0 #sidebar, #menu-sidebar .sidebar From 1b6c571e0128bde9d13e7d243435fa6cf2d5c8b2 Mon Sep 17 00:00:00 2001 From: Mir Bhatia Date: Tue, 17 Jun 2025 13:23:45 +0200 Subject: [PATCH 021/122] Move trial teasers from SaaS --- .../enterprise_edition/banner_component.html.erb | 12 ++++-------- .../enterprise_edition/buy_now_button_component.rb | 2 +- .../enterprise_edition/plan_for_feature.rb | 11 ++++------- .../enterprise_edition/upsell_buttons_component.rb | 9 +-------- app/models/enterprise_token.rb | 8 ++++++++ app/views/layouts/base.html.erb | 5 +++++ config/initializers/homescreen.rb | 2 +- config/locales/en.yml | 1 + lib/redmine/menu_manager/top_menu_helper.rb | 5 +++++ 9 files changed, 30 insertions(+), 25 deletions(-) diff --git a/app/components/enterprise_edition/banner_component.html.erb b/app/components/enterprise_edition/banner_component.html.erb index 24df82dec02..513c7900f98 100644 --- a/app/components/enterprise_edition/banner_component.html.erb +++ b/app/components/enterprise_edition/banner_component.html.erb @@ -69,14 +69,10 @@ end flex.with_row(mt: 1) do - if teaser? - render(EnterpriseEdition::BuyNowButtonComponent.new) - else - render EnterpriseEdition::UpsellButtonsComponent.new( - feature_key, - justify_content: (large? ? :center : :flex_start) - ) - end + render EnterpriseEdition::UpsellButtonsComponent.new( + feature_key, + justify_content: (large? ? :center : :flex_start) + ) end end end diff --git a/app/components/enterprise_edition/buy_now_button_component.rb b/app/components/enterprise_edition/buy_now_button_component.rb index 6e371c4ba1d..49cf12bf830 100644 --- a/app/components/enterprise_edition/buy_now_button_component.rb +++ b/app/components/enterprise_edition/buy_now_button_component.rb @@ -38,7 +38,7 @@ module EnterpriseEdition render(Primer::Beta::Button.new( classes: "upsell-colored-background hidden-for-mobile", tag: :a, - href: "/admin/subscriptions/new", + href: OpenProject::Enterprise.upgrade_path, align_self: :center )) do I18n.t(:button_buy_now) diff --git a/app/components/enterprise_edition/plan_for_feature.rb b/app/components/enterprise_edition/plan_for_feature.rb index 5ec1f3244f8..7bff4abcf0f 100644 --- a/app/components/enterprise_edition/plan_for_feature.rb +++ b/app/components/enterprise_edition/plan_for_feature.rb @@ -41,9 +41,8 @@ module EnterpriseEdition feature_key == :teaser end - # Allow providing a subscription - def current_subscription - nil + def token + @token ||= EnterpriseToken.active_trial_tokens.last end def title @@ -54,9 +53,8 @@ module EnterpriseEdition teaser? ? teaser_title : I18n.t(feature_key, scope: :"ee.features") end - # Allow providing a title based on the subscription def teaser_title - nil + I18n.t("ee.teaser.title", count: token.days_left, trial_plan: token.plan) end def description @@ -79,9 +77,8 @@ module EnterpriseEdition ) end - # Allow providing a description based on the subscription def teaser_description - nil + I18n.t("ee.teaser.description", trial_plan: plan_name(token.plan)).html_safe end def features diff --git a/app/components/enterprise_edition/upsell_buttons_component.rb b/app/components/enterprise_edition/upsell_buttons_component.rb index aec2538fd2b..5b34e32a19e 100644 --- a/app/components/enterprise_edition/upsell_buttons_component.rb +++ b/app/components/enterprise_edition/upsell_buttons_component.rb @@ -75,14 +75,7 @@ module EnterpriseEdition return unless EnterpriseToken.active? return unless User.current.admin? - render(Primer::Beta::Button.new( - classes: "upsell-colored-background", - tag: :a, - href: OpenProject::Enterprise.upgrade_path, - align_self: :center - )) do - I18n.t("ee.upsell.buy_now_button") - end + render(EnterpriseEdition::BuyNowButtonComponent.new) end # Allow providing a custom upgrade now button diff --git a/app/models/enterprise_token.rb b/app/models/enterprise_token.rb index b6a159fdbe4..c27a2661959 100644 --- a/app/models/enterprise_token.rb +++ b/app/models/enterprise_token.rb @@ -61,6 +61,10 @@ class EnterpriseToken < ApplicationRecord active_tokens.any? end + def trial_only? + active_non_trial_tokens.empty? && active_trial_tokens.any? + end + def available_features active_tokens.map(&:available_features).flatten.uniq end @@ -218,6 +222,10 @@ class EnterpriseToken < ApplicationRecord [expires_at || FAR_FUTURE_DATE, starts_at || FAR_FUTURE_DATE] end + def days_left + (valid_until.to_date - Time.zone.today).to_i + end + private def strip_encoded_token diff --git a/app/views/layouts/base.html.erb b/app/views/layouts/base.html.erb index 16f75e741a1..f989fbd8177 100644 --- a/app/views/layouts/base.html.erb +++ b/app/views/layouts/base.html.erb @@ -105,6 +105,11 @@ See COPYRIGHT and LICENSE files for more details. <%= main_menu %> <%= content_for :main_menu %> <%= call_hook :view_layouts_base_main_menu %> + <% if current_user.admin? && EnterpriseToken.trial_only? %> + <%= + render(EnterpriseEdition::BannerComponent.new(:teaser, mx: 2, mt: 2, mb: 3)) + %> + <% end %>