Commit 123e7c69 authored by Vincent Pelletier's avatar Vincent Pelletier

erp5_crm: Add support for deeper "use" resource hierachy.

Remove depth limitations on domain generator.
Make ListField item list generator present resource hierarchically (either
through indentation or through relative title-based paths, depending on
preference).
Also, share a bit of code between both.
parent 148b1a57
......@@ -50,66 +50,41 @@
</item>
<item>
<key> <string>_body</string> </key>
<value> <string encoding="cdata"><![CDATA[
portal = context.getPortalObject()\n
request = context.REQUEST\n
<value> <string>portal = context.getPortalObject()\n
if depth:\n
category_relative_url = parent.getMembershipCriterionCategory()\n
else:\n
category_relative_url = portal.portal_preferences.getPreference(\n
\'preferred_\' + context.REQUEST[\'here\'].getPortalType().replace(\' Module\', \'\').lower().replace(\' \', \'_\') + \'_use\',\n
)\n
  • @vpelletier I got an error here by:

    • Going to a module and setting a domain that returns more than one documents
    • Accessing a document from the listbox
    • Pressing the -> arrow (on the top) to go to the next item.

    Got KeyError: 'here'

    It tries to access the next element the domain returned as far as I understand. Not sure why here is not in the REQUEST when I am on document. The below patch works:

      document = context.REQUEST.get('here', None)
      if document is None:
        document = portal.restrictedTraverse(context.REQUEST.get('object_path'))
      category_relative_url = portal.portal_preferences.getPreference(
        'preferred_' + document.getPortalType().replace(' Module', '').lower().replace(' ', '_') + '_use',
      )

    but not sure if good. Any better idea?

  • here is the hystorical name for context in TALES expressions. AFAIK it is deprecated, and I have no idea why it was put in REQUEST to begin with. Nor have I any idea why I thought context.REQUEST['here'] would make any sense.

    So I think the correct fix is to:

    • check we are actually always called with the module as context (just for consistency, as otherwise will not make a significant functional difference in this code)
    • stop accessing REQUEST at all, just use context
  • Yet context here is Temporary Domain Generator and this is because this is used as the Domain Generator Method of the ticket_use_domain. This is why I suppose accessing REQUEST was used in first place

  • This explains why here would be accessed, indeed.

    This also seems like a dubious design, as context is not even used itself for anything (just to access portal, and to access REQUEST which is itself just 2 acquisition levels above portal - so portal.REQUEST is faster than context.REQUEST).

    And this may be your culprit if you were using the new UI:

    $ git grep "\(['\"]\)here\1"
    [...]
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/documentation_template.zpt:      <tal:block tal:define="dummy python: request.set('here',here)">
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/form_dialog.zpt:        <tal:block tal:define="dummy python: request.set('here', here);
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/form_dialog.zpt:        <tal:block tal:define="dummy python: request.set('here', here);
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/form_dialog.zpt:        <tal:block tal:define="dummy python: request.set('here', here);
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/form_render.zpt:              tal:define="dummy        python: request.set('here', here);
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/global_definitions.zpt:           dummy  python: request.set('here', here);
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/relation_form.zpt:        <tal:block tal:define="dummy python: request.set('here', here);
    product/ERP5/bootstrap/erp5_xhtml_style/SkinTemplateItem/portal_skins/erp5_xhtml_style/relation_form.zpt:        <tal:block tal:define="dummy python: request.set('here', here);
    [...]

    How could it be made any uglier, I wonder ?

  • And this may be your culprit if you were using the new UI:

    Ah, no, this is also broken with the old UI whenever the domain is populated outside of UI rendering context anyway, like with these next/previous/first/last actions. So this may just be a very old bug.

    Also, above grep on generic repository shows this pattern is rather common. Ugh.

  • Yes, as far as I understand it is old bug.

    Not sure how often people use the previous / next arrows in a document, I never do myself, I just happened to see it.

    My fix works and I think it is harmless. It does not make things pretty indeed, but I doubt I would have time to work in something better soon. Should we apply?

    cc @jerome

Please register or sign in to reply
if not category_relative_url:\n
return ()\n
child_list, resource_list = portal.portal_categories.use.restrictedTraverse(category_relative_url).Category_getUseCategoryListAndResourceList()\n
domain_list = []\n
\n
here = request[\'here\']\n
portal_type = here.getPortalType().replace(\' Module\', \'\')\n
\n
preference_id = \'preferred_%s_use\' % \'_\'.join(token.lower() for token in portal_type.split(\' \'))\n
preferred_use = portal.portal_preferences.getPreference(preference_id)\n
if not preferred_use:\n
return ()\n
\n
use = portal.portal_categories.use.restrictedTraverse(preferred_use)\n
sub_use_list = use.contentValues()\n
\n
max_depth = 0\n
if sub_use_list:\n
max_depth = 1\n
if depth > max_depth:\n
return ()\n
\n
if sub_use_list:\n
if depth == 0:\n
for sub_use in sub_use_list:\n
domain = parent.generateTempDomain(id = \'new_%s\' % sub_use.getProperty(\'uid\'))\n
domain.edit(title=sub_use.getTranslatedTitle(),\n
membership_criterion_category=(sub_use.getRelativeUrl(),),\n
domain_generator_method_id=script.id,\n
uid=use.getUid())\n
domain.setCriterionPropertyList([\'related_resource_from_use_category_uid\'])\n
domain.setCriterion(\'related_resource_from_use_category_uid\', identity=sub_use.getUid())\n
domain_list.append(domain)\n
return domain_list\n
else:\n
use = portal.portal_categories.use.restrictedTraverse(parent.getMembershipCriterionCategory())\n
\n
sql_kw = { \'portal_type\': portal.getPortalResourceTypeList(),\n
\'use_uid\': use.getUid(),\n
\'validation_state\': \'validated\',\n
\'sort_on\': \'title\'}\n
\n
for service in portal.portal_catalog(**sql_kw):\n
domain = parent.generateTempDomain(id = \'new_%s\' % service.getProperty(\'uid\'))\n
domain.edit(title=service.getTranslatedTitle(),\n
membership_criterion_base_category=(\'resource\', ),\n
membership_criterion_category=(\'resource/%s\' % service.getRelativeUrl(),),\n
domain_generator_method_id=script.id,\n
uid=service.getUid())\n
for child in child_list:\n
domain = parent.generateTempDomain(id=child.getId())\n
domain.edit(\n
title=child.getTranslatedTitle(),\n
membership_criterion_category=(child.getRelativeUrl(), ),\n
domain_generator_method_id=script.id,\n
)\n
domain.setCriterionPropertyList([\'related_resource_from_use_category_uid\'])\n
domain.setCriterion(\'related_resource_from_use_category_uid\', identity=child.getUid())\n
domain_list.append(domain)\n
\n
return domain_list\n
]]></string> </value>
for resource in resource_list:\n
domain = parent.generateTempDomain(id=resource.getId())\n
domain.edit(\n
title=resource.getTranslatedTitle(),\n
membership_criterion_base_category=(\'resource\', ),\n
membership_criterion_category=(\'resource/\' + resource.getRelativeUrl(), ),\n
)\n
domain_list.append(domain)\n
return sorted(domain_list, key=lambda x: x.getTitle())\n
</string> </value>
</item>
<item>
<key> <string>_params</string> </key>
<value> <string>depth, parent, **kw</string> </value>
<value> <string>depth, parent</string> </value>
</item>
<item>
<key> <string>id</string> </key>
......
<?xml version="1.0"?>
<ZopeData>
<record id="1" aka="AAAAAAAAAAE=">
<pickle>
<global name="PythonScript" module="Products.PythonScripts.PythonScript"/>
</pickle>
<pickle>
<dictionary>
<item>
<key> <string>Script_magic</string> </key>
<value> <int>3</int> </value>
</item>
<item>
<key> <string>_bind_names</string> </key>
<value>
<object>
<klass>
<global name="NameAssignments" module="Shared.DC.Scripts.Bindings"/>
</klass>
<tuple/>
<state>
<dictionary>
<item>
<key> <string>_asgns</string> </key>
<value>
<dictionary>
<item>
<key> <string>name_container</string> </key>
<value> <string>container</string> </value>
</item>
<item>
<key> <string>name_context</string> </key>
<value> <string>context</string> </value>
</item>
<item>
<key> <string>name_m_self</string> </key>
<value> <string>script</string> </value>
</item>
<item>
<key> <string>name_subpath</string> </key>
<value> <string>traverse_subpath</string> </value>
</item>
</dictionary>
</value>
</item>
</dictionary>
</state>
</object>
</value>
</item>
<item>
<key> <string>_body</string> </key>
<value> <string>portal = context.getPortalObject()\n
return context.objectValues(), portal.portal_catalog(\n
portal_type=portal.getPortalResourceTypeList(),\n
strict_use_uid=context.getUid(),\n
validation_state=\'validated\',\n
)\n
</string> </value>
</item>
<item>
<key> <string>_params</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>id</string> </key>
<value> <string>Category_getUseCategoryListAndResourceList</string> </value>
</item>
</dictionary>
</pickle>
</record>
</ZopeData>
......@@ -51,52 +51,132 @@
<item>
<key> <string>_body</string> </key>
<value> <string>"""\n
This script returns the list of items based on the preferred\n
resources for tickets. It is intended to be used\n
by ListField instances.\n
portal_type (None, string)\n
Used to determine "use" category to start recursing from, using preference\n
settings for this portal type.\n
When None, context\'s portal_type is used.\n
empty_item (bool)\n
Controls presence of [\'\', \'\'] element in result.\n
indent_category (bool)\n
When true, category captions are indented.\n
When false, categories captions are paths, relative to topmost category (not\n
necessarily a Base Category !).\n
indent_resource (bool)\n
When true, resource captions are indented.\n
When false, resource captions are not indented.\n
compact (bool)\n
When true, getCompactTranslatedTitle is used to generate captions.\n
When false, getTranslatedTitle is used to generate captions.\n
empty_category (bool)\n
When true, categories with no resource children (at any depth) are present\n
in result.\n
When false, categories with no resource children (at any depth) are pruned\n
from result.\n
\n
When indent_category, indent_resource and compact are simultaneously not\n
provided (or None), a default is built from\n
getPreferredCategoryChildItemListMethodId.\n
"""\n
from zExceptions import Unauthorized\n
# Note: a possible improvement would be to merge consecutive disabled entries.\n
# This is difficult though, because it requires splitting work a lot,\n
# increasing complexity significantly for such little improvement:\n
# - non-child categories must not be concatenated (empty /1/12/ must not be\n
# merged with a following /2/)\n
# - all resource child must be properly indented\n
# It is much simpler if only "empty_category=False" case is handled.\n
from Products.ERP5Type.Cache import CachingMethod\n
portal = context.getPortalObject()\n
portal_preferences = portal.portal_preferences\n
category_id = portal_preferences.getPreference(\n
\'preferred_\' + (portal_type or context.getPortalType()).lower().replace(\' \', \'_\') + \'_use\',\n
)\n
if indent_category == indent_resource == compact == None:\n
indent_category, indent_resource, compact = {\n
\'getCategoryChildTranslatedCompactLogicalPathItemList\': (False, False, True),\n
\'getCategoryChildTranslatedLogicalPathItemList\': (False, True, False),\n
\'getCategoryChildTranslatedIndentedCompactTitleItemList\': (True, False, True),\n
\'getCategoryChildTranslatedIndentedTitleItemList\': (True, True, False),\n
}.get(portal_preferences.getPreferredCategoryChildItemListMethodId(), (True, True, False))\n
\n
if not portal_type:\n
portal_type = context.getPortalType()\n
\n
def getResourceItemList(portal_type):\n
preference_id = \'preferred_%s_use\' % \'_\'.join(token.lower() for token in portal_type.split(\' \'))\n
sql_kw = {\'portal_type\': portal.getPortalResourceTypeList(),\n
\'use_uid\': portal.portal_categories.getCategoryUid(portal.portal_preferences.getPreference(preference_id), base_category=\'use\'),\n
\'validation_state\': \'validated\',\n
\'sort_on\': \'title\'}\n
return [(\'\', \'\')] + [(result.getTranslatedTitle(), result.getRelativeUrl()) for result in portal.portal_catalog(**sql_kw)]\n
\n
getResourceItemList = CachingMethod(getResourceItemList, \n
id=(script.id, portal_type, context.Localizer.get_selected_language()), \n
cache_factory=\'erp5_ui_long\')\n
\n
result_list = getResourceItemList(portal_type)[:]\n
accessor_id = \'getCompactTranslatedTitle\' if compact else \'getTranslatedTitle\'\n
\n
# BBB returns actual value in list field\n
if include_context and context.getResource() and context.getResource() not in [result[1] for result in result_list]:\n
try:\n
resource_value = portal.portal_categories.getCategoryValue(context.getResource(), base_category=\'resource\')\n
if resource_value is not None:\n
if resource_value.getPortalType() == \'Category\':\n
category_relative_url = resource_value.getCategoryRelativeUrl()\n
category_title = resource_value.getTranslatedTitle()\n
else:\n
category_relative_url = resource_value.getRelativeUrl()\n
category_title = resource_value.getTitle()\n
result_list.append((category_title, category_relative_url))\n
except Unauthorized:\n
pass\n
def getResourceItemList():\n
INDENT = \'\\xc2\\xa0\' * 2 # UTF-8 Non-breaking space\n
RESOURCE_INDENT = INDENT if indent_resource else \'\'\n
getResourceTitle = lambda resource, category, depth: RESOURCE_INDENT * depth + getattr(resource, accessor_id)()\n
if indent_category:\n
def getCategoryTitle(category, depth):\n
return INDENT * depth + getattr(category, accessor_id)()\n
else:\n
def getCategoryTitle_(category, depth):\n
result = []\n
append = result.append\n
for _ in xrange(depth + 1):\n
append(getattr(category, accessor_id)())\n
category = category.getParentValue()\n
return \'/\'.join(result[::-1])\n
if indent_resource:\n
getCategoryTitle = getCategoryTitle_\n
else:\n
getCategoryTitle = lambda category, depth: None\n
def getResourceTitle(resource, category, depth):\n
resource_title = getattr(resource, accessor_id)()\n
# depth - 1 because we are at category\'s child level\n
category_path = getCategoryTitle_(category, depth - 1)\n
if category_path:\n
return category_path + \'/\' + resource_title\n
return resource_title\n
def recurse(category, depth):\n
child_list, resource_list = category.Category_getUseCategoryListAndResourceList()\n
# Resources before child categories, to avoid ambiguity when resources are not indented\n
result = sorted(\n
[(getResourceTitle(x, category, depth), x.getRelativeUrl()) for x in resource_list],\n
key=lambda x: x[0],\n
)\n
append = result.append\n
extend = result.extend\n
for _, caption, grand_child_list in sorted(\n
[(x.getIntIndex(), getCategoryTitle(x, depth), recurse(x, depth + 1)) for x in child_list],\n
  • @vpelletier what would you think of the idea of excluding 'expired' categories here? Like

              [(x.getIntIndex(), getCategoryTitle(x, depth), recurse(x, depth + 1)) for x in child_list if x.getValidationState() != 'expired'],

    and maybe also below (line 148 in this revision)

      if category is None or category.getValidationState() == 'expired':
        return []

    I am not 100% sure what 'expired' means semantically for us, but it seems valid to my current knowledge

  • "Expired" means "users should not use this category for any new document", without breaking links for historical documents which still use that category. Code may still propagate an old category to new documents (ex: sale order exists with relation to old category, sale packing list is later created and relation is copied there).

    But from UI point of view there are some caveat:

    • If the widget which shows the relation on an existing document suddenly stops listing the cateory of current relation because it became "expired", then users get a " (???)" caption, which may make them think the relation is broken, then they may submit useless reports or try "fixing" it themelves.
    • It is not completely clear to me whether it is "should not" or "must not" ("...use that category"). Maybe there are valid reasons for a user to create documents on expired categories sometimes.

    So it depends a lot on how this value is used, where (dialog or edit UI), what we really mean when setting these categories to "expired"... In the case of "use" category, we may want to keep all value in edition fields on use-able documents (services, ...), but prevent listing them on "reply to support request" dialog when looking for some templates. I'm not a big fan of extending this parameter list, but maybe there is no way around.

  • If such filter has to be implemented, I think we can implement by checking if user has View permission on category, it can be achieved using checked_permission='View', like we do on most category list fields. This also covers categories in "protected" state.

    • If the widget which shows the relation on an existing document suddenly stops listing the cateory of current relation because it became "expired", then users get a " (???)" caption, which may make them think the relation is broken, then they may submit useless reports or try "fixing" it themelves.

    We could probably add current value if it does not exist, like

    resource_value = context.getResourceValue()
    checkPermission = portal.portal_membership.checkPermission # see Jerome's suggestion
    ...
    ... if checkPermission('View', x) or x == resource_value]

    This does become more complex, but it makes some sense to me.

    • It is not completely clear to me whether it is "should not" or "must not" ("...use that category"). Maybe there are valid reasons for a user to create documents on expired categories sometimes.

    Yet, then what is really the difference of expired against embedded if we do show it here? Do we have other checks on category_publication_workflow state?

    If such filter has to be implemented, I think we can implement by checking if user has View permission on category, it can be achieved using checked_permission='View', like we do on most category list fields. This also covers categories in "protected" state.

    Makes sense indeed, thanks. Though I think it is complex here to do it in field definition, I think it should still be in this script, right?

  • if checkPermission('View', x) or x == resource_value]

    I think this check makes not much sense, since the result is cached. So e.g a Manager user could cache the result for a non-Manager user, and the latter would see also the expired ones

  • I think this check makes not much sense, since the result is cached

    True, it would be incompatible with this cache.

    I think the default "item list methods" are cached by user when they involve security, the code seem to be here.

    [edit: I checked very quickly, this might be wrong]

    Edited by Jérome Perrin
Please register or sign in to reply
key=lambda x: x[:2],\n
):\n
if grand_child_list or empty_category:\n
if caption is not None:\n
append((caption, None))\n
extend(grand_child_list)\n
return result\n
category = portal.portal_categories.getCategoryValue(category_id, base_category=\'use\')\n
if category is None:\n
return []\n
return recurse(category, 0)\n
\n
return result_list\n
result = CachingMethod(\n
getResourceItemList,\n
id=(\n
script.id,\n
context.Localizer.get_selected_language(),\n
bool(indent_resource),\n
bool(indent_category),\n
accessor_id,\n
bool(empty_category),\n
category_id,\n
),\n
cache_factory=\'erp5_ui_long\',\n
)()\n
if empty_item:\n
prefix = [(\'\', \'\')]\n
else:\n
prefix = []\n
if include_context:\n
context_resource_value = context.getResourceValue()\n
context_resource = context.getResource()\n
if context_resource_value is not None and context_resource not in [x for _, x in result]:\n
prefix.append((getattr(context_resource_value, accessor_id)(), context_resource))\n
return prefix + result\n
</string> </value>
</item>
<item>
<key> <string>_params</string> </key>
<value> <string>portal_type=None, include_context=True</string> </value>
<value> <string>portal_type=None, include_context=True, empty_item=True, indent_category=None, indent_resource=None, compact=None, empty_category=False</string> </value>
</item>
<item>
<key> <string>id</string> </key>
......
668
\ No newline at end of file
669
......@@ -437,8 +437,12 @@ class TestCRM(BaseTestCRM):
# Then create One event and play with it
portal_type = 'Visit'
module = self.portal.getDefaultModule(portal_type)
event = module.newContent(portal_type=portal_type,
resource='0')
event = module.newContent(portal_type=portal_type)
# Check that existing valid resource relations which should not be normaly
# found by Event_getResourceItemList are present.
self.assertTrue(event.getResource() not in\
[item[1] for item in event.Event_getResourceItemList()])
event.setResource('0')
self.assertTrue(event.getResourceValue() is not None)
self.assertTrue(event.getResource() in\
[item[1] for item in event.Event_getResourceItemList()])
......
......@@ -235,34 +235,41 @@ class Predicate(XMLObject):
# Build the identity criterion
catalog_kw = {}
catalog_kw.update(kw) # query_table, REQUEST, ignore_empty_string, **kw
for criterion in self.getCriterionList():
if criterion.min and criterion.max:
catalog_kw[criterion.property] = { 'query' : (criterion.min, criterion.max),
'range' : 'minmax'
}
elif criterion.min:
catalog_kw[criterion.property] = { 'query' : criterion.min,
'range' : 'min'
}
elif criterion.max:
catalog_kw[criterion.property] = { 'query' : criterion.max,
'range' : 'max'
}
else:
# if a filter was passed as argument
if catalog_kw.has_key(criterion.property):
if isinstance(catalog_kw[criterion.property], (tuple, list)):
catalog_filter_set = set(catalog_kw[criterion.property])
else:
catalog_filter_set = set([catalog_kw[criterion.property]])
if isinstance(criterion.identity, (tuple, list)):
parameter_filter_set = set(criterion.identity)
else:
parameter_filter_set = set([criterion.identity])
catalog_kw[criterion.property] = \
list(catalog_filter_set.intersection(parameter_filter_set))
criterion_list = self.getCriterionList()
# BBB: accessor is not present on old Predicate property sheet.
if criterion_list or getattr(self, 'isEmptyPredicateValid', lambda: True)():
for criterion in criterion_list:
if criterion.min and criterion.max:
catalog_kw[criterion.property] = { 'query' : (criterion.min, criterion.max),
'range' : 'minmax'
}
elif criterion.min:
catalog_kw[criterion.property] = { 'query' : criterion.min,
'range' : 'min'
}
elif criterion.max:
catalog_kw[criterion.property] = { 'query' : criterion.max,
'range' : 'max'
}
else:
catalog_kw[criterion.property] = criterion.identity
# if a filter was passed as argument
if catalog_kw.has_key(criterion.property):
if isinstance(catalog_kw[criterion.property], (tuple, list)):
catalog_filter_set = set(catalog_kw[criterion.property])
else:
catalog_filter_set = set([catalog_kw[criterion.property]])
if isinstance(criterion.identity, (tuple, list)):
parameter_filter_set = set(criterion.identity)
else:
parameter_filter_set = set([criterion.identity])
catalog_kw[criterion.property] = \
list(catalog_filter_set.intersection(parameter_filter_set))
else:
catalog_kw[criterion.property] = criterion.identity
else:
# By catalog definition, no object has uid 0, so this condition forces an
# empty result.
catalog_kw['uid'] = 0
portal_catalog = getToolByName(self, 'portal_catalog')
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment