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

Add static catalog vocabulary to support various widgets with relationsfields or uuid-fields #66

Merged
merged 10 commits into from
Aug 12, 2021

Conversation

pbauer
Copy link
Member

@pbauer pbauer commented Feb 1, 2021

No description provided.

@mister-roboto
Copy link

@pbauer thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@alecpm
Copy link
Member

alecpm commented Mar 1, 2021

It's using plone.memoize.view.memoize now. I've also added some tests and a smarter path method for the default title template.

@alecpm
Copy link
Member

alecpm commented Mar 1, 2021

@jenkins-plone-org please run jobs

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

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

Remove the code that is no longer needed. Or did I miss something?

@jensens
Copy link
Member

jensens commented Mar 1, 2021

@jenkins-plone-org please run jobs

@@ -1,4 +1,5 @@
# -*- coding: utf-8 -*-
import json
Copy link
Member

Choose a reason for hiding this comment

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

This import should go after import itertools.
Not a big deal, but imports are already sorted and would be nice to keep them sorted

def createTerm(self, brain, context=None):
return SimpleTerm(
value=brain.UID, token=brain.UID,
title=self.title_template.format(
Copy link
Member

Choose a reason for hiding this comment

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

If I am not wrong this might break in Python 2, where title is expected to be text. Please consider this before merging

Copy link
Member

@alecpm alecpm Mar 2, 2021

Choose a reason for hiding this comment

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

You're right, we probably need some special handling here for Python 2 where brain.Title is always an encoded string, but Term.title is always unicode. I think that can be fixed with a simple call to safe_unicode though.

Copy link
Member

Choose a reason for hiding this comment

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

This is the last blocker here before we can merge, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, wrapping this with safe_unicode seem to be the right thing to do

@pbauer
Copy link
Member Author

pbauer commented Mar 10, 2021

@ale-rt @alecpm The change to use plone.memoize.view.memoize breaks this for me. In a edit-form with two different fields that use this feature with different queries I get the same options twice.
When I go back to 121c816 it works again and each field offers the expected options.

Here is the relevant excerpt from my schema:

    basisvariante = RelationChoice(
        title='Basisvariante',
        description=_('Leer lassen, wenn dies eine Basisvariante ist!'),
        vocabulary=StaticCatalogVocabulary(
            {
                'portal_type': ['product'],
                'base_variant': True,
                'path': '/Plone/produkte',
                'sort_on': 'id',
            },
            title_template='{brain.getId} - {brain.Title}',
        ),
        required=False,
    )
    directives.widget(
        'basisvariante',
        SelectFieldWidget,
    )

    sonderausstattung = RelationList(
        title='Sonderausstattung',
        default=[],
        value_type=RelationChoice(
            vocabulary=StaticCatalogVocabulary(
                {
                    'portal_type': 'accessory',
                    'path': '/Plone/accessories',
                    'sort_on': 'id',
                },
                title_template='{brain.getId} - {brain.Title}',
            )
        ),
        required=False,
        missing_value=[],
    )
    directives.widget(
        'sonderausstattung',
        AjaxSelectFieldWidget,
        vocabulary=StaticCatalogVocabulary(
            {
                'portal_type': 'accessory',
                'path': '/Plone/accessories',
                'sort_on': 'id',
            },
            title_template='{brain.getId} - {brain.Title}',
        ),
        pattern_options={
            'closeOnSelect': False,
            'minimumInputLength': 2,
            'ajax': {'quietMillis': 500},
        },
    )

@jensens
Copy link
Member

jensens commented Mar 15, 2021

@pbauer, indeed, view.memoize does take context physical path and function arguments into the cache key, both are the same in one request for brains if there are two fields. I would propose to use plone.memoize.ram and cache based on a custom cache key self.query, context.getPhysicalPath() and the catalogCounter (did I miss something?).

@alecpm
Copy link
Member

alecpm commented Mar 15, 2021

The original implementation using request.memoize used the self.query as part of the cache key, and I think that's probably the simplest solution. The main goal of the caching was to avoid doing the same query multiple times in a request when doing field/widget validation using the __contains__ method. Fields tend to validate themselves a bunch of times, and doing a multiple catalog queries for each stored or input value gets expensive really fast. I think it's probably best if I restore the old caching behavior which was working fine. I'll do that and fix the unicode title issue later today.

@alecpm
Copy link
Member

alecpm commented Apr 5, 2021

Sorry for the delay all. I've reverted the cache changes, added a call to safe_unicode, and cleaned up the import order. Should be ready to go.

@jensens
Copy link
Member

jensens commented Apr 6, 2021

@jenkins-plone-org please run jobs

@jensens
Copy link
Member

jensens commented Apr 6, 2021

Seems tests in Python 2.7 are still failing because of some unicode confusion. I would say, after fixing this just go and merge: https://jenkins.plone.org/job/pull-request-5.2/1959/testReport/junit/plone.app.vocabularies/catalog/StaticCatalogVocabulary/

@ale-rt
Copy link
Member

ale-rt commented Apr 6, 2021

Sorry for giving you a wrong hint about using plone.memoize!

@pbauer pbauer force-pushed the static-catalog-vocabulary branch from 738fc9d to b3c12f6 Compare August 11, 2021 15:58
@pbauer
Copy link
Member Author

pbauer commented Aug 11, 2021

@jenkins-plone-org please run jobs

@pbauer
Copy link
Member Author

pbauer commented Aug 12, 2021

@jenkins-plone-org please run jobs

@pbauer pbauer merged commit 4569295 into master Aug 12, 2021
@jensens jensens deleted the static-catalog-vocabulary branch August 17, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants