plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
460 stars 624 forks source link

Fix for getting a vocabulary that depends on the context #3216

Open oggers opened 2 years ago

oggers commented 2 years ago

When Volto gets a vocabulary it uses the action getVocabulary, but this action gets the vocabulary from Plone without taking into account the context. This is the part where the path is defined (action/vocabularies/vocabularies.js):

  return {
    type: GET_VOCABULARY,
    vocabulary: vocabNameOrURL,
    start,
    request: {
      op: 'get',
      path: `/@vocabularies/${vocabulary}?${queryString}`,
    },
    subrequest,
  };

Now look at this vocabulary:

@provider(IVocabularyFactory)
def my_vocabulary(context):
    return SimpleVocabulary.fromValues(aq_parent(context).my_categories)

When Volto requests this vocabulary the context is always the SiteRoot using this URL: http://localhost:3000/++api++/@vocabularies/my_vocabulary but the contents of the vocabulary depends on the context.

In order to fix this I came up with this simple patch:

  const vocabPath = flattenToAppURL(vocabNameOrURL);
  return {
    type: GET_VOCABULARY,
    vocabulary: vocabNameOrURL,
    start,
    request: {
      op: 'get',
      path: `${vocabPath}?${queryString}`,
    },
    subrequest,
  };

Now Volto uses this URL: http://localhost:3000/++api++/<context>/@vocabularies/my_vocabulary

As I am new to Volto, I don't know if this is the correct way, maybe someone can help me.

Either way, I consider this a bug and I think it needs to be addressed.

oggers commented 2 years ago

Just improved the patch:

  // Use complete URL to provide correct context
  const vocabPath = isUrl(vocabNameOrURL)
    ? flattenToAppURL(vocabNameOrURL)
    : `/@vocabularies/${vocabulary}`;

  return {
    type: GET_VOCABULARY,
    vocabulary: vocabNameOrURL,
    start,
    request: {
      op: 'get',
      path: `${vocabPath}?${queryString}`,
    },
    subrequest,
  };
mbarde commented 1 year ago

Thanks for the patch! Had the same problem and didnt find a solution without patching it.

davisagli commented 1 year ago

@plone/volto-team My thoughts on contextual vocabularies:

There are some situations where it is important to fetch the vocabulary in the correct context. But, we only want to do that when necessary, because it is harder to cache.

Volto already has access to the full contextual URL from the types service. But the getVocabulary action ignores it and always fetches the vocabulary at the site root. The challenge is how to make a smart decision about when to use the URL that includes the full context.

I propose that we add a volto setting with a list of vocabulary names that should be fetched in the right context, and make the getVocabulary action use it. This has the advantage that it's fully backwards compatible (no change unless you modify the setting). The downside is that it requires manual configuration for each contextual vocabulary, but I think that's okay since it's a 20% use case.

(I looked at whether we could provide a hint from the API about which vocabularies need to be fetched contextually, based on information available on the backend. This is difficult because we only have the vocabulary name available on the schema fields...we can't add an additional attribute on the actual vocabulary, because then the vocabulary has to be fully instantiated while building the response for the @types endpoint, which is currently not necessary. We could possibly implement a contextual_vocabulary schema directive, and then serialize that in the API and use it as a hint in volto...)

Thoughts?

tiberiuichim commented 1 year ago

But, we only want to do that when necessary, because it is harder to cache.

I think cache is hard to control for vocabularies, specially if they are role-based. It's easier to implement that cache directly in the Plone backend, so I'd be fine with having vocabularies always context based.

pnicolli commented 1 year ago

I propose that we add a volto setting with a list of vocabulary names that should be fetched in the right context, and make the getVocabulary action use it. This has the advantage that it's fully backwards compatible (no change unless you modify the setting). The downside is that it requires manual configuration for each contextual vocabulary, but I think that's okay since it's a 20% use case.

What if we just let the developer decide? We add a parameter to getVocabulary and we decide it based on where it is used, instead of adding more configuration. This might be worse, I don't know a lot of situations where contextual vocabularies are needed, so I am just putting another idea out there.

tiberiuichim commented 1 year ago

We add a parameter to getVocabulary and we decide it based on where it is used, instead of adding more configuration.

Sounds good, not sure how this is exposed from the restapi type schema, but it needs to also be supported from there.

erral commented 1 year ago

We add a parameter to getVocabulary and we decide it based on where it is used, instead of adding more configuration.

Sounds good, not sure how this is exposed from the restapi type schema, but it needs to also be supported from there.

I also agree.

I would say that we can't do this just on the Volto side. The contextuality of the vocabulary should be handled by the back-end.

Volto is just a cosumer of the Plone REST API (although the main one), so the API should inform about the vocabulary being contextual or not.

The point is how to do it in the backend? AFAIK when defining the vocabulary in ZCA there is no way to tie it to a context...

sneridagh commented 1 year ago

What's the problem to go full with context always? We did the same for the @types and it was not that painful. Probably I'm missing something.

Also, it would solve the use case of the IContextSourceBinders ones or whatever they are called, which AFAIK they do not work yet in p.restapi... (maybe I'm outdated on this one).

tiberiuichim commented 1 year ago

The contextuality of the vocabulary should be handled by the back-end.

Then the context has to be known all the time.

davisagli commented 1 year ago

pnicolli: What if we just let the developer decide? We add a parameter to getVocabulary and we decide it based on where it is used, instead of adding more configuration.

@pnicolli We could certainly add that parameter, but it's not that helpful if we want to pass it differently based on which vocabulary is requested. Often the action is called from quite generic code like ArrayWidget and it would be annoying to customize a widget just to request some vocabularies differently.

erral: Volto is just a cosumer of the Plone REST API (although the main one), so the API should inform about the vocabulary being contextual or not.

You could argue that it already does: It always provides a vocabulary URL with full context. If we make volto always fetch the URL that was provided by the backend, then we can optimize some vocabularies to always use a site-root or nav-root URL at the API level.

tiberiuichiim: I'd be fine with having vocabularies always context based.

sneridagh: What's the problem to go full with context always?

Maybe I was being overly cautious! Always using the context would be okay with me. I like the simplicity.

(Note that there is a separate problem to solve with the vocabularies that are serialized into the /@querystring endpoint response, which is always fetched at the site root.)

tiberiuichim commented 1 year ago

(Note that there is a separate problem to solve with the vocabularies that are serialized into the /@querystring endpoint response, which is always fetched at the site root.)

@querystringsearch allows passing path query, so it should be fine.

davisagli commented 1 year ago

@tiberiuichim I am talking about @querystring (which gets info about the available indexes), not @querystringsearch

ericof commented 1 month ago

@davisagli, @tiberiuichim, @sneridagh: Can we prioritize this?

davisagli commented 1 month ago

@ericof There is a PR which is almost ready: https://github.com/plone/volto/pull/6236

It just needs docs for the new setting.