From 755c3e09aeaadf2ce4e98ef00e966c6ad0a78fba Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Sun, 19 May 2024 14:42:57 +0200 Subject: [PATCH 01/28] Add public flag to ProjectQuery --- app/models/queries/projects/project_query.rb | 3 +++ db/migrate/20240519123921_add_public_to_project_queries.rb | 6 ++++++ 2 files changed, 9 insertions(+) create mode 100644 db/migrate/20240519123921_add_public_to_project_queries.rb diff --git a/app/models/queries/projects/project_query.rb b/app/models/queries/projects/project_query.rb index 6a44a497199a..6dab3947a2fa 100644 --- a/app/models/queries/projects/project_query.rb +++ b/app/models/queries/projects/project_query.rb @@ -36,6 +36,9 @@ class Queries::Projects::ProjectQuery < ApplicationRecord serialize :orders, coder: Queries::Serialization::Orders.new(self) serialize :selects, coder: Queries::Serialization::Selects.new(self) + scope :public, -> { where(public: true) } + scope :private, ->(user = User.current) { where(public: false, user:) } + def self.model Project end diff --git a/db/migrate/20240519123921_add_public_to_project_queries.rb b/db/migrate/20240519123921_add_public_to_project_queries.rb new file mode 100644 index 000000000000..fa2190b01b9f --- /dev/null +++ b/db/migrate/20240519123921_add_public_to_project_queries.rb @@ -0,0 +1,6 @@ +class AddPublicToProjectQueries < ActiveRecord::Migration[7.1] + def change + add_column :project_queries, :public, :boolean, default: false, null: false + add_index :project_queries, :public + end +end From e57eb91d5a1642d48c614a66cd2417f8c60983a5 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Sun, 19 May 2024 15:04:43 +0200 Subject: [PATCH 02/28] Add `manage_public_project_queries` permission globally --- config/initializers/permissions.rb | 10 +++++++++- config/locales/en.yml | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/config/initializers/permissions.rb b/config/initializers/permissions.rb index 0f474ddcbb9d..799b24da2d8f 100644 --- a/config/initializers/permissions.rb +++ b/config/initializers/permissions.rb @@ -112,7 +112,7 @@ map.permission :select_project_custom_fields, { - 'projects/settings/project_custom_fields': %i[show toggle enable_all_of_section disable_all_of_section] + "projects/settings/project_custom_fields": %i[show toggle enable_all_of_section disable_all_of_section] }, permissible_on: :project, require: :member @@ -178,6 +178,14 @@ permissible_on: :global, require: :loggedin, grant_to_admin: true + + map.permission :manage_public_project_queries, + { + "projects/queries": %i[publish unpublish] + }, + permissible_on: :globa, + require: :loggedin, + grant_to_admin: true end map.project_module :work_package_tracking, order: 90 do |wpt| diff --git a/config/locales/en.yml b/config/locales/en.yml index 258ba4841285..f1334e537ab4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2832,6 +2832,7 @@ Project attributes and sections are defined in the Date: Sun, 19 May 2024 15:04:59 +0200 Subject: [PATCH 03/28] Rename scopes to remove duplication --- app/models/queries/projects/project_query.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/queries/projects/project_query.rb b/app/models/queries/projects/project_query.rb index 6dab3947a2fa..7cfba892f808 100644 --- a/app/models/queries/projects/project_query.rb +++ b/app/models/queries/projects/project_query.rb @@ -36,8 +36,8 @@ class Queries::Projects::ProjectQuery < ApplicationRecord serialize :orders, coder: Queries::Serialization::Orders.new(self) serialize :selects, coder: Queries::Serialization::Selects.new(self) - scope :public, -> { where(public: true) } - scope :private, ->(user = User.current) { where(public: false, user:) } + scope :public_lists, -> { where(public: true) } + scope :private_lists, ->(user = User.current) { where(public: false, user:) } def self.model Project From 3768e72b6273a7d4aec03b937c721654f3130c0a Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Sun, 19 May 2024 16:07:12 +0200 Subject: [PATCH 04/28] Fix typo in permission scope --- config/initializers/permissions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/permissions.rb b/config/initializers/permissions.rb index 799b24da2d8f..c92ee82c1f11 100644 --- a/config/initializers/permissions.rb +++ b/config/initializers/permissions.rb @@ -183,7 +183,7 @@ { "projects/queries": %i[publish unpublish] }, - permissible_on: :globa, + permissible_on: :global, require: :loggedin, grant_to_admin: true end From ca6e583cdbdbd9b6407bd609e33b96e9ebc93e18 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Sun, 19 May 2024 16:07:25 +0200 Subject: [PATCH 05/28] Add methods to publish and unpublish a project list based on permission --- .../index_page_header_component.html.erb | 23 +++++++- .../projects/index_page_header_component.rb | 4 ++ app/components/projects/row_component.rb | 42 +++++++-------- app/components/projects/table_component.rb | 18 ++++--- .../projects/queries/publish_contract.rb | 39 ++++++++++++++ .../projects/queries_controller.rb | 52 +++++++++++-------- app/controllers/projects/query_loading.rb | 10 ++-- .../project_queries/publish_service.rb | 50 ++++++++++++++++++ config/routes.rb | 3 ++ 9 files changed, 187 insertions(+), 54 deletions(-) create mode 100644 app/contracts/projects/queries/publish_contract.rb create mode 100644 app/services/queries/projects/project_queries/publish_service.rb diff --git a/app/components/projects/index_page_header_component.html.erb b/app/components/projects/index_page_header_component.html.erb index 88189b02fea8..dff8c5e76b1c 100644 --- a/app/components/projects/index_page_header_component.html.erb +++ b/app/components/projects/index_page_header_component.html.erb @@ -81,6 +81,28 @@ end if query.persisted? + if can_publish? + if query.public? + menu.with_item( + label: t(:button_unpublish), + scheme: :danger, + href: unpublish_projects_query_path(query), + content_arguments: { data: { method: :post } } + ) do |item| + item.with_leading_visual_icon(icon: 'eye-closed') + end + else + menu.with_item( + label: t(:button_publish), + scheme: :default, + href: publish_projects_query_path(query), + content_arguments: { data: { method: :post } } + ) do |item| + item.with_leading_visual_icon(icon: 'eye') + end + end + end + menu.with_item( label: t(:button_delete), scheme: :danger, @@ -92,7 +114,6 @@ end end %> - <%= render(Projects::ConfigureViewModalComponent.new(query:)) %> <%= render(Projects::DeleteListModalComponent.new(query:)) if query.persisted? %> <%= render(Projects::ExportListModalComponent.new(query:)) %> diff --git a/app/components/projects/index_page_header_component.rb b/app/components/projects/index_page_header_component.rb index fc7f6939bef6..15f19a7ba0ce 100644 --- a/app/components/projects/index_page_header_component.rb +++ b/app/components/projects/index_page_header_component.rb @@ -74,6 +74,10 @@ def can_save? = can_save_as? && query.persisted? && query.user == current_user def can_rename? = may_save_as? && query.persisted? && query.user == current_user && !query.changed? + def can_publish? + current_user.allowed_globally?(:manage_public_project_queries) && query.persisted? + end + def show_state? state == :show end diff --git a/app/components/projects/row_component.rb b/app/components/projects/row_component.rb index 6dd80c99c2cd..11b55719c5a5 100644 --- a/app/components/projects/row_component.rb +++ b/app/components/projects/row_component.rb @@ -46,19 +46,19 @@ def hierarchy def favored render(Primer::Beta::IconButton.new( - icon: currently_favored? ? "star-fill" : "star", - scheme: :invisible, - mobile_icon: currently_favored? ? "star-fill" : "star", - size: :medium, - tag: :a, - tooltip_direction: :e, - href: helpers.build_favorite_path(project, format: :html), - data: { method: currently_favored? ? :delete : :post }, - classes: currently_favored? ? "op-primer--star-icon " : "op-project-row-component--favorite", - label: currently_favored? ? I18n.t(:button_unfavorite) : I18n.t(:button_favorite), - aria: { label: currently_favored? ? I18n.t(:button_unfavorite) : I18n.t(:button_favorite) }, - test_selector: 'project-list-favorite-button' - )) + icon: currently_favored? ? "star-fill" : "star", + scheme: :invisible, + mobile_icon: currently_favored? ? "star-fill" : "star", + size: :medium, + tag: :a, + tooltip_direction: :e, + href: helpers.build_favorite_path(project, format: :html), + data: { method: currently_favored? ? :delete : :post }, + classes: currently_favored? ? "op-primer--star-icon " : "op-project-row-component--favorite", + label: currently_favored? ? I18n.t(:button_unfavorite) : I18n.t(:button_favorite), + aria: { label: currently_favored? ? I18n.t(:button_unfavorite) : I18n.t(:button_favorite) }, + test_selector: "project-list-favorite-button" + )) end def currently_favored? @@ -197,7 +197,7 @@ def column_css_class(column) def additional_css_class(column) if column.attribute == :name "project--hierarchy #{project.archived? ? 'archived' : ''}" - elsif [:status_explanation, :description].include?(column.attribute) + elsif %i[status_explanation description].include?(column.attribute) "project-long-text-container" elsif custom_field_column?(column) cf = column.custom_field @@ -254,7 +254,7 @@ def more_menu_favorite_item href: helpers.build_favorite_path(project, format: :html), data: { method: :post }, label: I18n.t(:button_favorite), - aria: { label: I18n.t(:button_favorite) }, + aria: { label: I18n.t(:button_favorite) } } end @@ -269,7 +269,7 @@ def more_menu_unfavorite_item data: { method: :delete }, classes: "op-primer--star-icon", label: I18n.t(:button_unfavorite), - aria: { label: I18n.t(:button_unfavorite) }, + aria: { label: I18n.t(:button_unfavorite) } } end @@ -302,7 +302,7 @@ def more_menu_activity_item scheme: :default, icon: :check, label: I18n.t(:label_project_activity), - href: project_activity_index_path(project, event_types: ["project_attributes"]), + href: project_activity_index_path(project, event_types: ["project_attributes"]) } end end @@ -317,7 +317,7 @@ def more_menu_archive_item data: { confirm: t("project.archive.are_you_sure", name: project.name), method: :post - }, + } } end end @@ -340,7 +340,7 @@ def more_menu_copy_item scheme: :default, icon: :copy, label: I18n.t(:button_copy), - href: copy_project_path(project), + href: copy_project_path(project) } end end @@ -351,7 +351,7 @@ def more_menu_delete_item scheme: :danger, icon: :trash, label: I18n.t(:button_delete), - href: confirm_destroy_project_path(project), + href: confirm_destroy_project_path(project) } end end @@ -361,7 +361,7 @@ def user_can_view_project? end def custom_field_column?(column) - column.is_a?(Queries::Projects::Selects::CustomField) + column.is_a?(::Queries::Projects::Selects::CustomField) end end end diff --git a/app/components/projects/table_component.rb b/app/components/projects/table_component.rb index 3c053819422b..a2dea976cff6 100644 --- a/app/components/projects/table_component.rb +++ b/app/components/projects/table_component.rb @@ -64,11 +64,18 @@ def build_sort_header(column, options) # We don't return the project row # but the [project, level] array from the helper def rows +<<<<<<< HEAD @rows ||= begin projects_enumerator = ->(model) { to_enum(:projects_with_levels_order_sensitive, model).to_a } instance_exec(model, &projects_enumerator) end +======= + @rows ||= begin + projects_enumerator = ->(model) { to_enum(:projects_with_levels_order_sensitive, model).to_a } + instance_exec(model, &projects_enumerator) + end +>>>>>>> 0e1a80b408 (Add methods to publish and unpublish a project list based on permission) end def initialize_sorted_model @@ -113,12 +120,11 @@ def sortable_column?(select) end def columns - @columns ||= - begin - columns = query.selects.reject { |select| select.is_a?(Queries::Selects::NotExistingSelect) } + @columns ||= begin + columns = query.selects.reject { |select| select.is_a?(::Queries::Selects::NotExistingSelect) } - index = columns.index { |column| column.attribute == :name } - columns.insert(index, Queries::Projects::Selects::Default.new(:hierarchy)) if index + index = columns.index { |column| column.attribute == :name } + columns.insert(index, ::Queries::Projects::Selects::Default.new(:hierarchy)) if index columns end @@ -156,7 +162,7 @@ def projects_with_level(projects, &) end def favored_project_ids - @favored_projects ||= Favorite.where(user: current_user, favored_type: 'Project').pluck(:favored_id) + @favored_projects ||= Favorite.where(user: current_user, favored_type: "Project").pluck(:favored_id) end def sorted_by_lft? diff --git a/app/contracts/projects/queries/publish_contract.rb b/app/contracts/projects/queries/publish_contract.rb new file mode 100644 index 000000000000..ccf53f5ff9bf --- /dev/null +++ b/app/contracts/projects/queries/publish_contract.rb @@ -0,0 +1,39 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class Projects::Queries::PublishContract < BaseContract + validate :allowed_to_modify_public_flag + + protected + + def allowed_to_modify_public_flag + unless user.allowed_globally?(:manage_public_project_queries) + errors.add :base, :error_unauthorized + end + end +end diff --git a/app/controllers/projects/queries_controller.rb b/app/controllers/projects/queries_controller.rb index 273733778fe8..23439331915f 100644 --- a/app/controllers/projects/queries_controller.rb +++ b/app/controllers/projects/queries_controller.rb @@ -31,7 +31,7 @@ class Projects::QueriesController < ApplicationController # No need for a more specific authorization check. That is carried out in the contracts. before_action :require_login - before_action :find_query, only: %i[rename update destroy] + before_action :find_query, only: %i[rename update destroy publish unpublish] before_action :build_query_or_deny_access, only: %i[new create] current_menu_item [:new, :rename, :create, :update] do @@ -55,17 +55,7 @@ def create .new(from: @query, user: current_user) .call(permitted_query_params) - if call.success? - flash[:notice] = I18n.t("lists.create.success") - - redirect_to projects_path(query_id: call.result.id) - else - flash[:error] = I18n.t("lists.create.failure", errors: call.errors.full_messages.join("\n")) - - render template: "/projects/index", - layout: "global", - locals: { query: call.result, state: :edit } - end + render_result(call, success_i18n_key: "lists.create.success", error_i18n_key: "lists.create.failure") end def update @@ -73,17 +63,23 @@ def update .new(user: current_user, model: @query) .call(permitted_query_params) - if call.success? - flash[:notice] = I18n.t("lists.update.success") + render_result(call, success_i18n_key: "lists.update.success", error_i18n_key: "lists.update.failure") + end - redirect_to projects_path(query_id: call.result.id) - else - flash[:error] = I18n.t("lists.update.failure", errors: call.errors.full_messages.join("\n")) + def publish + call = Queries::Projects::ProjectQueries::PublishService + .new(user: current_user, model: @query) + .call(public: true) - render template: "/projects/index", - layout: "global", - locals: { query: call.result, state: :edit } - end + render_result(call, success_i18n_key: "lists.publish.success", error_i18n_key: "lists.publish.failure") + end + + def unpublish + call = Queries::Projects::ProjectQueries::PublishService + .new(user: current_user, model: @query) + .call(public: false) + + render_result(call, success_i18n_key: "lists.unpublish.success", error_i18n_key: "lists.unpublish.failure") end def destroy @@ -95,6 +91,20 @@ def destroy private + def render_result(service_call, success_i18n_key:, error_i18n_key:) + if service_call.success? + flash[:notice] = I18n.t(success_i18n_key) + + redirect_to projects_path(query_id: service_call.result.id) + else + flash[:error] = I18n.t(error_i18n_key, errors: service_call.errors.full_messages.join("\n")) + + render template: "/projects/index", + layout: "global", + locals: { query: service_call.result, state: :edit } + end + end + def find_query @query = Queries::Projects::ProjectQuery.find(params[:id]) end diff --git a/app/controllers/projects/query_loading.rb b/app/controllers/projects/query_loading.rb index 1d0edfa44eac..c7679d9b9b8a 100644 --- a/app/controllers/projects/query_loading.rb +++ b/app/controllers/projects/query_loading.rb @@ -30,10 +30,10 @@ module QueryLoading private def load_query(duplicate:) - Queries::Projects::Factory.find(params[:query_id], - params: permitted_query_params, - user: current_user, - duplicate:) + ::Queries::Projects::Factory.find(params[:query_id], + params: permitted_query_params, + user: current_user, + duplicate:) end def load_query_or_deny_access @@ -55,7 +55,7 @@ def permitted_query_params query_params.merge!(params.require(:query).permit(:name)) end - query_params.merge!(Queries::ParamsParser.parse(params)) + query_params.merge!(::Queries::ParamsParser.parse(params)) query_params.with_indifferent_access end diff --git a/app/services/queries/projects/project_queries/publish_service.rb b/app/services/queries/projects/project_queries/publish_service.rb new file mode 100644 index 000000000000..ad68a1bc45d1 --- /dev/null +++ b/app/services/queries/projects/project_queries/publish_service.rb @@ -0,0 +1,50 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class Queries::Projects::ProjectQueries::PublishService < BaseServices::BaseContracted + include Contracted + + def initialize(user:, model:, contract_class: Projects::Queries::PublishContract) + super(user:, contract_class:) + self.model = model + end + + private + + def after_validate(params, service_call) + model.public = params[:public] + + service_call + end + + def persist(service_call) + model.save + + service_call + end +end diff --git a/config/routes.rb b/config/routes.rb index 7fda512c194f..80c29d6a84e4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -184,6 +184,9 @@ resources :queries, only: %i[new create update destroy] do member do get :rename + + post :publish + post :unpublish end end end From 1954629b84d1975e48842931026e615034522980 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 12:24:29 +0200 Subject: [PATCH 06/28] Add feature flag for Project List sharing while in development --- app/components/projects/index_page_header_component.html.erb | 2 +- config/initializers/feature_decisions.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/components/projects/index_page_header_component.html.erb b/app/components/projects/index_page_header_component.html.erb index dff8c5e76b1c..ef38c2243013 100644 --- a/app/components/projects/index_page_header_component.html.erb +++ b/app/components/projects/index_page_header_component.html.erb @@ -81,7 +81,7 @@ end if query.persisted? - if can_publish? + if can_publish? && OpenProject::FeatureDecisions.project_list_sharing_active? if query.public? menu.with_item( label: t(:button_unpublish), diff --git a/config/initializers/feature_decisions.rb b/config/initializers/feature_decisions.rb index e98381d9be1e..54ed096da78c 100644 --- a/config/initializers/feature_decisions.rb +++ b/config/initializers/feature_decisions.rb @@ -38,3 +38,5 @@ # initializer 'the_engine.feature_decisions' do # OpenProject::FeatureDecisions.add :some_flag # end + +OpenProject::FeatureDecisions.add :project_list_sharing From 76f76d6acb70e32a8eb77f41258c986bfaeb33de Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 12:54:54 +0200 Subject: [PATCH 07/28] Fix Rubocop --- app/components/projects/table_component.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/projects/table_component.rb b/app/components/projects/table_component.rb index a2dea976cff6..3251b7ad0840 100644 --- a/app/components/projects/table_component.rb +++ b/app/components/projects/table_component.rb @@ -162,7 +162,7 @@ def projects_with_level(projects, &) end def favored_project_ids - @favored_projects ||= Favorite.where(user: current_user, favored_type: "Project").pluck(:favored_id) + @favored_project_ids ||= Favorite.where(user: current_user, favored_type: "Project").pluck(:favored_id) end def sorted_by_lft? From 539e2970ea0128fc19385b98a573d7829a4d56e0 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 13:03:08 +0200 Subject: [PATCH 08/28] Add a list item to the sidebar to show public lists --- app/helpers/menus/projects.rb | 11 ++++++++++- app/models/queries/projects/project_query.rb | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/helpers/menus/projects.rb b/app/helpers/menus/projects.rb index fae5464c58ea..dc5e5ea63e7a 100644 --- a/app/helpers/menus/projects.rb +++ b/app/helpers/menus/projects.rb @@ -44,6 +44,8 @@ def first_level_menu_items [ OpenProject::Menu::MenuGroup.new(header: nil, children: main_static_filters), + OpenProject::Menu::MenuGroup.new(header: I18n.t(:"projects.lists.public"), + children: public_filters), OpenProject::Menu::MenuGroup.new(header: I18n.t(:"projects.lists.my_private"), children: my_filters), OpenProject::Menu::MenuGroup.new(header: I18n.t(:"activerecord.attributes.project.status_code"), @@ -76,9 +78,16 @@ def static_filters(ids) end end + def public_filters + ::Queries::Projects::ProjectQuery + .public_lists + .order(:name) + .map { |query| query_menu_item(query) } + end + def my_filters ::Queries::Projects::ProjectQuery - .where(user: current_user) + .private_lists(user: current_user) .order(:name) .map { |query| query_menu_item(query) } end diff --git a/app/models/queries/projects/project_query.rb b/app/models/queries/projects/project_query.rb index 7cfba892f808..cf63a00b060d 100644 --- a/app/models/queries/projects/project_query.rb +++ b/app/models/queries/projects/project_query.rb @@ -37,7 +37,7 @@ class Queries::Projects::ProjectQuery < ApplicationRecord serialize :selects, coder: Queries::Serialization::Selects.new(self) scope :public_lists, -> { where(public: true) } - scope :private_lists, ->(user = User.current) { where(public: false, user:) } + scope :private_lists, ->(user: User.current) { where(public: false, user:) } def self.model Project From e13a8f67e36e083030bbafb5c37fcccf03c822af Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 13:03:27 +0200 Subject: [PATCH 09/28] Add i18n keys for everything I added --- config/locales/en.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/config/locales/en.yml b/config/locales/en.yml index f1334e537ab4..941509808088 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -298,6 +298,7 @@ en: my: "My projects" favored: "Favorite projects" archived: "Archived projects" + public: "Public project lists" my_private: "My private project lists" new: placeholder: "New project list" @@ -345,6 +346,12 @@ Project attributes and sections are defined in the Date: Tue, 21 May 2024 15:20:35 +0200 Subject: [PATCH 10/28] Allow access to public queries for everyone --- app/controllers/projects/queries_controller.rb | 2 +- app/models/queries/projects/factory.rb | 2 +- app/models/queries/projects/project_query.rb | 8 ++++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/queries_controller.rb b/app/controllers/projects/queries_controller.rb index 23439331915f..11940de55099 100644 --- a/app/controllers/projects/queries_controller.rb +++ b/app/controllers/projects/queries_controller.rb @@ -106,6 +106,6 @@ def render_result(service_call, success_i18n_key:, error_i18n_key:) end def find_query - @query = Queries::Projects::ProjectQuery.find(params[:id]) + @query = Queries::Projects::ProjectQuery.visible(user: current_user).find(params[:id]) end end diff --git a/app/models/queries/projects/factory.rb b/app/models/queries/projects/factory.rb index 818c7dd2594e..81c4b7b02f1a 100644 --- a/app/models/queries/projects/factory.rb +++ b/app/models/queries/projects/factory.rb @@ -133,7 +133,7 @@ def find_static_query_and_set_attributes(id, params, user, duplicate:) end def find_persisted_query_and_set_attributes(id, params, user, duplicate:) - query = Queries::Projects::ProjectQuery.where(user:).find_by(id:) + query = Queries::Projects::ProjectQuery.visible(user:).find_by(id:) return unless query diff --git a/app/models/queries/projects/project_query.rb b/app/models/queries/projects/project_query.rb index cf63a00b060d..157ba36c2737 100644 --- a/app/models/queries/projects/project_query.rb +++ b/app/models/queries/projects/project_query.rb @@ -39,6 +39,14 @@ class Queries::Projects::ProjectQuery < ApplicationRecord scope :public_lists, -> { where(public: true) } scope :private_lists, ->(user: User.current) { where(public: false, user:) } + scope :visible, ->(user: User.current) { + public_lists.or(private_lists(user:)) + } + + def visible?(user: User.current) + public? || user == self.user + end + def self.model Project end From 8a450a4c32313a1fa0d2566ec00cc5d131692d18 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 15:20:48 +0200 Subject: [PATCH 11/28] Only redirect to a query when it is still visibil --- app/controllers/projects/queries_controller.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/queries_controller.rb b/app/controllers/projects/queries_controller.rb index 11940de55099..cc37266a0b04 100644 --- a/app/controllers/projects/queries_controller.rb +++ b/app/controllers/projects/queries_controller.rb @@ -91,17 +91,19 @@ def destroy private - def render_result(service_call, success_i18n_key:, error_i18n_key:) + def render_result(service_call, success_i18n_key:, error_i18n_key:) # rubocop:disable Metrics/AbcSize + modified_query = service_call.result + if service_call.success? flash[:notice] = I18n.t(success_i18n_key) - redirect_to projects_path(query_id: service_call.result.id) + redirect_to modified_query.visible? ? projects_path(query_id: modified_query.id) : projects_path else flash[:error] = I18n.t(error_i18n_key, errors: service_call.errors.full_messages.join("\n")) render template: "/projects/index", layout: "global", - locals: { query: service_call.result, state: :edit } + locals: { query: modified_query, state: :edit } end end From cb613396446675bfee5ba52021b6a646fb8bdc43 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 16:20:52 +0200 Subject: [PATCH 12/28] Tests for changes to PRojectQuery model --- .../queries/project_query_factory.rb | 2 + .../queries/projects/project_query_spec.rb | 62 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/spec/factories/queries/project_query_factory.rb b/spec/factories/queries/project_query_factory.rb index a82ddda60a12..969a39389522 100644 --- a/spec/factories/queries/project_query_factory.rb +++ b/spec/factories/queries/project_query_factory.rb @@ -29,6 +29,8 @@ FactoryBot.define do factory :project_query, class: "Queries::Projects::ProjectQuery" do sequence(:name) { |n| "Project query #{n}" } + public { false } + transient do select { [] } end diff --git a/spec/models/queries/projects/project_query_spec.rb b/spec/models/queries/projects/project_query_spec.rb index 61c9d1565c2b..f8869774f3df 100644 --- a/spec/models/queries/projects/project_query_spec.rb +++ b/spec/models/queries/projects/project_query_spec.rb @@ -387,4 +387,66 @@ end end end + + describe "scopes" do + describe ".public_lists" do + it "returns only public lists" do + public_query = create(:project_query, public: true) + public_query_other_user = create(:project_query, public: true) + private_query = create(:project_query, public: false) + + expect(described_class.public_lists).to contain_exactly(public_query, public_query_other_user) + end + end + + describe ".private_lists" do + it "returns only private lists owned by the user" do + public_query = create(:project_query, public: true) + private_query = create(:project_query, public: false) + private_query_other_user = create(:project_query, public: false) + + expect(described_class.private_lists(user: private_query.user)).to contain_exactly(private_query) + end + end + + describe ".visible" do + it "returns public and private queries owned by the user" do + public_query = create(:project_query, public: true) + public_query_other_user = create(:project_query, public: true) + private_query = create(:project_query, public: false) + private_query_other_user = create(:project_query, public: false) + + expect(described_class.visible(user: private_query.user)).to contain_exactly(public_query, public_query_other_user, + private_query) + end + end + end + + describe "#visible?" do + let(:public) { false } + + subject { build(:project_query, user: owner, public:) } + + context "when the user is the owner" do + let(:owner) { user } + + it { is_expected.to be_visible(user:) } + end + + context "when the user is not the owner" do + let(:owner) { build(:user) } + + context "and the query is public" do + let(:public) { true } + + it { is_expected.to be_visible(user:) } + end + + context "and the query is private" do + let(:public) { false } + + it { is_expected.not_to be_visible(user:) } + end + end + end end From 62c953558016d2f4a4332ef1a3612d510f052eb5 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 17:01:51 +0200 Subject: [PATCH 13/28] Add a convenience show route that redirects to the queried page --- app/controllers/projects/queries_controller.rb | 6 +++++- config/routes.rb | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/queries_controller.rb b/app/controllers/projects/queries_controller.rb index cc37266a0b04..37377be694e0 100644 --- a/app/controllers/projects/queries_controller.rb +++ b/app/controllers/projects/queries_controller.rb @@ -31,13 +31,17 @@ class Projects::QueriesController < ApplicationController # No need for a more specific authorization check. That is carried out in the contracts. before_action :require_login - before_action :find_query, only: %i[rename update destroy publish unpublish] + before_action :find_query, only: %i[show rename update destroy publish unpublish] before_action :build_query_or_deny_access, only: %i[new create] current_menu_item [:new, :rename, :create, :update] do :projects end + def show + redirect_to projects_path(query_id: @query.id) + end + def new render template: "/projects/index", layout: "global", diff --git a/config/routes.rb b/config/routes.rb index 80c29d6a84e4..06c8a660b8b4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -181,7 +181,7 @@ namespace :projects do resource :menu, only: %i[show] - resources :queries, only: %i[new create update destroy] do + resources :queries, only: %i[show new create update destroy] do member do get :rename From 6a835f4f2a06d1a25cbc175f9db280980b8a5dcf Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 17:02:21 +0200 Subject: [PATCH 14/28] Allow editing public queries when you have the permission --- .../projects/index_page_header_component.rb | 12 ++++++++++- .../projects/project_queries/base_contract.rb | 20 +++++++++++++++---- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/app/components/projects/index_page_header_component.rb b/app/components/projects/index_page_header_component.rb index 15f19a7ba0ce..34f7ed4b1537 100644 --- a/app/components/projects/index_page_header_component.rb +++ b/app/components/projects/index_page_header_component.rb @@ -70,7 +70,17 @@ def may_save_as? = current_user.logged? def can_save_as? = may_save_as? && query.changed? - def can_save? = can_save_as? && query.persisted? && query.user == current_user + def can_save? + return false unless current_user.logged? + return false unless query.persisted? + return false unless query.changed? + + if query.public? + current_user.allowed_globally?(:manage_public_project_queries) + else + query.user == current_user + end + end def can_rename? = may_save_as? && query.persisted? && query.user == current_user && !query.changed? diff --git a/app/contracts/queries/projects/project_queries/base_contract.rb b/app/contracts/queries/projects/project_queries/base_contract.rb index 4cc8bcd14d29..eae0f2228971 100644 --- a/app/contracts/queries/projects/project_queries/base_contract.rb +++ b/app/contracts/queries/projects/project_queries/base_contract.rb @@ -41,14 +41,26 @@ def self.model presence: true, length: { maximum: 255 } - validate :user_is_current_user_and_logged_in validate :name_select_included validate :existing_selects + validate :allowed_to_modify_private_query + validate :allowed_to_modify_public_query + protected - def user_is_current_user_and_logged_in - unless user.logged? && user == model.user - errors.add :base, :error_unauthorized + def allowed_to_modify_private_query + return if model.public? + + if model.user != user + errors.add :base, :can_only_be_modified_by_owner + end + end + + def allowed_to_modify_public_query + return unless model.public? + + unless user.allowed_globally?(:manage_public_project_queries) + errors.add :base, :need_permission_to_modify_public_query end end From 282d02b078c2874acf959dcced56c3e20d03e888 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 17:02:46 +0200 Subject: [PATCH 15/28] Fix publishing with more knowledge about Contracts --- .../project_queries}/publish_contract.rb | 12 ++------ .../project_queries/publish_service.rb | 29 +++++++++---------- 2 files changed, 17 insertions(+), 24 deletions(-) rename app/contracts/{projects/queries => queries/projects/project_queries}/publish_contract.rb (82%) diff --git a/app/contracts/projects/queries/publish_contract.rb b/app/contracts/queries/projects/project_queries/publish_contract.rb similarity index 82% rename from app/contracts/projects/queries/publish_contract.rb rename to app/contracts/queries/projects/project_queries/publish_contract.rb index ccf53f5ff9bf..98a9fe2844c9 100644 --- a/app/contracts/projects/queries/publish_contract.rb +++ b/app/contracts/queries/projects/project_queries/publish_contract.rb @@ -26,14 +26,8 @@ # See COPYRIGHT and LICENSE files for more details. #++ -class Projects::Queries::PublishContract < BaseContract - validate :allowed_to_modify_public_flag - - protected - - def allowed_to_modify_public_flag - unless user.allowed_globally?(:manage_public_project_queries) - errors.add :base, :error_unauthorized - end +module Queries::Projects::ProjectQueries + class PublishContract < BaseContract + attribute :public end end diff --git a/app/services/queries/projects/project_queries/publish_service.rb b/app/services/queries/projects/project_queries/publish_service.rb index ad68a1bc45d1..e2c26fcda821 100644 --- a/app/services/queries/projects/project_queries/publish_service.rb +++ b/app/services/queries/projects/project_queries/publish_service.rb @@ -26,25 +26,24 @@ # See COPYRIGHT and LICENSE files for more details. #++ -class Queries::Projects::ProjectQueries::PublishService < BaseServices::BaseContracted - include Contracted +module Queries::Projects::ProjectQueries + class PublishService < BaseServices::Update + private - def initialize(user:, model:, contract_class: Projects::Queries::PublishContract) - super(user:, contract_class:) - self.model = model - end - - private + def after_validate(params, service_call) + model.public = params[:public] - def after_validate(params, service_call) - model.public = params[:public] + service_call + end - service_call - end + def persist(service_call) + model.save - def persist(service_call) - model.save + service_call + end - service_call + def default_contract_class + Queries::Projects::ProjectQueries::PublishContract + end end end From 6988711217ccc0c471b56370ea2890f0bfa1d1d0 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Tue, 21 May 2024 17:06:32 +0200 Subject: [PATCH 16/28] Fix accidentally commited merge --- app/components/projects/table_component.rb | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/app/components/projects/table_component.rb b/app/components/projects/table_component.rb index 3251b7ad0840..8d68f035dbc8 100644 --- a/app/components/projects/table_component.rb +++ b/app/components/projects/table_component.rb @@ -64,18 +64,10 @@ def build_sort_header(column, options) # We don't return the project row # but the [project, level] array from the helper def rows -<<<<<<< HEAD - @rows ||= - begin - projects_enumerator = ->(model) { to_enum(:projects_with_levels_order_sensitive, model).to_a } - instance_exec(model, &projects_enumerator) - end -======= @rows ||= begin projects_enumerator = ->(model) { to_enum(:projects_with_levels_order_sensitive, model).to_a } instance_exec(model, &projects_enumerator) end ->>>>>>> 0e1a80b408 (Add methods to publish and unpublish a project list based on permission) end def initialize_sorted_model @@ -126,8 +118,8 @@ def columns index = columns.index { |column| column.attribute == :name } columns.insert(index, ::Queries::Projects::Selects::Default.new(:hierarchy)) if index - columns - end + columns + end end def projects(query) From 438afa8bb51b090206fddd70da5cead6da8089cb Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Wed, 22 May 2024 10:04:25 +0200 Subject: [PATCH 17/28] Fix specs for the factory by mocking the new query --- spec/models/queries/projects/factory_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/models/queries/projects/factory_spec.rb b/spec/models/queries/projects/factory_spec.rb index 53a50b4f973a..4d9fc2d5cab1 100644 --- a/spec/models/queries/projects/factory_spec.rb +++ b/spec/models/queries/projects/factory_spec.rb @@ -31,11 +31,11 @@ RSpec.describe Queries::Projects::Factory, with_settings: { enabled_projects_columns: %w[favored name project_status] } do - let!(:query_finder) do + before do scope = instance_double(ActiveRecord::Relation) allow(Queries::Projects::ProjectQuery) - .to receive(:where) + .to receive(:visible) .with(user: current_user) .and_return(scope) @@ -44,6 +44,7 @@ .with(id:) .and_return(persisted_query) end + let(:persisted_query) do build_stubbed(:project_query, name: "My query") do |query| query.order(id: :asc) From eef97fc95969feec899c3d6b111592703c0d4fa9 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Wed, 22 May 2024 12:35:28 +0200 Subject: [PATCH 18/28] Fix the QueriesController so that the user is the proper owner of the query --- .../projects/queries_controller_spec.rb | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/spec/controllers/projects/queries_controller_spec.rb b/spec/controllers/projects/queries_controller_spec.rb index 83f1314bb148..e1bd4542e308 100644 --- a/spec/controllers/projects/queries_controller_spec.rb +++ b/spec/controllers/projects/queries_controller_spec.rb @@ -39,7 +39,7 @@ end context "when logged in" do - let(:query) { build_stubbed(:project_query) } + let(:query) { build_stubbed(:project_query, user:) } let(:query_id) { "42" } let(:query_params) { double } @@ -110,7 +110,7 @@ end context "when logged in" do - let(:query) { build_stubbed(:project_query) } + let(:query) { build_stubbed(:project_query, user:) } let(:query_params) { double } let(:service_instance) { instance_double(service_class) } let(:service_result) { instance_double(ServiceResult, success?: success?, result: query) } @@ -181,7 +181,7 @@ end context "when logged in" do - let(:query) { build_stubbed(:project_query) } + let(:query) { build_stubbed(:project_query, user:) } let(:query_id) { "42" } let(:query_params) { double } let(:service_instance) { instance_double(service_class) } @@ -190,7 +190,9 @@ before do allow(controller).to receive(:permitted_query_params).and_return(query_params) - allow(Queries::Projects::ProjectQuery).to receive(:find).with(query_id).and_return(query) + scope = instance_double(ActiveRecord::Relation) + allow(Queries::Projects::ProjectQuery).to receive(:visible).and_return(scope) + allow(scope).to receive(:find).with(query_id).and_return(query) allow(service_class).to receive(:new).with(model: query, user:).and_return(service_instance) allow(service_instance).to receive(:call).with(query_params).and_return(service_result) @@ -252,12 +254,15 @@ end context "when logged in" do - let(:query) { build_stubbed(:project_query) } + let(:query) { build_stubbed(:project_query, user:) } let(:query_id) { "42" } let(:service_instance) { instance_spy(service_class) } before do - allow(Queries::Projects::ProjectQuery).to receive(:find).with(query_id).and_return(query) + scope = instance_double(ActiveRecord::Relation) + allow(Queries::Projects::ProjectQuery).to receive(:visible).and_return(scope) + allow(scope).to receive(:find).with(query_id).and_return(query) + allow(service_class).to receive(:new).with(model: query, user:).and_return(service_instance) login_as user From 175bacf0e2e0c7d25f5d6997baa9886ce3d9f11c Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Wed, 22 May 2024 12:59:18 +0200 Subject: [PATCH 19/28] Fix specs --- .../projects/project_queries/base_contract.rb | 7 +++ config/locales/en.yml | 2 + .../shared_contract_examples.rb | 46 ++++++++++++++++++- 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/app/contracts/queries/projects/project_queries/base_contract.rb b/app/contracts/queries/projects/project_queries/base_contract.rb index eae0f2228971..c3996e6cb034 100644 --- a/app/contracts/queries/projects/project_queries/base_contract.rb +++ b/app/contracts/queries/projects/project_queries/base_contract.rb @@ -43,11 +43,18 @@ def self.model validate :name_select_included validate :existing_selects + validate :user_is_logged_in validate :allowed_to_modify_private_query validate :allowed_to_modify_public_query protected + def user_is_logged_in + if !user.logged? + errors.add :base, :error_unauthorized + end + end + def allowed_to_modify_private_query return if model.public? diff --git a/config/locales/en.yml b/config/locales/en.yml index 941509808088..8c3d8b8f422b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -991,6 +991,8 @@ Project attributes and sections are defined in the Date: Wed, 22 May 2024 13:12:04 +0200 Subject: [PATCH 20/28] Add conroller tests --- .../projects/queries_controller_spec.rb | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/spec/controllers/projects/queries_controller_spec.rb b/spec/controllers/projects/queries_controller_spec.rb index e1bd4542e308..60dc3a371b86 100644 --- a/spec/controllers/projects/queries_controller_spec.rb +++ b/spec/controllers/projects/queries_controller_spec.rb @@ -31,6 +31,23 @@ RSpec.describe Projects::QueriesController do shared_let(:user) { create(:user) } + describe "#show" do + let(:query) { build_stubbed(:project_query, user:) } + + before do + scope = instance_double(ActiveRecord::Relation) + allow(Queries::Projects::ProjectQuery).to receive(:visible).with(user:).and_return(scope) + allow(scope).to receive(:find).with(query.id.to_s).and_return(query) + + login_as user + end + + it "redirects to the projects page" do + get :show, params: { id: query.id } + expect(response).to redirect_to(projects_path(query_id: query.id)) + end + end + describe "#new" do it "requires login" do get "new" @@ -244,6 +261,152 @@ end end + describe "#publish" do + let(:service_class) { Queries::Projects::ProjectQueries::PublishService } + + it "requires login" do + put "publish", params: { id: 42 } + + expect(response).not_to be_successful + end + + context "when logged in" do + let(:query) { build_stubbed(:project_query, user:) } + let(:query_id) { "42" } + let(:query_params) { { public: true } } + let(:service_instance) { instance_double(service_class) } + let(:service_result) { instance_double(ServiceResult, success?: success?, result: query) } + let(:success?) { true } + + before do + allow(controller).to receive(:permitted_query_params).and_return(query_params) + scope = instance_double(ActiveRecord::Relation) + allow(Queries::Projects::ProjectQuery).to receive(:visible).and_return(scope) + allow(scope).to receive(:find).with(query_id).and_return(query) + allow(service_class).to receive(:new).with(model: query, user:).and_return(service_instance) + allow(service_instance).to receive(:call).with(query_params).and_return(service_result) + + login_as user + end + + it "calls publish service on query" do + put "publish", params: { id: 42 } + + expect(service_instance).to have_received(:call).with(query_params) + end + + context "when service call succeeds" do + it "redirects to projects" do + allow(I18n).to receive(:t).with("lists.publish.success").and_return("foo") + + put "publish", params: { id: 42 } + + expect(flash[:notice]).to eq("foo") + expect(response).to redirect_to(projects_path(query_id: query.id)) + end + end + + context "when service call fails" do + let(:success?) { false } + let(:errors) { instance_double(ActiveModel::Errors, full_messages: ["something", "went", "wrong"]) } + + before do + allow(service_result).to receive(:errors).and_return(errors) + end + + it "renders projects/index" do + allow(I18n).to receive(:t).with("lists.publish.failure", errors: "something\nwent\nwrong").and_return("bar") + + put "publish", params: { id: 42 } + + expect(flash[:error]).to eq("bar") + expect(response).to render_template("projects/index") + end + + it "passes variables to template" do + allow(controller).to receive(:render).and_call_original + + put "update", params: { id: 42 } + + expect(controller).to have_received(:render).with(include(locals: { query:, state: :edit })) + end + end + end + end + + describe "#unpublish" do + let(:service_class) { Queries::Projects::ProjectQueries::PublishService } + + it "requires login" do + put "unpublish", params: { id: 42 } + + expect(response).not_to be_successful + end + + context "when logged in" do + let(:query) { build_stubbed(:project_query, user:) } + let(:query_id) { "42" } + let(:query_params) { { public: false } } + let(:service_instance) { instance_double(service_class) } + let(:service_result) { instance_double(ServiceResult, success?: success?, result: query) } + let(:success?) { true } + + before do + allow(controller).to receive(:permitted_query_params).and_return(query_params) + scope = instance_double(ActiveRecord::Relation) + allow(Queries::Projects::ProjectQuery).to receive(:visible).and_return(scope) + allow(scope).to receive(:find).with(query_id).and_return(query) + allow(service_class).to receive(:new).with(model: query, user:).and_return(service_instance) + allow(service_instance).to receive(:call).with(query_params).and_return(service_result) + + login_as user + end + + it "calls publish service on query" do + put "unpublish", params: { id: 42 } + + expect(service_instance).to have_received(:call).with(query_params) + end + + context "when service call succeeds" do + it "redirects to projects" do + allow(I18n).to receive(:t).with("lists.unpublish.success").and_return("foo") + + put "unpublish", params: { id: 42 } + + expect(flash[:notice]).to eq("foo") + expect(response).to redirect_to(projects_path(query_id: query.id)) + end + end + + context "when service call fails" do + let(:success?) { false } + let(:errors) { instance_double(ActiveModel::Errors, full_messages: ["something", "went", "wrong"]) } + + before do + allow(service_result).to receive(:errors).and_return(errors) + end + + it "renders projects/index" do + allow(I18n).to receive(:t).with("lists.unpublish.failure", errors: "something\nwent\nwrong").and_return("bar") + + put "unpublish", params: { id: 42 } + + expect(flash[:error]).to eq("bar") + expect(response).to render_template("projects/index") + end + + it "passes variables to template" do + allow(controller).to receive(:render).and_call_original + + put "unpublish", params: { id: 42 } + + expect(controller).to have_received(:render).with(include(locals: { query:, state: :edit })) + end + end + end + end + describe "#destroy" do let(:service_class) { Queries::Projects::ProjectQueries::DeleteService } From 03b9411547f7875a79a91807ec41225b219a6726 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Wed, 22 May 2024 13:19:07 +0200 Subject: [PATCH 21/28] Fix Lint/UselessAssignment errors --- spec/models/queries/projects/project_query_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/models/queries/projects/project_query_spec.rb b/spec/models/queries/projects/project_query_spec.rb index f8869774f3df..c2fc44a5d631 100644 --- a/spec/models/queries/projects/project_query_spec.rb +++ b/spec/models/queries/projects/project_query_spec.rb @@ -393,7 +393,7 @@ it "returns only public lists" do public_query = create(:project_query, public: true) public_query_other_user = create(:project_query, public: true) - private_query = create(:project_query, public: false) + create(:project_query, public: false) expect(described_class.public_lists).to contain_exactly(public_query, public_query_other_user) end @@ -401,9 +401,9 @@ describe ".private_lists" do it "returns only private lists owned by the user" do - public_query = create(:project_query, public: true) + create(:project_query, public: true) private_query = create(:project_query, public: false) - private_query_other_user = create(:project_query, public: false) + create(:project_query, public: false) expect(described_class.private_lists(user: private_query.user)).to contain_exactly(private_query) end @@ -414,7 +414,7 @@ public_query = create(:project_query, public: true) public_query_other_user = create(:project_query, public: true) private_query = create(:project_query, public: false) - private_query_other_user = create(:project_query, public: false) + create(:project_query, public: false) expect(described_class.visible(user: private_query.user)).to contain_exactly(public_query, public_query_other_user, private_query) From 1358215a0a0daab31137c51be56e0b645eee8e86 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 23 May 2024 08:51:58 +0200 Subject: [PATCH 22/28] Move feature decision check into the can_publish? method --- app/components/projects/index_page_header_component.html.erb | 2 +- app/components/projects/index_page_header_component.rb | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/components/projects/index_page_header_component.html.erb b/app/components/projects/index_page_header_component.html.erb index ef38c2243013..dff8c5e76b1c 100644 --- a/app/components/projects/index_page_header_component.html.erb +++ b/app/components/projects/index_page_header_component.html.erb @@ -81,7 +81,7 @@ end if query.persisted? - if can_publish? && OpenProject::FeatureDecisions.project_list_sharing_active? + if can_publish? if query.public? menu.with_item( label: t(:button_unpublish), diff --git a/app/components/projects/index_page_header_component.rb b/app/components/projects/index_page_header_component.rb index 34f7ed4b1537..d5fab8b4e365 100644 --- a/app/components/projects/index_page_header_component.rb +++ b/app/components/projects/index_page_header_component.rb @@ -85,7 +85,9 @@ def can_save? def can_rename? = may_save_as? && query.persisted? && query.user == current_user && !query.changed? def can_publish? - current_user.allowed_globally?(:manage_public_project_queries) && query.persisted? + OpenProject::FeatureDecisions.project_list_sharing_active? && + current_user.allowed_globally?(:manage_public_project_queries) && + query.persisted? end def show_state? From ca42c30f950c0ed315520cfee98fb8f003afe298 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 23 May 2024 08:53:56 +0200 Subject: [PATCH 23/28] Use unless in favor of if ! --- app/contracts/queries/projects/project_queries/base_contract.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/contracts/queries/projects/project_queries/base_contract.rb b/app/contracts/queries/projects/project_queries/base_contract.rb index c3996e6cb034..da00abf3366a 100644 --- a/app/contracts/queries/projects/project_queries/base_contract.rb +++ b/app/contracts/queries/projects/project_queries/base_contract.rb @@ -50,7 +50,7 @@ def self.model protected def user_is_logged_in - if !user.logged? + unless user.logged? errors.add :base, :error_unauthorized end end From 28d39f405bd56aa5bf45057e0d1e2810f5af3a1c Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 23 May 2024 09:04:35 +0200 Subject: [PATCH 24/28] Use positional arguments for user in viible methods --- app/controllers/projects/queries_controller.rb | 2 +- app/models/queries/projects/factory.rb | 2 +- app/models/queries/projects/project_query.rb | 4 ++-- spec/controllers/projects/queries_controller_spec.rb | 2 +- spec/models/queries/projects/factory_spec.rb | 2 +- spec/models/queries/projects/project_query_spec.rb | 10 +++++----- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/controllers/projects/queries_controller.rb b/app/controllers/projects/queries_controller.rb index 37377be694e0..bf9bbc36fd1c 100644 --- a/app/controllers/projects/queries_controller.rb +++ b/app/controllers/projects/queries_controller.rb @@ -112,6 +112,6 @@ def render_result(service_call, success_i18n_key:, error_i18n_key:) # rubocop:di end def find_query - @query = Queries::Projects::ProjectQuery.visible(user: current_user).find(params[:id]) + @query = Queries::Projects::ProjectQuery.visible(current_user).find(params[:id]) end end diff --git a/app/models/queries/projects/factory.rb b/app/models/queries/projects/factory.rb index 81c4b7b02f1a..bbc7dc19acaa 100644 --- a/app/models/queries/projects/factory.rb +++ b/app/models/queries/projects/factory.rb @@ -133,7 +133,7 @@ def find_static_query_and_set_attributes(id, params, user, duplicate:) end def find_persisted_query_and_set_attributes(id, params, user, duplicate:) - query = Queries::Projects::ProjectQuery.visible(user:).find_by(id:) + query = Queries::Projects::ProjectQuery.visible(user).find_by(id:) return unless query diff --git a/app/models/queries/projects/project_query.rb b/app/models/queries/projects/project_query.rb index 157ba36c2737..7bf65abb9ee2 100644 --- a/app/models/queries/projects/project_query.rb +++ b/app/models/queries/projects/project_query.rb @@ -39,11 +39,11 @@ class Queries::Projects::ProjectQuery < ApplicationRecord scope :public_lists, -> { where(public: true) } scope :private_lists, ->(user: User.current) { where(public: false, user:) } - scope :visible, ->(user: User.current) { + scope :visible, ->(user = User.current) { public_lists.or(private_lists(user:)) } - def visible?(user: User.current) + def visible?(user = User.current) public? || user == self.user end diff --git a/spec/controllers/projects/queries_controller_spec.rb b/spec/controllers/projects/queries_controller_spec.rb index 60dc3a371b86..529318d1751e 100644 --- a/spec/controllers/projects/queries_controller_spec.rb +++ b/spec/controllers/projects/queries_controller_spec.rb @@ -36,7 +36,7 @@ before do scope = instance_double(ActiveRecord::Relation) - allow(Queries::Projects::ProjectQuery).to receive(:visible).with(user:).and_return(scope) + allow(Queries::Projects::ProjectQuery).to receive(:visible).with(user).and_return(scope) allow(scope).to receive(:find).with(query.id.to_s).and_return(query) login_as user diff --git a/spec/models/queries/projects/factory_spec.rb b/spec/models/queries/projects/factory_spec.rb index 4d9fc2d5cab1..c4fc2fc8fa0e 100644 --- a/spec/models/queries/projects/factory_spec.rb +++ b/spec/models/queries/projects/factory_spec.rb @@ -36,7 +36,7 @@ allow(Queries::Projects::ProjectQuery) .to receive(:visible) - .with(user: current_user) + .with(current_user) .and_return(scope) allow(scope) diff --git a/spec/models/queries/projects/project_query_spec.rb b/spec/models/queries/projects/project_query_spec.rb index c2fc44a5d631..f86a17593d6a 100644 --- a/spec/models/queries/projects/project_query_spec.rb +++ b/spec/models/queries/projects/project_query_spec.rb @@ -416,8 +416,8 @@ private_query = create(:project_query, public: false) create(:project_query, public: false) - expect(described_class.visible(user: private_query.user)).to contain_exactly(public_query, public_query_other_user, - private_query) + expect(described_class.visible(private_query.user)).to contain_exactly(public_query, public_query_other_user, + private_query) end end end @@ -430,7 +430,7 @@ context "when the user is the owner" do let(:owner) { user } - it { is_expected.to be_visible(user:) } + it { is_expected.to be_visible(user) } end context "when the user is not the owner" do @@ -439,13 +439,13 @@ context "and the query is public" do let(:public) { true } - it { is_expected.to be_visible(user:) } + it { is_expected.to be_visible(user) } end context "and the query is private" do let(:public) { false } - it { is_expected.not_to be_visible(user:) } + it { is_expected.not_to be_visible(user) } end end end From 92a83274cd32187650fa0f432523295a3b234780 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 23 May 2024 17:01:33 +0200 Subject: [PATCH 25/28] Remove `can_rename?` method and alias it to can_save? --- app/components/projects/index_page_header_component.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/projects/index_page_header_component.rb b/app/components/projects/index_page_header_component.rb index d5fab8b4e365..4f6a8cc28b92 100644 --- a/app/components/projects/index_page_header_component.rb +++ b/app/components/projects/index_page_header_component.rb @@ -82,7 +82,7 @@ def can_save? end end - def can_rename? = may_save_as? && query.persisted? && query.user == current_user && !query.changed? + alias can_rename? can_save? def can_publish? OpenProject::FeatureDecisions.project_list_sharing_active? && From 3264517c50d40cb559165761bafb261f6d6320c8 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 23 May 2024 17:14:05 +0200 Subject: [PATCH 26/28] Fix specs --- spec/controllers/projects/queries_controller_spec.rb | 4 +++- spec/helpers/menus/projects_spec.rb | 12 ++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/spec/controllers/projects/queries_controller_spec.rb b/spec/controllers/projects/queries_controller_spec.rb index 529318d1751e..a1cce957c2b8 100644 --- a/spec/controllers/projects/queries_controller_spec.rb +++ b/spec/controllers/projects/queries_controller_spec.rb @@ -96,7 +96,9 @@ let(:query_id) { "42" } before do - allow(Queries::Projects::ProjectQuery).to receive(:find).with(query_id).and_return(query) + scope = instance_double(ActiveRecord::Relation) + allow(Queries::Projects::ProjectQuery).to receive(:visible).and_return(scope) + allow(scope).to receive(:find).with(query_id).and_return(query) login_as user end diff --git a/spec/helpers/menus/projects_spec.rb b/spec/helpers/menus/projects_spec.rb index 71a2cdfe40c2..594aee6aaed3 100644 --- a/spec/helpers/menus/projects_spec.rb +++ b/spec/helpers/menus/projects_spec.rb @@ -45,11 +45,15 @@ Queries::Projects::ProjectQuery.create!(name: "Other user query", user: build(:user)) end + shared_let(:public_query) do + Queries::Projects::ProjectQuery.create!(name: "Public query", user: build(:user), public: true) + end + subject(:first_level_menu_items) { instance.first_level_menu_items } - it "returns 3 menu groups" do + it "returns 4 menu groups" do expect(first_level_menu_items).to all(be_a(OpenProject::Menu::MenuGroup)) - expect(first_level_menu_items.length).to eq(3) + expect(first_level_menu_items.length).to eq(4) end describe "children items" do @@ -66,6 +70,10 @@ it "doesn't contain item for other user query" do expect(children_menu_items).not_to include(have_attributes(title: "Other user query")) end + + it "contains item for public query" do + expect(children_menu_items).to include(have_attributes(title: "Public query")) + end end describe "selected children items" do From 1ba1481d5679ceefe6ab87b2315216bbf0dfe006 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 23 May 2024 17:32:14 +0200 Subject: [PATCH 27/28] Reintroduce can_rename? --- .../projects/index_page_header_component.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/components/projects/index_page_header_component.rb b/app/components/projects/index_page_header_component.rb index 4f6a8cc28b92..5cf1928edbaa 100644 --- a/app/components/projects/index_page_header_component.rb +++ b/app/components/projects/index_page_header_component.rb @@ -82,7 +82,17 @@ def can_save? end end - alias can_rename? can_save? + def can_rename? + return false unless current_user.logged? + return false unless query.persisted? + return false if query.changed? + + if query.public? + current_user.allowed_globally?(:manage_public_project_queries) + else + query.user == current_user + end + end def can_publish? OpenProject::FeatureDecisions.project_list_sharing_active? && From f81c34b4e73ef0f21eb6216fda58822faa5718dc Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 27 May 2024 11:43:56 +0200 Subject: [PATCH 28/28] Add a TODO about removing the temporary menu items --- app/components/projects/index_page_header_component.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/components/projects/index_page_header_component.html.erb b/app/components/projects/index_page_header_component.html.erb index dff8c5e76b1c..6f99b4163acd 100644 --- a/app/components/projects/index_page_header_component.html.erb +++ b/app/components/projects/index_page_header_component.html.erb @@ -81,6 +81,7 @@ end if query.persisted? + # TODO: Remove section when the sharing modal is implemented (https://community.openproject.org/projects/openproject/work_packages/55163) if can_publish? if query.public? menu.with_item(