ERP5Site: Improve getDefaultModule*.

Add support for visibility checking.
Use portal_type accessors, not properties.
Also, original implementation of getDefaultModuleId would get the object,
then its id. getDefaultModuleValue using it, it was getting the object,
then its id, then the object again...
Also, follow naming convention: getters returning documents are named
getXxxValue, not getXxx.

product/ERP5/ERP5Site.py

@@ -1517,54 +1517,78 @@ class ERP5Site(FolderMixIn, CMFSite, CacheCookieMixin):
 
   security.declareProtected(Permissions.AccessContentsInformation,
                             'getDefaultModuleId')
-  def getDefaultModuleId(self, portal_type, default=MARKER):
+  def getDefaultModuleId(self, portal_type, default=MARKER, only_visible=False):
     """
     Return default module id where a object with portal_type can
     be created.
     """
-    portal_object = self
-    module_id = None
-    # first try to find by naming convention
-    expected_module_id = portal_type.lower().replace(' ','_')
-    if portal_object._getOb(expected_module_id, None) is not None:
-      module_id = expected_module_id
+    try:
+      module = self.getDefaultModuleValue(portal_type, only_visible=False)
+    except ValueError:
+      if default is MARKER:
+        raise ValueError('Unable to find module for portal_type: ' + portal_type)
+      return default
     else:
-      expected_module_id += '_module'
-      if portal_object._getOb(expected_module_id, None) is not None:
-        module_id = expected_module_id
-      # then look for module where the type is allowed
-      else:
-        for expected_module_id in portal_object.objectIds(('ERP5 Folder',)):
-          module = portal_object._getOb(expected_module_id, None)
-          if module is not None:
-            if portal_type in self.portal_types[module.getPortalType()].\
-                                      allowed_content_types:
-              module_id = expected_module_id
-              break
-
-    if module_id is None:
-      if default is not MARKER:
-        return default
-      else:
-        # now we fail
-        LOG('ERP5Site, getDefaultModuleId', 0,
-            'Unable to find default module for portal_type: %s' % \
-             portal_type)
-        raise ValueError, 'Unable to find module for portal_type: %s' % \
-               portal_type
-
-    return module_id
+      return module.getId()
 
   security.declareProtected(Permissions.AccessContentsInformation,
-                            'getDefaultModule')
+                            'getDefaultModuleValue')
+  def getDefaultModuleValue(self, portal_type, default=MARKER, only_visible=False):
+    """
+    Return default module where a object with portal_type can be created
+    portal_type (str)
+      Module or top-level document portal type.
+    default (anything)
+      Value to return if no module can be found from given portal type.
+      If not given and module is not found, ValueError is raised.
+    only_visible (bool)
+      When true, check that given portal type is part of module's visible
+      content types, else return default.
+    """
+    # first try to find by naming convention
+    expected_module_id = portal_type.lower().replace(' ','_')
+    module = self._getOb(expected_module_id, None)
+    if module is not None:
+      return module
+    if only_visible:
+      allowed = lambda x: (
+        x is not None and
+        portal_type in x.getVisibleAllowedContentTypeList()
+      )
+    else:
+      allowed = lambda x: (
+        x is not None and
+        portal_type in (y.id for y in x.allowedContentTypes())
+      )
+    expected_module_id += '_module'
+    module = self._getOb(expected_module_id, None)
+    if allowed(module):
+      return module
+    # then look for module where the type is allowed
+    for expected_module_id in self.objectIds(('ERP5 Folder',)):
+      module = self._getOb(expected_module_id, None)
+      if allowed(module):
+        return module
+    if default is MARKER:
+      raise ValueError('Unable to find module for portal_type: ' + portal_type)
+    return default
+
+  # BBB
+  security.declareProtected(
+    Permissions.AccessContentsInformation,
+    'getDefaultModule',
+  )
   def getDefaultModule(self, portal_type, default=MARKER):
     """
-      Return default module where a object with portal_type can be created
+    For backward-compatibility.
+    Use getDefaultModuleValue (beware of slight "default" semantic change !).
     """
-    module_id = self.getDefaultModuleId(portal_type, default)
-    if module_id:
-      return getattr(self, module_id, None)
-    return None
+    try:
+      return self.getDefaultModuleValue(portal_type)
+    except ValueError:
+      if default is MARKER:
+        raise ValueError('Unable to find module for portal_type: ' + portal_type)
+      return self._getOb(default)
 
   security.declareProtected(Permissions.AddPortalContent, 'newContent')
   def newContent(self, id=None, portal_type=None, **kw):

product/ERP5/tests/testERP5Core.py

