Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion COPYRIGHT
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,4 @@ 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.
Comment thread
NobodysNightmare marked this conversation as resolved.
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
2 changes: 1 addition & 1 deletion COPYRIGHT_short
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ 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.
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.

See COPYRIGHT and LICENSE files for more details.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#
# 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.
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module Queries
Comment thread
NobodysNightmare marked this conversation as resolved.
module Wikis
module PageLinks
module Filter
class ProviderFilter < Filters::Base
self.model = ::Wikis::PageLink
Comment thread
mereghost marked this conversation as resolved.

def type = :list

def human_name
::Wikis::PageLink.human_attribute_name(name)
Copy link
Copy Markdown
Contributor

@NobodysNightmare NobodysNightmare May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 🟡 Typo? undefined local variable or method 'name'?

Suggested change
::Wikis::PageLink.human_attribute_name(name)
::Wikis::PageLink.human_attribute_name(:name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this does not seem to be an error. There is a method called name defined and PageLink has no attribute called :name.

But then what is this? A page link has

  • linkable
  • provider(_id)
  • author(_id)
  • type
  • identifier

Is this ever going to be something else than :provider? Do we win anything by passing in name as a method here instead of :provider directly?

Suggested change
::Wikis::PageLink.human_attribute_name(name)
::Wikis::PageLink.human_attribute_name(:provider)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name comes from the filter base class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ever going to be something else than :provider? Do we win anything by passing in name as a method here instead of :provider directly?

Repeating this question. To me it still seems much clearer to just reference the attribute name that we are going to to translate. Right now it rather looks like a coincidence to me that the name is usable as an attribute name. But it's incredibly hard to track down, because name comes from whatever is passed in as key during create! of the filter.

This means it's theoretically possible to end up with a ProviderFilter that has a name different than :provider. It's probably unlikely etc. But that's why I don't understand why we'd couple these two things that seem unrelated to each other.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... at this point I'm not sure what you expect. human_name must tied obligatory to be defined.

Also #name is used by the underlying structure of query resolution to figure out which filter to instantiate and is tied to the class name, besides being a common idiom used on the codebase.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Editorial note: I started this response in a bad mood. Maybe because I felt unheard or at some point would have expected longer answers from your side as well. Not sure. I left my original thoughts in here uneditied under "Jan is mad", but read those with some grains of salt and pepper. (I leave them in because they still explain why I expect something different here).

Everything under "The constructive part" is nicer.

Jan is mad

Well... at this point I'm not sure what you expect.

I wrote that above: ::Wikis::PageLink.human_attribute_name(:provider).

human_name must tied obligatory to be defined.

What?

Also #name is used by the underlying structure of query resolution to figure out which filter to instantiate and is tied to the class name

None of this is visible when looking at ProviderFilter. I even looked pretty far down the call stack and that name will eventually resolve to provider is something that I was able to guess from context (because what else should the human name resolve from?), but not able to see from code.

I am surprised that my wish to understand code from looking at a single file seems to be an alien thing.

besides being a common idiom used on the codebase.

I get the feeling that I will only feel bad feelings when looking at idioms and practices around these filters...

The constructive part

You are right, that you are following mostly a common idiom. Actually a lot of implementations go as far as writing

  def human_name
    model.human_attribute_name(name)
  end

This is interesting, because we have exactly the same code working across a broad range of models. IMO this could be a refactoring that could be done to filters.

Imagine someone would change the base class implementation of human_name from

  def human_name
    raise SubclassResponsibilityError
  end

to

  def human_name
    model.human_attribute_name(name)
  end

This would mean that subclasses don't need to idiomatically implement something that they don't understand (subclasses don't seem to have other uses for name and they don't directly choose it). They only need to overwrite it, if they prefer a different human name, but there would be a sane default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad mood @NobodysNightmare is funny. XD

But yeah, I wasn't able to communicate, because I was in a bad mood for other reasons (IRL) that your complaint would be probably better addressed with a refactor of filters.

No harm done. <3

end

def allowed_values
::Wikis::Provider.enabled.pluck(:universal_identifier).map { |uid| [uid, uid] }
end

def left_outer_joins = :provider

def where
operator_strategy.sql_for_field(values, ::Wikis::Provider.table_name, "universal_identifier")
end
end
end
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module Queries
module Wikis
module PageLinks
class PageLinkQuery
include BaseQuery
include UnpersistedQuery

def self.model
@model ||= ::Wikis::PageLink
end

def default_scope
::Wikis::PageLink.includes(:provider).all
end
end
end
end
end
74 changes: 74 additions & 0 deletions modules/wikis/app/services/wikis/page_link_metadata_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module Wikis
class PageLinkMetadataService
# @param page_links [ActiveRecord::Relation<Wikis::PageLink>]
def initialize(page_links)
@result = ServiceResult.success(errors: ActiveModel::Errors.new(self))
@relation = page_links
end

# @return [ServiceResult<ActiveRecord::Relation<Wikis::PageLink>]
def call
metadata = relation.group_by(&:provider).flat_map do |provider, page_links|
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 I just realized that the group_by(&:provider) is not needed at all since we fetch results one by one anyways.

It would be useful if we were trying to obtain infos in bulk, but given that we are getting them one by one, we could as well get away without the grouping.

build_inputs(page_links).filter_map do |input_data|
provider.resolve("queries.page_info").call(input_data).value_or(nil)
end
end

@result.result = enrich_models(metadata)
@result
end

private

attr_reader :relation

def build_inputs(page_links)
page_links.filter_map do |page_link|
Adapters::Input::PageInfo.build(identifier: page_link.identifier).value_or(nil)
end
end

def enrich_models(metadata)
identifier_title_map = metadata.map { [it.identifier, it.title, it.provider.id] }
variable_placeholders = Array.new(identifier_title_map.size, "(?,?,?)").join(",")
join_string = <<~SQL.squish
LEFT JOIN (VALUES #{variable_placeholders}) AS metadata(identifier, title, provider_id)
ON metadata.identifier = wiki_page_links.identifier AND metadata.provider_id = wiki_page_links.provider_id
SQL

join_expression = ActiveRecord::Base.sanitize_sql_array([join_string, *identifier_title_map.flatten])

relation.joins(join_expression).select("wiki_page_links.*, metadata.title as title")
end
end
end
2 changes: 1 addition & 1 deletion modules/wikis/app/services/wikis/page_link_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#
# 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.
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++
Expand Down
2 changes: 2 additions & 0 deletions modules/wikis/config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
en:
activerecord:
attributes:
wikis/page_link:
provider: Wiki Provider
wikis/xwiki_provider:
authentication_method: Authentication method
authentication_methods:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module API
module V3
module PageLinks
class PageLinkCollectionRepresenter < Decorators::OffsetPaginatedCollection
def _type = "WikiPageLinkCollection"
end
end
end
end
17 changes: 15 additions & 2 deletions modules/wikis/lib/api/v3/page_links/page_link_representer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,22 @@
module API
module V3
module PageLinks
URN_INLINE_PAGE_LINK = "#{URN_PREFIX}wikiPageLinks:Inline".freeze
URN_RELATION_PAGE_LINK = "#{URN_PREFIX}wikiPageLinks:Relation".freeze

URN_PAGE_LINK_TYPE = {
"Wikis::RelationPageLink" => URN_RELATION_PAGE_LINK,
"Wikis::InlinePageLink" => URN_INLINE_PAGE_LINK
}.freeze

class PageLinkRepresenter < Decorators::Single
include Decorators::LinkedResource
include Decorators::DateProperty
include Caching::CachedRepresenter

property :id
property :identifier
property :wiki_page_link_type, exec_context: :decorator

date_time_property :created_at
date_time_property :updated_at
Expand All @@ -61,15 +70,19 @@ class PageLinkRepresenter < Decorators::Single
}
end

associated_resource :provider, v3_path: :wiki_provider
associated_resource :provider, v3_path: :wiki_provider, link: ->(*) {
{ href: api_v3_paths.wiki_provider(represented.provider.universal_identifier), title: represented.provider.name }
}

# TODO: Make this truly polymorphic - @mereghost 2026-04-13
associated_resource :linkable,
v3_path: :work_package,
representer: ::API::V3::WorkPackages::WorkPackageRepresenter,
skip_render: ->(*) { represented.linkable_id.nil? || represented.linkable_type != "WorkPackage" }

def _type = represented.class.name.demodulize
def _type = "WikiPageLink"

def wiki_page_link_type = URN_PAGE_LINK_TYPE[represented.class.name]

private

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# frozen_string_literal: true

#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) the OpenProject GmbH
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2013 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See COPYRIGHT and LICENSE files for more details.
#++

module API
module V3
module PageLinks
class WorkPackageWikiPageLinksAPI < OpenProjectAPI
helpers do
def enrich_models_with_wiki_metadata(relation)
Wikis::PageLinkMetadataService.new(relation).call
end
end

resources :wiki_page_links do
get do
query = ParamsToQueryService.new(
::Wikis::PageLink,
current_user,
query_class: ::Queries::Wikis::PageLinks::PageLinkQuery
).call(params)

unless query.valid?
message = I18n.t("api_v3.errors.missing_or_malformed_parameter", parameter: "filters")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The ParamsToQueryService deal with different parameters:

  • filters
  • sortBy
  • groupBy

How do we know that an invalid query is caused by filters? Couldn't it also be that a client tried to sort by something and messed the params for that up?

Shouldn't we be able to get from the query what was causing it to be invalid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, this is the pattern used around the code. =/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O.O

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked into this, because this comment still didn't receive any update. Queries do validate a bunch of attributes:

  • :filters
  • :orders
  • :group_by

Looking for usages of the api_v3.errors.missing_or_malformed_parameter translation, I agree to you, that it is only ever called with parameter: "filters". I can't tell you how I feel for all of this API code.

My current suspicion is that filters are the only thing that are changeable from the client side and order and group by come magically from inside OpenProject. While this is not at all visible when interacting with an interface like query.valid? it didn't seem to stop people from either using knowledge of this implementation detail or cargo culting their way around missing knowledge.

I am giving up here, feel free to resolve this discussion if you think that this cargo cult is idiomatic, or add a fix that bases the parameter variable on facts derivable from the validation errors.

raise ::API::Errors::InvalidQuery.new(message)
end

relation = query.results.where(linkable: @work_package)

PageLinkCollectionRepresenter.new(
enrich_models_with_wiki_metadata(relation).result,
Comment thread
NobodysNightmare marked this conversation as resolved.
per_page: params[:pageSize],
self_link: api_v3_paths.work_package_page_links(@work_package.id),
current_user:
)
end
end
end
end
end
end
4 changes: 2 additions & 2 deletions modules/wikis/lib/api/v3/providers/provider_representer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ class ProviderRepresenter < Decorators::Single
include Decorators::LinkedResource
include Decorators::DateProperty

property :id
property :universal_identifier
property :name

date_time_property :created_at
date_time_property :updated_at

self_link(path: :wiki_provider)
self_link(path: :wiki_provider, id_attribute: :universal_identifier)
end
end
end
Expand Down
Loading
Loading