diff --git a/Gemfile.lock b/Gemfile.lock index 1c9ae13e8dbc..db1e8d15fa95 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -579,8 +579,8 @@ GEM representable (~> 3.0) retriable (>= 2.0, < 4.a) rexml - google-apis-gmail_v1 (0.40.0) - google-apis-core (>= 0.14.0, < 2.a) + google-apis-gmail_v1 (0.41.0) + google-apis-core (>= 0.15.0, < 2.a) google-cloud-env (2.1.1) faraday (>= 1.0, < 3.a) googleauth (1.11.0) @@ -1134,7 +1134,7 @@ GEM activesupport faraday (~> 2.0) faraday-follow_redirects - webmock (3.23.0) + webmock (3.23.1) addressable (>= 2.8.0) crack (>= 0.3.2) hashdiff (>= 0.4.0, < 2.0.0) diff --git a/app/components/projects/index_page_header_component.html.erb b/app/components/projects/index_page_header_component.html.erb index 88189b02fea8..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,29 @@ 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( + 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 +115,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..5cf1928edbaa 100644 --- a/app/components/projects/index_page_header_component.rb +++ b/app/components/projects/index_page_header_component.rb @@ -70,9 +70,35 @@ 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? - def can_rename? = may_save_as? && query.persisted? && query.user == current_user && !query.changed? + if query.public? + current_user.allowed_globally?(:manage_public_project_queries) + else + query.user == current_user + end + end + + 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? && + current_user.allowed_globally?(:manage_public_project_queries) && + query.persisted? + end def show_state? state == :show 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..8d68f035dbc8 100644 --- a/app/components/projects/table_component.rb +++ b/app/components/projects/table_component.rb @@ -64,11 +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 - @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 end def initialize_sorted_model @@ -113,15 +112,14 @@ 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 + columns + end end def projects(query) @@ -156,7 +154,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? diff --git a/app/contracts/queries/projects/project_queries/base_contract.rb b/app/contracts/queries/projects/project_queries/base_contract.rb index 4cc8bcd14d29..da00abf3366a 100644 --- a/app/contracts/queries/projects/project_queries/base_contract.rb +++ b/app/contracts/queries/projects/project_queries/base_contract.rb @@ -41,17 +41,36 @@ def self.model presence: true, length: { maximum: 255 } - validate :user_is_current_user_and_logged_in 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_current_user_and_logged_in - unless user.logged? && user == model.user + def user_is_logged_in + unless user.logged? errors.add :base, :error_unauthorized end end + 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 + def name_select_included if model.selects.none? { |s| s.attribute == :name } errors.add :selects, :name_not_included diff --git a/app/contracts/queries/projects/project_queries/publish_contract.rb b/app/contracts/queries/projects/project_queries/publish_contract.rb new file mode 100644 index 000000000000..98a9fe2844c9 --- /dev/null +++ b/app/contracts/queries/projects/project_queries/publish_contract.rb @@ -0,0 +1,33 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Queries::Projects::ProjectQueries + class PublishContract < BaseContract + attribute :public + end +end diff --git a/app/controllers/admin/settings/api_settings_controller.rb b/app/controllers/admin/settings/api_settings_controller.rb index 249c390e3ce1..f7aae0d4d183 100644 --- a/app/controllers/admin/settings/api_settings_controller.rb +++ b/app/controllers/admin/settings/api_settings_controller.rb @@ -39,5 +39,11 @@ def settings_params settings["apiv3_cors_origins"] = settings["apiv3_cors_origins"].split(/\r?\n/) end end + + def extra_permitted_filters + # attachment_whitelist is normally permitted as an array parameter. + # Explicitly permit it as a string here. + [:apiv3_cors_origins] + end end end diff --git a/app/controllers/admin/settings/attachments_settings_controller.rb b/app/controllers/admin/settings/attachments_settings_controller.rb index 8ab3bb5e6f4b..7929473435ae 100644 --- a/app/controllers/admin/settings/attachments_settings_controller.rb +++ b/app/controllers/admin/settings/attachments_settings_controller.rb @@ -41,5 +41,11 @@ def settings_params settings["attachment_whitelist"] = settings["attachment_whitelist"].split(/\r?\n/) end end + + def extra_permitted_filters + # attachment_whitelist is normally permitted as an array parameter. + # Explicitly permit it as a string here. + [:attachment_whitelist] + end end end diff --git a/app/controllers/admin/settings_controller.rb b/app/controllers/admin/settings_controller.rb index 8d9823ced5a1..c7daac6c37d2 100644 --- a/app/controllers/admin/settings_controller.rb +++ b/app/controllers/admin/settings_controller.rb @@ -90,7 +90,17 @@ def find_plugin end def settings_params - permitted_params.settings.to_h + permitted_params.settings(*extra_permitted_filters).to_h + end + + # Override to allow additional permitted parameters. + # + # Useful when the format of the setting in the parameters is different from + # the expected format in the setting definition, for instance a setting is + # an array in the definition but is passed as a string to be split in the + # parameters. + def extra_permitted_filters + nil end def update_service diff --git a/app/controllers/projects/queries_controller.rb b/app/controllers/projects/queries_controller.rb index 273733778fe8..bf9bbc36fd1c 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] + 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", @@ -55,17 +59,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 +67,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,7 +95,23 @@ def destroy private + 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 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: modified_query, state: :edit } + end + end + def find_query - @query = Queries::Projects::ProjectQuery.find(params[:id]) + @query = Queries::Projects::ProjectQuery.visible(current_user).find(params[:id]) end 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/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/permitted_params.rb b/app/models/permitted_params.rb index 6ed1a45ae846..241c79b26db5 100644 --- a/app/models/permitted_params.rb +++ b/app/models/permitted_params.rb @@ -26,7 +26,7 @@ # See COPYRIGHT and LICENSE files for more details. #++ -require 'permitted_params/allowed_settings' +require "permitted_params/allowed_settings" class PermittedParams # This class intends to provide a method for all params hashes coming from the @@ -162,7 +162,7 @@ def query p = params.require(:query).permit(*self.class.permitted_attributes[:query]) p[:sort_criteria] = params .require(:query) - .permit(sort_criteria: { '0' => [], '1' => [], '2' => [] }) + .permit(sort_criteria: { "0" => [], "1" => [], "2" => [] }) p[:sort_criteria].delete :sort_criteria p end @@ -179,11 +179,8 @@ def status params.require(:status).permit(*self.class.permitted_attributes[:status]) end - def settings - permitted_params = params.require(:settings).permit - all_valid_keys = AllowedSettings.all - - permitted_params.merge(params[:settings].to_unsafe_hash.slice(*all_valid_keys)) + def settings(extra_permitted_filters = nil) + params.require(:settings).permit(*AllowedSettings.filters, *extra_permitted_filters) end def user(additional_params = []) @@ -340,7 +337,7 @@ def message(project = nil) end def attachments - params.permit(attachments: %i[file description id])['attachments'] + params.permit(attachments: %i[file description id])["attachments"] end def enumerations @@ -407,7 +404,7 @@ def custom_field_values(key = nil, required: true) # Reject blank values from include_hidden select fields values.each { |_, v| v.compact_blank! if v.is_a?(Array) } - values.empty? ? {} : { 'custom_field_values' => values.permit! } + values.empty? ? {} : { "custom_field_values" => values.permit! } end def permitted_attributes(key, additions = {}) @@ -529,7 +526,7 @@ def self.permitted_attributes :subject, Proc.new do |args| # avoid costly allowed_in_project? if the param is not there at all - if args[:params]['work_package']&.has_key?('watcher_user_ids') && + if args[:params]["work_package"]&.has_key?("watcher_user_ids") && args[:current_user].allowed_in_project?(:add_work_package_watchers, args[:project]) { watcher_user_ids: [] } diff --git a/app/models/permitted_params/allowed_settings.rb b/app/models/permitted_params/allowed_settings.rb index ef30aab777de..ce1e2b1de59d 100644 --- a/app/models/permitted_params/allowed_settings.rb +++ b/app/models/permitted_params/allowed_settings.rb @@ -29,6 +29,27 @@ def all keys end + def filters + restricted_keys = Set.new(self.restricted_keys) + Settings::Definition.all.flat_map do |key, definition| + next if restricted_keys.include?(key) + + case definition.format + when :hash + { key => {} } + when :array + { key => [] } + else + key + end + end + end + + def restricted_keys + restrictions.select(&:applicable?) + .flat_map(&:restricted_keys) + end + def add_restriction!(keys:, condition:) restrictions << Restriction.new(keys, condition) end diff --git a/app/models/queries/projects/factory.rb b/app/models/queries/projects/factory.rb index 818c7dd2594e..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.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 6a44a497199a..7bf65abb9ee2 100644 --- a/app/models/queries/projects/project_query.rb +++ b/app/models/queries/projects/project_query.rb @@ -36,6 +36,17 @@ class Queries::Projects::ProjectQuery < ApplicationRecord serialize :orders, coder: Queries::Serialization::Orders.new(self) 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 :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 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..e2c26fcda821 --- /dev/null +++ b/app/services/queries/projects/project_queries/publish_service.rb @@ -0,0 +1,49 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Queries::Projects::ProjectQueries + class PublishService < BaseServices::Update + private + + def after_validate(params, service_call) + model.public = params[:public] + + service_call + end + + def persist(service_call) + model.save + + service_call + end + + def default_contract_class + Queries::Projects::ProjectQueries::PublishContract + end + end +end diff --git a/app/workers/mails/reminder_job.rb b/app/workers/mails/reminder_job.rb index d110423c5a38..afcac61bac04 100644 --- a/app/workers/mails/reminder_job.rb +++ b/app/workers/mails/reminder_job.rb @@ -28,6 +28,12 @@ class Mails::ReminderJob < Mails::DeliverJob include ::Notifications::WithMarkedNotifications + include GoodJob::ActiveJobExtensions::Concurrency + + good_job_control_concurrency_with( + total_limit: 1, + key: -> { "#{self.class.name}-#{arguments.last}" } + ) private diff --git a/app/workers/notifications/create_date_alerts_notifications_job.rb b/app/workers/notifications/create_date_alerts_notifications_job.rb index c29d714b20e6..181c50286684 100644 --- a/app/workers/notifications/create_date_alerts_notifications_job.rb +++ b/app/workers/notifications/create_date_alerts_notifications_job.rb @@ -28,6 +28,13 @@ module Notifications class CreateDateAlertsNotificationsJob < ApplicationJob + include GoodJob::ActiveJobExtensions::Concurrency + + good_job_control_concurrency_with( + total_limit: 1, + key: -> { "#{self.class.name}-#{arguments.last}" } + ) + def perform(user) return unless EnterpriseToken.allows_to?(:date_alerts) diff --git a/app/workers/work_packages/progress/job.rb b/app/workers/work_packages/progress/job.rb index 6356bfe6c575..c4c1bb47222e 100644 --- a/app/workers/work_packages/progress/job.rb +++ b/app/workers/work_packages/progress/job.rb @@ -39,4 +39,8 @@ class WorkPackages::Progress::Job < ApplicationJob perform_limit: 1, key: -> { "WorkPackagesProgressJob" } ) + + retry_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError, + wait: 5.minutes, + attempts: :unlimited end diff --git a/config/constants/settings/definition.rb b/config/constants/settings/definition.rb index 2af6bc3b15f6..ee3dcab35f71 100644 --- a/config/constants/settings/definition.rb +++ b/config/constants/settings/definition.rb @@ -1087,7 +1087,7 @@ class Definition description: "Web worker count and threads configuration", default: { "workers" => 2, - "timeout" => 120, + "timeout" => Rails.env.production? ? 120 : 0, "wait_timeout" => 10, "min_threads" => 4, "max_threads" => 16 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 diff --git a/config/initializers/permissions.rb b/config/initializers/permissions.rb index 0f474ddcbb9d..c92ee82c1f11 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: :global, + 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..8c3d8b8f422b 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 { "#{self.class.name}-#{arguments.last[:storage].id}" } ) + retry_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError, + wait: 5.minutes, + attempts: 3 + discard_on ActiveJob::DeserializationError def perform(storage:) diff --git a/modules/storages/app/workers/storages/manage_storage_integrations_job.rb b/modules/storages/app/workers/storages/manage_storage_integrations_job.rb index d753e7911524..1588ab8b08ce 100644 --- a/modules/storages/app/workers/storages/manage_storage_integrations_job.rb +++ b/modules/storages/app/workers/storages/manage_storage_integrations_job.rb @@ -46,6 +46,11 @@ class ManageStorageIntegrationsJob < ApplicationJob enqueue_limit: 1, perform_limit: 1 ) + + retry_on GoodJob::ActiveJobExtensions::Concurrency::ConcurrencyExceededError, + wait: 5.minutes, + attempts: 3 + SINGLE_THREAD_DEBOUNCE_TIME = 4.seconds.freeze KEY = :manage_nextcloud_integration_job_debounce_happened_at CRON_JOB_KEY = :"Storages::ManageStorageIntegrationsJob" diff --git a/modules/storages/config/locales/crowdin/lt.yml b/modules/storages/config/locales/crowdin/lt.yml index c11fb68a88fc..ef69139d270e 100644 --- a/modules/storages/config/locales/crowdin/lt.yml +++ b/modules/storages/config/locales/crowdin/lt.yml @@ -49,7 +49,7 @@ lt: permission_manage_storages_in_project: Tvarkyti failų saugyklas projekte permission_read_files: 'Automatiškai valdomi projekto aplankai: Skaityti failus' permission_share_files: 'Automatiškai valdomi projekto aplankai: Bendrinti failus' - permission_share_files_explanation: Šis teisė galima tik Nextcloud saugykloms + permission_share_files_explanation: Ši teisė galima tik Nextcloud saugykloms permission_view_file_links: Žiūrėti failo nuorodas permission_write_files: 'Automatiškai valdomi projekto aplankai: Rašyti failus' project_module_storages: Failai diff --git a/spec/contracts/queries/projects/project_queries/shared_contract_examples.rb b/spec/contracts/queries/projects/project_queries/shared_contract_examples.rb index 6a3315276f10..a553cbdb5490 100644 --- a/spec/contracts/queries/projects/project_queries/shared_contract_examples.rb +++ b/spec/contracts/queries/projects/project_queries/shared_contract_examples.rb @@ -55,7 +55,51 @@ context "if the user is not the current user" do let(:query_user) { build_stubbed(:user) } - it_behaves_like "contract is invalid", base: :error_unauthorized + it_behaves_like "contract is invalid", base: :can_only_be_modified_by_owner + end + + context "if the list is public and the editing user has the permission" do + let(:query_user) { build_stubbed(:user) } + + before do + query.change_by_system do + query.public = true + end + + mock_permissions_for(current_user) do |mock| + mock.allow_globally :manage_public_project_queries + end + end + + it_behaves_like "contract is valid" + end + + context "if the list is public and the editing user does not have the permission" do + let(:query_user) { build_stubbed(:user) } + + before do + query.change_by_system do + query.public = true + end + + mock_permissions_for(current_user, &:forbid_everything) + end + + it_behaves_like "contract is invalid", base: :need_permission_to_modify_public_query + end + + context "if the list is public and the editing user does not have the permission, even if they are the owner" do + let(:query_user) { current_user } + + before do + query.change_by_system do + query.public = true + end + + mock_permissions_for(current_user, &:forbid_everything) + end + + it_behaves_like "contract is invalid", base: :need_permission_to_modify_public_query end context "if the user and the current user is anonymous" do diff --git a/spec/controllers/projects/queries_controller_spec.rb b/spec/controllers/projects/queries_controller_spec.rb index 83f1314bb148..a1cce957c2b8 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" @@ -39,7 +56,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 } @@ -79,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 @@ -110,7 +129,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 +200,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 +209,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) @@ -242,6 +263,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 } @@ -252,12 +419,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 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/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 diff --git a/spec/models/permitted_params_spec.rb b/spec/models/permitted_params_spec.rb index e19a6cae5c17..23d42e88fda7 100644 --- a/spec/models/permitted_params_spec.rb +++ b/spec/models/permitted_params_spec.rb @@ -26,13 +26,13 @@ # See COPYRIGHT and LICENSE files for more details. #++ -require 'spec_helper' +require "spec_helper" RSpec.describe PermittedParams do let(:user) { build_stubbed(:user) } let(:admin) { build_stubbed(:admin) } - shared_context 'prepare params comparison' do + shared_context "with prepare params comparison" do let(:params_key) { defined?(hash_key) ? hash_key : attribute } let(:params) do nested_params = @@ -52,182 +52,181 @@ ActionController::Parameters.new(ac_params) end - subject { PermittedParams.new(params, user).send(attribute).to_h } + subject { described_class.new(params, user).send(attribute).to_h } end - shared_examples_for 'allows params' do - include_context 'prepare params comparison' + shared_examples_for "allows params" do + include_context "with prepare params comparison" it do - expected = defined?(allowed_params) ? allowed_params : hash + expected = defined?(expected_allowed_params) ? expected_allowed_params : hash expect(subject).to eq(expected) end end - shared_examples_for 'allows nested params' do - include_context 'prepare params comparison' + shared_examples_for "allows nested params" do + include_context "with prepare params comparison" it { expect(subject).to eq(hash) } end - shared_examples_for 'forbids params' do - include_context 'prepare params comparison' + shared_examples_for "forbids params" do + include_context "with prepare params comparison" it { expect(subject).not_to eq(hash) } end - describe '#permit' do - it 'adds an attribute to be permitted later' do + describe "#permit" do + it "adds an attribute to be permitted later" do # just taking project_type here as an example, could be anything # taking the originally whitelisted params to be restored later - original_whitelisted = PermittedParams.instance_variable_get(:@whitelisted_params) + original_whitelisted = described_class.instance_variable_get(:@whitelisted_params) - params = ActionController::Parameters.new(project_type: { 'blubs1' => 'blubs' }) + ActionController::Parameters.new(project_type: { "blubs1" => "blubs" }) - PermittedParams.instance_variable_set(:@whitelisted_params, original_whitelisted) + described_class.instance_variable_set(:@whitelisted_params, original_whitelisted) end - it 'raises an argument error if key does not exist' do - expect { PermittedParams.permit(:bogus_key) }.to raise_error ArgumentError + it "raises an argument error if key does not exist" do + expect { described_class.permit(:bogus_key) }.to raise_error ArgumentError end end - describe '#pref' do + describe "#pref" do let(:attribute) { :pref } let(:hash) do acceptable_params = %w(hide_mail time_zone comments_sorting warn_on_leaving_unsaved) - acceptable_params.index_with { |_x| 'value' } + acceptable_params.index_with { |_x| "value" } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#news' do + describe "#news" do let(:attribute) { :news } let(:hash) do - %w(title summary description).index_with { |_x| 'value' }.to_h + %w(title summary description).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#comment' do + describe "#comment" do let(:attribute) { :comment } let(:hash) do - %w(commented author comments).index_with { |_x| 'value' }.to_h + %w(commented author comments).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#watcher' do + describe "#watcher" do let(:attribute) { :watcher } let(:hash) do - %w(watchable user user_id).index_with { |_x| 'value' }.to_h + %w(watchable user user_id).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#reply' do + describe "#reply" do let(:attribute) { :reply } let(:hash) do - %w(content subject).index_with { |_x| 'value' }.to_h + %w(content subject).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#wiki' do + describe "#wiki" do let(:attribute) { :wiki } let(:hash) do - %w(start_page).index_with { |_x| 'value' }.to_h + %w(start_page).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#membership' do + describe "#membership" do let(:attribute) { :membership } let(:hash) do - { 'project_id' => '1', 'role_ids' => ['1', '2', '4'] } + { "project_id" => "1", "role_ids" => ["1", "2", "4"] } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#category' do + describe "#category" do let(:attribute) { :category } let(:hash) do - %w(name assigned_to_id).index_with { |_x| 'value' }.to_h + %w(name assigned_to_id).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#version' do + describe "#version" do let(:attribute) { :version } - context 'whitelisted params' do + context "with whitelisted params" do let(:hash) do %w(name description effective_date due_date - start_date wiki_page_title status sharing).index_with { |_x| 'value' }.to_h + start_date wiki_page_title status sharing).index_with { |_x| "value" }.to_h end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'empty' do + context "when empty" do let(:hash) { {} } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'custom field values' do - let(:hash) { { 'custom_field_values' => { '1' => '5' } } } + context "for custom field values" do + let(:hash) { { "custom_field_values" => { "1" => "5" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end end - describe '#message' do + describe "#message" do let(:attribute) { :message } - context 'no instance passed' do - let(:allowed_params) do - %w(subject content forum_id).index_with { |_x| 'value' }.to_h + context "with no instance passed" do + let(:expected_allowed_params) do + %w(subject content forum_id).index_with { |_x| "value" }.to_h end let(:hash) do - allowed_params.merge(evil: 'true', sticky: 'true', locked: 'true') + expected_allowed_params.merge(evil: "true", sticky: "true", locked: "true") end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'empty' do + context "when empty" do let(:hash) { {} } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'with instance passed' do - let(:instance) { double('message', project: double('project')) } - let(:project) { double('project') } - let(:allowed_params) do - { 'subject' => 'value', - 'content' => 'value', - 'forum_id' => 'value', - 'sticky' => 'true', - 'locked' => 'true' } + context "with project instance passed" do + let(:project) { instance_double(Project) } + let(:expected_allowed_params) do + { "subject" => "value", + "content" => "value", + "forum_id" => "value", + "sticky" => "true", + "locked" => "true" } end let(:hash) do - ActionController::Parameters.new('message' => allowed_params.merge(evil: 'true')) + ActionController::Parameters.new("message" => expected_allowed_params.merge(evil: "true")) end before do @@ -236,215 +235,209 @@ end end - subject { PermittedParams.new(hash, user).message(project).to_h } + subject { described_class.new(hash, user).message(project).to_h } it do - expect(subject).to eq(allowed_params) + expect(subject).to eq(expected_allowed_params) end end end - describe '#attachments' do + describe "#attachments" do let(:attribute) { :attachments } let(:hash) do - { 'file' => 'myfile', - 'description' => 'mydescription' } + { "file" => "myfile", + "description" => "mydescription" } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#projects_type_ids' do + describe "#projects_type_ids" do let(:attribute) { :projects_type_ids } - let(:hash_key) { 'project' } + let(:hash_key) { "project" } let(:hash) do - { 'type_ids' => ['1', '', '2'] } + { "type_ids" => ["1", "", "2"] } end - let(:allowed_params) do + let(:expected_allowed_params) do [1, 2] end - include_context 'prepare params comparison' + include_context "with prepare params comparison" it do - actual = PermittedParams.new(params, user).send(attribute) + actual = described_class.new(params, user).send(attribute) - expect(actual).to eq(allowed_params) + expect(actual).to eq(expected_allowed_params) end end - describe '#color' do + describe "#color" do let(:attribute) { :color } let(:hash) do - { 'name' => 'blubs', - 'hexcode' => '#fff' } + { "name" => "blubs", + "hexcode" => "#fff" } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#color_move' do + describe "#color_move" do let(:attribute) { :color_move } - let(:hash_key) { 'color' } + let(:hash_key) { "color" } let(:hash) do - { 'move_to' => '1' } + { "move_to" => "1" } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#custom_field' do + describe "#custom_field" do let(:attribute) { :custom_field } let(:hash) do - { 'editable' => '0', 'visible' => '0' } + { "editable" => "0", "visible" => "0" } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe '#custom_action' do + describe "#custom_action" do let(:attribute) { :custom_action } let(:hash) do { - 'name' => 'blubs', - 'description' => 'blubs blubs', - 'actions' => { 'assigned_to' => '1' }, - 'conditions' => { 'status' => '42' }, - 'move_to' => 'lower' + "name" => "blubs", + "description" => "blubs blubs", + "actions" => { "assigned_to" => "1" }, + "conditions" => { "status" => "42" }, + "move_to" => "lower" } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end describe "#update_work_package" do let(:attribute) { :update_work_package } - let(:hash_key) { 'work_package' } + let(:hash_key) { "work_package" } - context 'subject' do - let(:hash) { { 'subject' => 'blubs' } } + describe "subject" do + let(:hash) { { "subject" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'description' do - let(:hash) { { 'description' => 'blubs' } } + describe "description" do + let(:hash) { { "description" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'start_date' do - let(:hash) { { 'start_date' => '2013-07-08' } } + describe "start_date" do + let(:hash) { { "start_date" => "2013-07-08" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'due_date' do - let(:hash) { { 'due_date' => '2013-07-08' } } + describe "due_date" do + let(:hash) { { "due_date" => "2013-07-08" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'assigned_to_id' do - let(:hash) { { 'assigned_to_id' => '1' } } + describe "assigned_to_id" do + let(:hash) { { "assigned_to_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'responsible_id' do - let(:hash) { { 'responsible_id' => '1' } } + describe "responsible_id" do + let(:hash) { { "responsible_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'type_id' do - let(:hash) { { 'type_id' => '1' } } + describe "type_id" do + let(:hash) { { "type_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'priority_id' do - let(:hash) { { 'priority_id' => '1' } } + describe "priority_id" do + let(:hash) { { "priority_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'parent_id' do - let(:hash) { { 'parent_id' => '1' } } + describe "parent_id" do + let(:hash) { { "parent_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'parent_id' do - let(:hash) { { 'parent_id' => '1' } } + describe "version_id" do + let(:hash) { { "version_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'version_id' do - let(:hash) { { 'version_id' => '1' } } + describe "estimated_hours" do + let(:hash) { { "estimated_hours" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'estimated_hours' do - let(:hash) { { 'estimated_hours' => '1' } } + describe "done_ratio" do + let(:hash) { { "done_ratio" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'done_ratio' do - let(:hash) { { 'done_ratio' => '1' } } + describe "lock_version" do + let(:hash) { { "lock_version" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'lock_version' do - let(:hash) { { 'lock_version' => '1' } } + describe "status_id" do + let(:hash) { { "status_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'status_id' do - let(:hash) { { 'status_id' => '1' } } + describe "category_id" do + let(:hash) { { "category_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'category_id' do - let(:hash) { { 'category_id' => '1' } } + describe "budget_id" do + let(:hash) { { "budget_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'budget_id' do - let(:hash) { { 'budget_id' => '1' } } + describe "notes" do + let(:hash) { { "journal_notes" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'notes' do - let(:hash) { { 'journal_notes' => 'blubs' } } + describe "attachments" do + let(:hash) { { "attachments" => [{ "file" => "djskfj", "description" => "desc" }] } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'attachments' do - let(:hash) { { 'attachments' => [{ 'file' => 'djskfj', 'description' => 'desc' }] } } - - it_behaves_like 'allows params' - end - - context 'watcher_user_ids' do - include_context 'prepare params comparison' - let(:hash) { { 'watcher_user_ids' => ['1', '2'] } } - let(:project) { double('project') } + describe "watcher_user_ids" do + include_context "with prepare params comparison" + let(:hash) { { "watcher_user_ids" => ["1", "2"] } } + let(:project) { instance_double(Project) } before do mock_permissions_for(user) do |mock| @@ -452,9 +445,9 @@ end end - subject { PermittedParams.new(params, user).update_work_package(project:).to_h } + subject { described_class.new(params, user).update_work_package(project:).to_h } - context 'user is allowed to add watchers' do + context "when user is allowed to add watchers" do before do mock_permissions_for(user) do |mock| mock.allow_in_project :add_work_package_watchers, project: @@ -466,7 +459,7 @@ end end - context 'user is not allowed to add watchers' do + context "when user is not allowed to add watchers" do before do mock_permissions_for(user, &:forbid_everything) end @@ -477,20 +470,20 @@ end end - context 'custom field values' do - let(:hash) { { 'custom_field_values' => { '1' => '5' } } } + context "for custom field values" do + let(:hash) { { "custom_field_values" => { "1" => "5" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context "removes custom field values that do not follow the schema 'id as string' => 'value as string'" do - let(:hash) { { 'custom_field_values' => { 'blubs' => '5', '5' => { '1' => '2' } } } } + describe "removes custom field values that do not follow the schema 'id as string' => 'value as string'" do + let(:hash) { { "custom_field_values" => { "blubs" => "5", "5" => { "1" => "2" } } } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end - describe '#time_entry_activities_project' do + describe "#time_entry_activities_project" do let(:attribute) { :time_entry_activities_project } let(:hash) do [ @@ -498,98 +491,101 @@ { "activity_id" => "6", "active" => "1" } ] end - let(:allowed_params) do - [{ "activity_id" => "5", "active" => "0" }, { "activity_id" => "6", "active" => "1" }] + let(:expected_allowed_params) do + [ + ActionController::Parameters.new("activity_id" => "5", "active" => "0").permit!, + ActionController::Parameters.new("activity_id" => "6", "active" => "1").permit! + ] end - it_behaves_like 'allows params' do - subject { PermittedParams.new(params, user).send(attribute) } + it_behaves_like "allows params" do + subject { described_class.new(params, user).send(attribute) } end end - describe '#user' do - include_context 'prepare params comparison' + describe "#user" do + include_context "with prepare params comparison" - let(:hash_key) { 'user' } + let(:hash_key) { "user" } let(:external_authentication) { false } let(:change_password_allowed) { true } - subject { PermittedParams.new(params, user).send(attribute, external_authentication, change_password_allowed).to_h } + subject { described_class.new(params, user).send(attribute, external_authentication, change_password_allowed).to_h } - all_permissions = ['admin', - 'login', - 'firstname', - 'lastname', - 'mail', - 'language', - 'custom_fields', - 'ldap_auth_source_id', - 'force_password_change'] + all_permissions = ["admin", + "login", + "firstname", + "lastname", + "mail", + "language", + "custom_fields", + "ldap_auth_source_id", + "force_password_change"] - describe :user_create_as_admin do + describe "#user_create_as_admin" do let(:attribute) { :user_create_as_admin } let(:default_permissions) { %w[custom_fields firstname lastname language mail ldap_auth_source_id] } - context 'for a non-admin' do + context "for a non-admin" do let(:hash) { all_permissions.zip(all_permissions).to_h } - it 'permits default permissions' do + it "permits default permissions" do expect(subject.keys).to match_array(default_permissions) end end - context 'for a non-admin with global :create_user permission' do + context "for a non-admin with global :create_user permission" do let(:user) { create(:user, global_permissions: [:create_user]) } let(:hash) { all_permissions.zip(all_permissions).to_h } it 'permits default permissions and "login"' do - expect(subject.keys).to match_array(default_permissions + ['login']) + expect(subject.keys).to match_array(default_permissions + ["login"]) end end - context 'for an admin' do + context "for an admin" do let(:user) { admin } all_permissions.each do |field| context field do - let(:hash) { { field => 'test' } } + let(:hash) { { field => "test" } } it "permits #{field}" do - expect(subject).to eq(field => 'test') + expect(subject).to eq(field => "test") end end end - context 'with no password change allowed' do - let(:hash) { { 'force_password_change' => 'true' } } + context "with no password change allowed" do + let(:hash) { { "force_password_change" => "true" } } let(:change_password_allowed) { false } - it 'does not permit force_password_change' do + it "does not permit force_password_change" do expect(subject).to eq({}) end end - context 'with external authentication' do - let(:hash) { { 'ldap_auth_source_id' => 'true' } } + context "with external authentication" do + let(:hash) { { "ldap_auth_source_id" => "true" } } let(:external_authentication) { true } - it 'does not permit ldap_auth_source_id' do + it "does not permit ldap_auth_source_id" do expect(subject).to eq({}) end end - context 'custom field values' do - let(:hash) { { 'custom_field_values' => { '1' => '5' } } } + context "for custom field values" do + let(:hash) { { "custom_field_values" => { "1" => "5" } } } - it 'permits custom_field_values' do + it "permits custom_field_values" do expect(subject).to eq(hash) end end - context "custom field values that do not follow the schema 'id as string' => 'value as string'" do - let(:hash) { { 'custom_field_values' => { 'blubs' => '5', '5' => { '1' => '2' } } } } + context "for custom field values that do not follow the schema 'id as string' => 'value as string'" do + let(:hash) { { "custom_field_values" => { "blubs" => "5", "5" => { "1" => "2" } } } } - it 'are removed' do + it "are removed" do expect(subject).to eq({}) end end @@ -597,134 +593,134 @@ end user_permissions = [ - 'firstname', - 'lastname', - 'mail', - 'language', - 'custom_fields' + "firstname", + "lastname", + "mail", + "language", + "custom_fields" ] - describe '#user' do + describe "#user" do let(:attribute) { :user } let(:user) { admin } user_permissions.each do |field| context field do - let(:hash) { { field => 'test' } } + let(:hash) { { field => "test" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end end (all_permissions - user_permissions).each do |field| - context "#{field} (admin-only)" do - let(:hash) { { field => 'test' } } + context "for #{field} (admin-only)" do + let(:hash) { { field => "test" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end - context 'custom field values' do - let(:hash) { { 'custom_field_values' => { '1' => '5' } } } + context "for custom field values" do + let(:hash) { { "custom_field_values" => { "1" => "5" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context "custom field values that do not follow the schema 'id as string' => 'value as string'" do - let(:hash) { { 'custom_field_values' => { 'blubs' => '5', '5' => { '1' => '2' } } } } + context "for custom field values that do not follow the schema 'id as string' => 'value as string'" do + let(:hash) { { "custom_field_values" => { "blubs" => "5", "5" => { "1" => "2" } } } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end - context 'identity_url' do - let(:hash) { { 'identity_url' => 'test_identity_url' } } + context "for identity_url" do + let(:hash) { { "identity_url" => "test_identity_url" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end end - describe '#user_register_via_omniauth' do + describe "#user_register_via_omniauth" do let(:attribute) { :user_register_via_omniauth } - let(:hash_key) { 'user' } + let(:hash_key) { "user" } user_permissions = %w(login firstname lastname mail language) user_permissions.each do |field| - let(:hash) { { field => 'test' } } + let(:hash) { { field => "test" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'identity_url' do - let(:hash) { { 'identity_url' => 'test_identity_url' } } + context "for identity_url" do + let(:hash) { { "identity_url" => "test_identity_url" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end - shared_examples_for 'allows enumeration move params' do - let(:hash) { { '2' => { 'move_to' => 'lower' } } } + shared_examples_for "allows enumeration move params" do + let(:hash) { { "2" => { "move_to" => "lower" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - shared_examples_for 'allows move params' do - let(:hash) { { 'move_to' => 'lower' } } + shared_examples_for "allows move params" do + let(:hash) { { "move_to" => "lower" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - shared_examples_for 'allows custom fields' do - describe 'valid custom fields' do - let(:hash) { { '1' => { 'custom_field_values' => { '1' => '5' } } } } + shared_examples_for "allows custom fields" do + describe "valid custom fields" do + let(:hash) { { "1" => { "custom_field_values" => { "1" => "5" } } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'invalid custom fields' do - let(:hash) { { 'custom_field_values' => { 'blubs' => '5', '5' => { '1' => '2' } } } } + describe "invalid custom fields" do + let(:hash) { { "custom_field_values" => { "blubs" => "5", "5" => { "1" => "2" } } } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end - describe '#status' do + describe "#status" do let (:attribute) { :status } - describe 'name' do - let(:hash) { { 'name' => 'blubs' } } + describe "name" do + let(:hash) { { "name" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'default_done_ratio' do - let(:hash) { { 'default_done_ratio' => '10' } } + describe "default_done_ratio" do + let(:hash) { { "default_done_ratio" => "10" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'is_closed' do - let(:hash) { { 'is_closed' => 'true' } } + describe "is_closed" do + let(:hash) { { "is_closed" => "true" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'is_default' do - let(:hash) { { 'is_default' => 'true' } } + describe "is_default" do + let(:hash) { { "is_default" => "true" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'move_to' do - it_behaves_like 'allows move params' + describe "move_to" do + it_behaves_like "allows move params" end end - describe '#settings' do + describe "#settings" do let (:attribute) { :settings } - describe 'with password login enabled' do + describe "with password login enabled" do before do allow(OpenProject::Configuration) .to receive(:disable_password_login?) @@ -733,19 +729,19 @@ let(:hash) do { - 'sendmail_arguments' => 'value', - 'brute_force_block_after_failed_logins' => 'value', - 'password_active_rules' => ['value'], - 'default_projects_modules' => ['value', 'value'], - 'emails_footer' => { 'en' => 'value' } + "sendmail_arguments" => "value", + "brute_force_block_after_failed_logins" => "value", + "password_active_rules" => ["value"], + "default_projects_modules" => ["value", "value"], + "emails_footer" => { "en" => "value" } } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'with password login disabled' do - include_context 'prepare params comparison' + describe "with password login disabled" do + include_context "with prepare params comparison" before do allow(OpenProject::Configuration) @@ -755,27 +751,27 @@ let(:hash) do { - 'sendmail_arguments' => 'value', - 'brute_force_block_after_failed_logins' => 'value', - 'password_active_rules' => ['value'], - 'default_projects_modules' => ['value', 'value'], - 'emails_footer' => { 'en' => 'value' } + "sendmail_arguments" => "value", + "brute_force_block_after_failed_logins" => "value", + "password_active_rules" => ["value"], + "default_projects_modules" => ["value", "value"], + "emails_footer" => { "en" => "value" } } end let(:permitted_hash) do { - 'sendmail_arguments' => 'value', - 'brute_force_block_after_failed_logins' => 'value', - 'default_projects_modules' => ['value', 'value'], - 'emails_footer' => { 'en' => 'value' } + "sendmail_arguments" => "value", + "brute_force_block_after_failed_logins" => "value", + "default_projects_modules" => ["value", "value"], + "emails_footer" => { "en" => "value" } } end it { expect(subject).to eq(permitted_hash) } end - describe 'with writable registration footer' do + describe "with writable registration footer" do before do allow(Setting) .to receive(:registration_footer_writable?) @@ -784,17 +780,17 @@ let(:hash) do { - 'registration_footer' => { - 'en' => 'some footer' + "registration_footer" => { + "en" => "some footer" } } end - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'with a non-writable registration footer (set via env var or config file)' do - include_context 'prepare params comparison' + describe "with a non-writable registration footer (set via env var or config file)" do + include_context "with prepare params comparison" before do allow(Setting) @@ -804,8 +800,8 @@ let(:hash) do { - 'registration_footer' => { - 'en' => 'some footer' + "registration_footer" => { + "en" => "some footer" } } end @@ -816,175 +812,215 @@ it { expect(subject).to eq(expected_permitted_hash) } end + + context "when fetching settings" do + include_context "with prepare params comparison" + + let(:hash) do + { + "registration_footer" => { + "en" => "some footer" + }, + "working_days" => ["", "1", "2", "3", "4", "5"] + } + end + + def recording_notifications_for(notification) + events = [] + subscription = ActiveSupport::Notifications.subscribe notification do |*args| + events << ActiveSupport::Notifications::Event.new(*args) + end + + begin + yield + ensure + ActiveSupport::Notifications.unsubscribe(subscription) + end + + events + end + + it "does not log any 'unpermitted' message" do + events = recording_notifications_for(/unpermitted_parameters/) do + subject + end + expect(events).to be_empty + end + end end - describe '#enumerations' do + describe "#enumerations" do let (:attribute) { :enumerations } - describe 'name' do - let(:hash) { { '1' => { 'name' => 'blubs' } } } + describe "name" do + let(:hash) { { "1" => { "name" => "blubs" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'active' do - let(:hash) { { '1' => { 'active' => 'true' } } } + describe "active" do + let(:hash) { { "1" => { "active" => "true" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'is_default' do - let(:hash) { { '1' => { 'is_default' => 'true' } } } + describe "is_default" do + let(:hash) { { "1" => { "is_default" => "true" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'reassign_to_id' do - let(:hash) { { '1' => { 'reassign_to_id' => '1' } } } + describe "reassign_to_id" do + let(:hash) { { "1" => { "reassign_to_id" => "1" } } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'move_to' do - it_behaves_like 'allows enumeration move params' + describe "move_to" do + it_behaves_like "allows enumeration move params" end - describe 'custom fields' do - it_behaves_like 'allows custom fields' + describe "custom fields" do + it_behaves_like "allows custom fields" end end - describe '#wiki_page_rename' do + describe "#wiki_page_rename" do let(:hash_key) { :page } let (:attribute) { :wiki_page_rename } - describe 'title' do - let(:hash) { { 'title' => 'blubs' } } + describe "title" do + let(:hash) { { "title" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'redirect_existing_links' do - let(:hash) { { 'redirect_existing_links' => '1' } } + describe "redirect_existing_links" do + let(:hash) { { "redirect_existing_links" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end end - describe '#wiki_page' do + describe "#wiki_page" do let(:hash_key) { :page } let (:attribute) { :wiki_page } - describe 'title' do - let(:hash) { { 'title' => 'blubs' } } + describe "title" do + let(:hash) { { "title" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'parent_id' do - let(:hash) { { 'parent_id' => '1' } } + describe "parent_id" do + let(:hash) { { "parent_id" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'redirect_existing_links' do - let(:hash) { { 'redirect_existing_links' => '1' } } + describe "redirect_existing_links" do + let(:hash) { { "redirect_existing_links" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'journal_notes' do - let(:hash) { { 'journal_notes' => 'blubs' } } + describe "journal_notes" do + let(:hash) { { "journal_notes" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'text' do - let(:hash) { { 'text' => 'blubs' } } + describe "text" do + let(:hash) { { "text" => "blubs" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'lock_version' do - let(:hash) { { 'lock_version' => '1' } } + describe "lock_version" do + let(:hash) { { "lock_version" => "1" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end end - describe 'member' do + describe "member" do let (:attribute) { :member } - describe 'role_ids' do - let(:hash) { { 'role_ids' => [] } } + describe "role_ids" do + let(:hash) { { "role_ids" => [] } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - describe 'user_id' do - let(:hash) { { 'user_id' => 'blubs' } } + describe "user_id" do + let(:hash) { { "user_id" => "blubs" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end - describe 'project_id' do - let(:hash) { { 'project_id' => 'blubs' } } + describe "project_id" do + let(:hash) { { "project_id" => "blubs" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end - describe 'created_at' do - let(:hash) { { 'created_at' => 'blubs' } } + describe "created_at" do + let(:hash) { { "created_at" => "blubs" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end - describe '.add_permitted_attributes' do + describe ".add_permitted_attributes" do before do - @original_permitted_attributes = PermittedParams.permitted_attributes.clone + @original_permitted_attributes = described_class.permitted_attributes.clone end after do # Class variable is not accessible within class_eval original_permitted_attributes = @original_permitted_attributes - PermittedParams.class_eval do + described_class.class_eval do @whitelisted_params = original_permitted_attributes end end - describe 'with a known key' do + describe "with a known key" do let(:attribute) { :user } before do - PermittedParams.send(:add_permitted_attributes, user: [:a_test_field]) + described_class.send(:add_permitted_attributes, user: [:a_test_field]) end - context 'with an allowed parameter' do - let(:hash) { { 'a_test_field' => 'a test value' } } + context "with an allowed parameter" do + let(:hash) { { "a_test_field" => "a test value" } } - it_behaves_like 'allows params' + it_behaves_like "allows params" end - context 'with a disallowed parameter' do - let(:hash) { { 'a_not_allowed_field' => 'a test value' } } + context "with a disallowed parameter" do + let(:hash) { { "a_not_allowed_field" => "a test value" } } - it_behaves_like 'forbids params' + it_behaves_like "forbids params" end end - describe 'with an unknown key' do + describe "with an unknown key" do let(:attribute) { :unknown_key } - let(:hash) { { 'a_test_field' => 'a test value' } } + let(:hash) { { "a_test_field" => "a test value" } } before do - expect(Rails.logger).not_to receive(:warn) - PermittedParams.send(:add_permitted_attributes, unknown_key: [:a_test_field]) + allow(Rails.logger).to receive(:warn) + described_class.send(:add_permitted_attributes, unknown_key: [:a_test_field]) + end + + it "permitted attributes should include the key" do + expect(described_class.permitted_attributes.keys).to include(:unknown_key) end - it 'permitted attributes should include the key' do - expect(PermittedParams.permitted_attributes.keys).to include(:unknown_key) + it "does not log any warnings" do + described_class.permitted_attributes.keys + expect(Rails.logger).not_to have_received(:warn) end end end diff --git a/spec/models/queries/projects/factory_spec.rb b/spec/models/queries/projects/factory_spec.rb index 53a50b4f973a..c4fc2fc8fa0e 100644 --- a/spec/models/queries/projects/factory_spec.rb +++ b/spec/models/queries/projects/factory_spec.rb @@ -31,12 +31,12 @@ 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) - .with(user: current_user) + .to receive(:visible) + .with(current_user) .and_return(scope) allow(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) diff --git a/spec/models/queries/projects/project_query_spec.rb b/spec/models/queries/projects/project_query_spec.rb index 61c9d1565c2b..d095a0349dff 100644 --- a/spec/models/queries/projects/project_query_spec.rb +++ b/spec/models/queries/projects/project_query_spec.rb @@ -387,4 +387,61 @@ end end end + + describe "scopes" do + shared_let(:public_query) { create(:project_query, user:, public: true) } + shared_let(:public_query_other_user) { create(:project_query, public: true) } + shared_let(:private_query) { create(:project_query, user:) } + shared_let(:private_query_other_user) { create(:project_query) } + + describe ".public_lists" do + it "returns only public lists" do + 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 + expect(described_class.private_lists(user:)).to contain_exactly(private_query) + end + end + + describe ".visible" do + it "returns public and private queries owned by the user" do + expect(described_class.visible(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