@@ -334,47 +334,110 @@ class TestERP5Core(ERP5TypeTestCase, ZopeTestCase.Functional):
 
   def test_03_getDefaultModule(self, quiet=quiet, run=run_all_test):
     """
-    test getDefaultModule method
+    test getDefaultModule(|Id|Value) methods
     """
     if not run:
       return
-    portal_id = self.getPortal().getId()
-    object_portal_type = ' '.join([part.capitalize() for part \
-                                    in portal_id.split('_')])
-    module_portal_type='%s Module' % object_portal_type
-    portal_skins_folder='erp5_unittest'
-    object_title=object_portal_type
-    module_id="%s_module" % portal_id
-    module_title='%ss' % object_portal_type
+    portal = self.portal
+    portal_id = portal.getId()
+    object_portal_type = ' '.join(
+      part.capitalize() for part in portal_id.split('_')
+    )
+    module_portal_type = object_portal_type + ' Module'
+    portal_skins_folder = 'erp5_unittest'
+    module_id = portal_id + "_module"
 
     # Create module for testing
-    self.assertFalse(self.portal._getOb(module_id, None) is not None)
-    self.assertEqual(self.portal.portal_skins._getOb(portal_skins_folder, None),
-                     None)
-    self.assertEqual(self.portal.portal_types.getTypeInfo(module_portal_type),
-                     None)
-    self.assertEqual(self.portal.portal_types.getTypeInfo(object_portal_type),
-                     None)
-    self.portal.ERP5Site_createModule(module_portal_type=module_portal_type,
-                                      portal_skins_folder=portal_skins_folder,
-                                      object_portal_type=object_portal_type,
-                                      object_title=object_title,
-                                      module_id=module_id,
-                                      module_title=module_title)
-
-    # Test
+    self.assertEqual(portal._getOb(module_id, None), None)
     self.assertEqual(
-        module_id,
-        self.portal.getDefaultModule(object_portal_type).getId())
+      portal.portal_skins._getOb(portal_skins_folder, None),
+      None,
+    )
     self.assertEqual(
-        module_portal_type,
-        self.portal.getDefaultModule(object_portal_type).getPortalType())
+      portal.portal_types.getTypeInfo(module_portal_type),
+      None,
+    )
     self.assertEqual(
+      portal.portal_types.getTypeInfo(object_portal_type),
+      None,
+    )
+    portal.ERP5Site_createModule(
+      module_portal_type=module_portal_type,
+      portal_skins_folder=portal_skins_folder,
+      object_portal_type=object_portal_type,
+      object_title=object_portal_type,
+      module_id=module_id,
+      module_title=object_portal_type + 's',
+    )
+    module = portal._getOb(module_id)
+
+    # Test
+
+    for portal_type in (object_portal_type, module_portal_type):
+      self.assertEqual(
+        module,
+        portal.getDefaultModule(portal_type),
+      )
+      self.assertEqual(
+        module,
+        portal.getDefaultModuleValue(portal_type),
+      )
+      self.assertEqual(
         module_id,
-        self.portal.getDefaultModule(module_portal_type).getId())
-    self.assertEqual(
-        module_portal_type,
-        self.portal.getDefaultModule(module_portal_type).getPortalType())
+        portal.getDefaultModuleId(portal_type),
+      )
+
+    default = object()
+    for portal_type in (
+          object_portal_type + ' DoesNotExist',
+          module_portal_type + ' DoesNotExist',
+        ):
+      self.assertRaises(
+        ValueError,
+        portal.getDefaultModule,
+        portal_type,
+      )
+      # XXX: this behaviour may not be intentional, and differs a lot from how
+      # typical default values behave.
+      self.assertEqual(
+        module,
+        portal.getDefaultModule(portal_type, default=module_id)
+      )
+      self.assertIs(
+        default,
+        portal.getDefaultModuleValue(portal_type, default=default),
+      )
+      self.assertEqual(
+        default,
+        portal.getDefaultModuleId(portal_type, default=default),
+      )
+
+    # XXX: only_visible not testable here, because manager can see hidden
+    # allowed content types.
+    '''
+    module_portal_type_value = portal.portal_types[module_portal_type]
+    module_portal_type_value.setTypeHiddenContentTypeList(
+      module_portal_type_value.getTypeAllowedContentTypeList()
+    )
+    for portal_type in (object_portal_type, module_portal_type):
+      # Note: getDefaultModule does not support this semantic.
+      self.assertIs(
+        default,
+        portal.getDefaultModuleValue(
+          portal_type,
+          default=default,
+          only_visible=True,
+        ),
+      )
+      self.assertIs(
+        default,
+        portal.getDefaultModuleId(
+          portal_type,
+          default=default,
+          only_visible=True,
+        ),
+      )
+    '''
 
   def test_catalog_with_very_long_login_name(self, quiet=quiet, run=run_all_test):
     """Make sure that user with very long login name can find his document by catalog"""