Predicate: fix empty_criterion_valid check implementation.

It should be only for the case where no predicate criterion exists at all.

product/ERP5Type/Core/Predicate.py

@@ -222,8 +222,7 @@ class Predicate(XMLObject):
     catalog_kw = {}
     catalog_kw.update(kw) # query_table, REQUEST, ignore_empty_string, **kw
     criterion_list = self.getCriterionList()
-    # BBB: accessor is not present on old Predicate property sheet.
-    if criterion_list or getattr(self, 'isEmptyCriterionValid', lambda: True)():
+    if criterion_list:
       for criterion in criterion_list:
         p = criterion.property
         if criterion.min:
@@ -248,10 +247,6 @@ class Predicate(XMLObject):
             else:
               f = (i,) if i in f else ()
             catalog_kw[p] = list(f)
-    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')
 
@@ -302,6 +297,13 @@ class Predicate(XMLObject):
       if and_expression:
         multimembership_select_list.append(and_expression)
 
+    # BBB: accessor is not present on old Predicate property sheet.
+    if not getattr(self, 'isEmptyCriterionValid', lambda: True)() and \
+      not catalog_kw and not membership_select_list and not multimembership_select_list:
+      # By catalog definition, no object has uid 0, so this condition forces an
+      # empty result.
+      catalog_kw['uid'] = 0
+
     # Build the join where expression
     join_select_list = []
     for k in from_table_dict.iterkeys():

[Vincent Pelletier]

Overall I agree with this (now old) change.

But I am not sure about checking catalog_kw here: it means that any condition provided by caller bypasses isEmptyCriterionValid. As I am reworking this method to not receive any parameter (it will produce a query tree which caller will then include in their catalog call with any other parameter), isEmptyCriterionValid will produce a no-match query, which is causing test failures which I believe are inconsistent with isEmptyCriterionValid intent.

For example, testERP5Commerce creates (with WebSite_setupECommerceWebSite) web sections with no predicate and without providing a value for isEmptyCriterionValid (so it defaults to False on any new such section). Then, it lists products with WebSite_getProductList, which passes portal_type="Product" as parameter. So catalog_kw is not empty even though there is no criterion, so this matches documents.

I believe this is not what is intended by isEmptyCriterionValid, to never return "everything" (even if everythin is "just" all products), but to force the web section to have an effect as a predicate. If caller wants something section-independent, they should just call catalog. Or set the isEmptyCriterionValid flag if the section is really about everything.

In the case of WebSite_setupECommerceWebSite, it only prepares a site template, so it makes sense to not define any criterion, so I would setEmptyCriterionValid(True) in the test (to not encourage setting this property in the default commerce site template, users should really define predicates).

/cc @Tyagov as testERP5Commerce author

[Vincent Pelletier]

For reference: I applied proposed test change (ack'ed by @Tyagov ) in 06af6169 .