Implements the GET /work_packages/:id/wiki_page_links endpoint

[mereghost]

This PR covers work packages: OP#73839, OP#73865 and OP#73866

What?

For the endpoint to be fully introduced a couple of new objects needed to be added: PageLinksQuery, ProviderFilters and a way to enrich the Models with some metadata, represented by the PageLinksMetadataService.

How?

It follows the same patterns we used in storages, having services isolating the API and the same processes to build the data.

What is missing?

1. An actual query to get the page titles
2. Some more error handling once this this available
3. Some assumptions where made, such that the commands would return a Result monad.
4. Some tests run on all pretend behaviour

[NobodysNightmare]

This PR looks incomplete. I see (partial) references to a query that doesn't exist.

Maybe it just needs to be committed? Or it was intended for a follow-up PR?

[NobodysNightmare]

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

Suggested change

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

[NobodysNightmare]

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)

[mereghost]

Name comes from the filter base class.

[NobodysNightmare]

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.

[mereghost]

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.

[NobodysNightmare]

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.

[mereghost]

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

[NobodysNightmare]

🟡 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?

[mereghost]

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

[NobodysNightmare]

O.O

[NobodysNightmare]

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.

[NobodysNightmare]

🟡 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.