Skip to content
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

Make collectionfilter show human-readable title when group_by is on index where dexterity has a vocabulary or choices and no display_modifier applied. #41

Open
pigeonflight opened this issue Apr 3, 2019 · 22 comments

Comments

@pigeonflight
Copy link
Member

pigeonflight commented Apr 3, 2019

Rationale

The collectionfilter portlet is not smart enough to return human readable titles on vocabularies provided through the web (TTW).
I would like to be able to show human readable titles, whether a vocabulary is provided from a filesystem product with the proper display_modifier or from a TTW provided vocabulary.

A specific example of this is collective.ambidexterity which provides a mechanism for associating a vocabulary with a content type. Ambidexterity vocabularies are defined through the web (TTW) in a control panel provided by collective.ambidexterity.

Most commonly, the vocabulary is presented as a list of tuples.

vocabulary = [ ('value1', u'Label for value1'), ('value2', u'Label for value2')]

expected behaviour

The resulting dropdown box would present the human readable title (term.value), most likely derived from the indexed token (term.token).

(for simplicity, I've removed id and class values below).

<select>
    <option value="--NOVALUE--">No value</option>
    <option value="value1">Label for value1</option>
    <option value="value2" selected="selected">Label for value2</option>
   </select>

actual behaviour

The resulting dropdown menu presents the dropdown with only the tokens, not the "human readable" (term.value).

<select>
    <option value="--NOVALUE--">No value</option>
    <option value="value1">value1</option>
    <option value="value2" selected="selected">value2</option>
   </select>

Proposed fix

Provide an option to fetch human readable title this would make best effort to locate the title from a vocabulary associated with the field.
It would

  • use from plone.dexterity.schema import SCHEMA_CACHE
  • find all content types in the collection result that have the field that we have selected to "group_by".
  • attempt to retrieve the term.value (human readable title) based on the indexed value (which we assume would correspond to the term.token

The provided interface would look something like this
image

@pigeonflight pigeonflight changed the title Dropdown portlets does not show human-readable title when working with a Dropdown portlet does not show human-readable title when working with a Apr 3, 2019
@pigeonflight pigeonflight changed the title Dropdown portlet does not show human-readable title when working with a Dropdown portlet does not show human-readable title when working with a list of tuples type vocabulary Apr 3, 2019
@petschki
Copy link
Member

petschki commented Apr 3, 2019

Well this is exactly what the display_modifier is made for. If you have a custom vocabulary make shure you adapt your index with its display modifier

https://github.com/collective/collective.collectionfilter/blob/master/src/collective/collectionfilter/interfaces.py

@petschki
Copy link
Member

petschki commented Apr 3, 2019

Here‘s a snippet I‘ve made for collective.taxonomy ...
Maybe this helps to get an idea.

# -*- coding: utf-8 -*-
from collective.collectionfilter.interfaces import IGroupByCriteria
from collective.collectionfilter.interfaces import IGroupByModifier
from collective.taxonomy.interfaces import ITaxonomy
from plone import api
from zope.component import adapter
from zope.component import queryUtility
from zope.interface import implementer

import re


class TaxonomyLabel(object):

    utility = None

    def __init__(self, taxonomy_name):
        self.utility = queryUtility(
            ITaxonomy, name='collective.taxonomy.%s' % taxonomy_name)

    def display(self, msgid):
        return self.utility.translate(
            msgid, target_language=api.portal.get_current_language())


@implementer(IGroupByModifier)
@adapter(IGroupByCriteria)
def groupby_modifier(groupby):

    for taxonomy in ('interviewort',
                     'ort',
                     'sammlung',
                     'themenfilter'):

        taxonomy_label = TaxonomyLabel(taxonomy)

        if taxonomy_label.utility is None:
            continue

        groupby._groupby['taxonomy_%s' % taxonomy] = {
            'index': 'taxonomy_%s' % taxonomy,
            'metadata': 'taxonomy_%s' % taxonomy,
            'display_modifier': taxonomy_label.display,
            'sort_key_function': lambda val: lambda val: val['title'].lower(),
        }

@pigeonflight
Copy link
Member Author

pigeonflight commented Apr 3, 2019

@petschki,
Just to clarify, this means...

  1. I need to provide something that implements IGroupByModifier for my use case?
  2. My product would have collective.collectionfilter as a dependency?

@petschki
Copy link
Member

petschki commented Apr 4, 2019

@pigeonflight
Copy link
Member Author

pigeonflight commented Apr 4, 2019

In my scenario, my vocabulary is defined TTW using collective.ambidexerity.
It seems like I have 3 choices.

  1. Make collective.collectionfilter aware of ambidexterity (some kind of adapter for ambidexterity vocabularies)
  2. Implement the adapter in ambidexterity
  3. Create a third party package that implements the adapter so that ambidexterity vocabularies have IGroupByModifier capabilities

@pigeonflight pigeonflight changed the title Dropdown portlet does not show human-readable title when working with a list of tuples type vocabulary Dropdown portlet does not show human-readable title when vocabulary is provided by collective.ambidexterity Apr 4, 2019
@instification
Copy link
Member

instification commented Apr 4, 2019

@pigeonflight I don't think checking for ambidexterity is the correct route. ambidexterity would either need to implement the adapter (or possibly a separate package).

However, I think we do need a solution for TTW usage (where writing adapters isn't available) and for basic/end users who would have higher expectations I think when using the collection filters.

I think we could implement the original proposal (give the user a setting to automatically try and get values for indexes from type schemas) in such a way that it's only an option if there are no IGroupByModifier adapters registered and update the documentation accordingly. That way, basic users and those implementing ttw solutions can get friendly values in their filters without the need to understand or develop adapters.

@pigeonflight pigeonflight changed the title Dropdown portlet does not show human-readable title when vocabulary is provided by collective.ambidexterity Dropdown portlet does not show human-readable title when vocabulary is provided TTW Apr 4, 2019
@pigeonflight
Copy link
Member Author

pigeonflight commented Apr 4, 2019

@instification
My updated proposal is more generic and inline with your suggestion. It should work for more TTW scenarios, not just when a vocabulary is coming from ambidexterity. (see above)

@pigeonflight pigeonflight changed the title Dropdown portlet does not show human-readable title when vocabulary is provided TTW Make collectionfilter portlet show human-readable title when group_by is on index where vocabulary is provided TTW Apr 4, 2019
@djay
Copy link
Member

djay commented Apr 5, 2019

@petschki I think that what we are suggesting here is an adjunct to the advanced usecase of creating an adapter + new plugin to get human readible names in the case where you have a dexterity schema with vocabularies already. This is going be a very common case and its built into plone so asking someone to install another plugin or write code for this is not very nice UI.
What we will do is if there is a display_modifier and it modifies the value we will leave it untouched. If not and there is a label associated with this index and vocabulary then we will use that.

@petschki
Copy link
Member

petschki commented Apr 5, 2019

@djay I'm not asking you to install another plugin. I just pointed to the places, where to implement it as a integrator for eg. in your theme package. I'm happy for every OOTB usability improvement in general. Maybe the Checkbox in the screenshot above should be a selector, where the user can explicitly define which vocabulary should be used to get the human readable label ... but if the underlying magic does the trick too it's fine.

@djay djay changed the title Make collectionfilter portlet show human-readable title when group_by is on index where vocabulary is provided TTW Make collectionfilter portlet show human-readable title when group_by is on index where dexterity has a vocabulary or choices and no display_modifier applied. Apr 5, 2019
@djay
Copy link
Member

djay commented Apr 5, 2019

@pigeonflight TTW or not is irrelevent.

@petschki petschki changed the title Make collectionfilter portlet show human-readable title when group_by is on index where dexterity has a vocabulary or choices and no display_modifier applied. Make collectionfilter show human-readable title when group_by is on index where dexterity has a vocabulary or choices and no display_modifier applied. Apr 5, 2019
@petschki
Copy link
Member

petschki commented Apr 5, 2019

let's keep it more general ... there are tiles too 😉

@djay
Copy link
Member

djay commented Apr 5, 2019

@petschki originally we had it as a selector to pick either the vocabulary or the at least the field. As there are cases where you can't map an index back to the field definitions that created it. For example you can a postcode on two different types with two different vocabularies yet the same index. But to have a UI to handle this makes it somewhat complicated so we figured for now its easier just to look up every vocabulary its likely to be and use the first one with a matching value.

@petschki
Copy link
Member

petschki commented Apr 5, 2019

just a note as looking through the code: there's a default display_modifier set to the i18n messagefactory of collective.collectionfilter (see https://github.com/collective/collective.collectionfilter/blob/master/src/collective/collectionfilter/vocabularies.py#L89) .. this makes sense in order to be able to translate vocabulary entries (which is also a valuable question in your usecase of TTW vocabularies). @thet might have some more thoughts on this ?

@pigeonflight
Copy link
Member Author

pigeonflight commented Apr 5, 2019

Just to say that the translation makes sense but doesn't solve many custom vocabulary scenarios.

Here's a slightly far fetched but illustrative usecase:

Site administrator has created a content type with a field called "my_workflow"

<field name="my_workflow" type="zope.schema.Choice" indexer:searchable="true">
    <title i18n:translate="">My Workflow</title>
    <description i18n:translate="">My Workflow of choice</description>
    <vocabulary>plone.app.vocabularies.Workflows</vocabulary>
    <default/>
    <required>True</required>
    <readonly>False</readonly>
  </field>

They create a collection filter portlet with group_by on my_workflow.
The resulting portlet returns output with the tokens instead of the human readable workflow names (see screenshot).

image

@pigeonflight
Copy link
Member Author

pigeonflight commented Apr 5, 2019

@pigeonflight TTW or not is irrelevent.

@djay
You're saying it isn't that helpful to include in the description?

@thet
Copy link
Member

thet commented Apr 5, 2019

Good idea to use a vocabulary if there is one.
I'm indifferent / not sure if that needs an extra option. Sounds like a good default.

As long as the code doesn't introduce any new dependencies, I'm fine with such an approach.
Vocabularies are normally registered as a utility and looked up by name. So, no extra deps needed for that. If the TTW vocabulary thing does it differently, we still can use soft-dependencies.

@thet
Copy link
Member

thet commented Apr 5, 2019

If the lookup of available vocabularies has a significant performance penalty, then this should a configurable option....

@instification
Copy link
Member

@thet agree entirely about the performance. We haven't really measured yet but if there are a lot of filters then this approach by default could certainly introduce a lot of overhead.

@pigeonflight
Copy link
Member Author

pigeonflight commented Apr 5, 2019

@thet
So perhaps keep it as an option, with a warning that it could degrade performance when enabled in some scenarios...

@thet
Copy link
Member

thet commented Apr 5, 2019

@pigeonflight @instification yes sounds good!
if it's just a named utility lookup it shouldn't add significantly to performance. but if it's some TTW vocabularies lookup magic, then it might.

@djay
Copy link
Member

djay commented Apr 22, 2019

@pigeonflight I made comments on your current approach here 7b203bd. Can you work in the collective rather than in private forks? Makes it harder for others to fix your code.
You also lack tests

@pigeonflight
Copy link
Member Author

Reviewing and working on a better implementation.
All the code is in the collective, this is the branch I'm working on now -->
https://github.com/collective/collective.collectionfilter/tree/pigeonflight/human-readable-titles-with-ttw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants