-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Implements the GET /work_packages/:id/wiki_page_links endpoint #22834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
55c04a1
a2b5fe5
feed45d
86d12cb
6ca9a27
9eecbef
b1e88b7
ac872fa
0396731
5867685
836e0bf
0767362
e48a24a
94b094e
1f42c3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||||||||||
|
NobodysNightmare marked this conversation as resolved.
|
||||||||||
| module Wikis | ||||||||||
| module PageLinks | ||||||||||
| module Filter | ||||||||||
| class ProviderFilter < Filters::Base | ||||||||||
| self.model = ::Wikis::PageLink | ||||||||||
|
mereghost marked this conversation as resolved.
|
||||||||||
|
|
||||||||||
| def type = :list | ||||||||||
|
|
||||||||||
| def human_name | ||||||||||
| ::Wikis::PageLink.human_attribute_name(name) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 But then what is this? A page link has
Is this ever going to be something else than
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name comes from the filter base class.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 This means it's theoretically possible to end up with a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well... at this point I'm not sure what you expect. Also
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
I wrote that above:
What?
None of this is visible when looking at I am surprised that my wish to understand code from looking at a single file seems to be an alien thing.
I get the feeling that I will only feel bad feelings when looking at idioms and practices around these filters... The constructive partYou 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)
endThis 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 def human_name
raise SubclassResponsibilityError
endto def human_name
model.human_attribute_name(name)
endThis 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
| 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| | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 I just realized that the 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 | ||
| 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 |
| 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") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The
How do we know that an invalid query is caused by Shouldn't we be able to get from the query what was causing it to be invalid?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, this is the pattern used around the code. =/
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. O.O
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Looking for usages of the My current suspicion is that 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 |
||
| raise ::API::Errors::InvalidQuery.new(message) | ||
| end | ||
|
|
||
| relation = query.results.where(linkable: @work_package) | ||
|
|
||
| PageLinkCollectionRepresenter.new( | ||
| enrich_models_with_wiki_metadata(relation).result, | ||
|
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 | ||
Uh oh!
There was an error while loading. Please reload this page.