Merge pull request #35 from plone/maurits-fixes

Various fixes

news/35.bugfix.1

@@ -0,0 +1,2 @@
+Do not export or import translations when `plone.app.multilingual` is not available.
+[maurits]

news/35.bugfix.2

@@ -0,0 +1,2 @@
+Do not export or import discussions/comments when `plone.app.discussion` is not available.
+[maurits]

news/35.bugfix.3

@@ -0,0 +1,2 @@
+Add a fixer for the `allow_discussion` key: this should only contain True or False when this is explicitly set on the object.
+[maurits]

news/35.bugfix.4

@@ -0,0 +1,2 @@
+Disallowlisted portlets were not imported when there was no accompanying change in the actual portlet list.
+[maurits]

news/35.bugfix.5

@@ -0,0 +1,3 @@
+Renamed `blacklisted_status` key to `blocked_status` to be sensitive.
+We still read the old key for backwards compatibility.
+[maurits]

news/35.bugfix.6

@@ -0,0 +1,3 @@
+Export adds a newline at the end of all files.
+This matches the `.editorconfig` settings that we have in most Plone packages.
+[maurits]

src/plone/exportimport/exporters/__init__.py

@@ -69,7 +69,8 @@ def export_site(self, path: Optional[Path] = None) -> List[Path]:
         with hooks.site(self.site):
             for exporter_name, exporter in self.exporters.items():
                 logger.debug(f"Exporting {self.site} with {exporter_name} to {path}")
-                paths.extend(exporter.export_data(path))
+                new_paths = exporter.export_data(path)
+                paths.extend(new_paths)
         return paths
 
 

src/plone/exportimport/exporters/base.py

@@ -44,6 +44,12 @@ def _dump(self, data: dict, filepath: Path = None) -> Path:
         path_utils.get_parent_folder(filepath)
         with open(filepath, "w") as fh:
             json.dump(data, fh, indent=2, sort_keys=True)
+            # json.dump does not add a newline at the end of the file, so we
+            # explicitly do it.  Otherwise when you manually edit a file and
+            # you use an editor that respects the standard `.editorconfig`
+            # that we have in most Plone packages, you always get a diff because
+            # your editor has automatically added a newline at the end.
+            fh.write("\n")
         return filepath
 
     def dump(self) -> List[Path]:

src/plone/exportimport/exporters/configure.zcml

@@ -33,19 +33,22 @@
       for="plone.base.interfaces.siteroot.IPloneSiteRoot"
       name="plone.exporter.relations"
       />
-  <adapter
-      factory=".translations.TranslationsExporter"
-      provides="plone.exportimport.interfaces.INamedExporter"
-      for="plone.base.interfaces.siteroot.IPloneSiteRoot"
-      name="plone.exporter.translations"
-      />
-  <adapter
-      factory=".discussions.DiscussionsExporter"
-      provides="plone.exportimport.interfaces.INamedExporter"
-      for="plone.base.interfaces.siteroot.IPloneSiteRoot"
-      name="plone.exporter.discussions"
-      />
-
+  <configure zcml:condition="installed plone.app.multilingual">
+    <adapter
+        factory=".translations.TranslationsExporter"
+        provides="plone.exportimport.interfaces.INamedExporter"
+        for="plone.base.interfaces.siteroot.IPloneSiteRoot"
+        name="plone.exporter.translations"
+        />
+  </configure>
+  <configure zcml:condition="installed plone.app.discussion">
+    <adapter
+        factory=".discussions.DiscussionsExporter"
+        provides="plone.exportimport.interfaces.INamedExporter"
+        for="plone.base.interfaces.siteroot.IPloneSiteRoot"
+        name="plone.exporter.discussions"
+        />
+  </configure>
   <configure zcml:condition="installed plone.app.portlets">
     <adapter
         factory=".portlets.PortletsExporter"

src/plone/exportimport/exporters/discussions.py

@@ -22,7 +22,7 @@ def dump(self) -> List[Path]:
         """Serialize object and dump it to disk."""
         if not HAS_DISCUSSION:
             logger.debug("- Discussions: Skipping (plone.app.discussion not installed)")
-            return
+            return []
 
         from plone.exportimport.utils import discussions as utils
 

src/plone/exportimport/importers/configure.zcml

@@ -33,19 +33,22 @@
       for="plone.base.interfaces.siteroot.IPloneSiteRoot"
       name="plone.importer.relations"
       />
-  <adapter
-      factory=".translations.TranslationsImporter"
-      provides="plone.exportimport.interfaces.INamedImporter"
-      for="plone.base.interfaces.siteroot.IPloneSiteRoot"
-      name="plone.importer.translations"
-      />
-  <adapter
-      factory=".discussions.DiscussionsImporter"
-      provides="plone.exportimport.interfaces.INamedImporter"
-      for="plone.base.interfaces.siteroot.IPloneSiteRoot"
-      name="plone.importer.discussions"
-      />
-
+  <configure zcml:condition="installed plone.app.multilingual">
+    <adapter
+        factory=".translations.TranslationsImporter"
+        provides="plone.exportimport.interfaces.INamedImporter"
+        for="plone.base.interfaces.siteroot.IPloneSiteRoot"
+        name="plone.importer.translations"
+        />
+  </configure>
+  <configure zcml:condition="installed plone.app.discussion">
+    <adapter
+        factory=".discussions.DiscussionsImporter"
+        provides="plone.exportimport.interfaces.INamedImporter"
+        for="plone.base.interfaces.siteroot.IPloneSiteRoot"
+        name="plone.importer.discussions"
+        />
+  </configure>
   <configure zcml:condition="installed plone.app.portlets">
     <adapter
         factory=".portlets.PortletsImporter"

src/plone/exportimport/utils/content/export_helpers.py

@@ -143,6 +143,33 @@ def fix_root_uid(
     return json.loads(item_str)
 
 
+def fix_allow_discussion(
+    item: dict, obj: DexterityContent, config: types.ExporterConfig
+) -> dict:
+    """Fix allow_discussion key.
+
+    Currently, plone.restapi always adds the 'allow_discussion' key.
+    This contains either True or False, based on various facts:
+
+    * Is the plone.app.discussion package available?
+    * Is the plone.app.discussion add-on activated?
+    * Is discussion globally allowed?
+    * Is discussion allowed on this portal type?
+    * Is discussion explicitly allowed on this object?
+
+    For exporting we only want the last one.
+    Otherwise when one of the other facts is not true at the moment of export,
+    then all content would get allow_discussion=false.
+    Then when someone imports it, and afterwards wants to globally allow discussions,
+    they would need to go through all content and explicitly set allow_discussion
+    to true or to none.
+
+    So: here we set the value to neutral (None), unless it is explicitly set.
+    """
+    item["allow_discussion"] = getattr(aq_base(obj), "allow_discussion", None)
+    return item
+
+
 def add_constrains_info(obj: DexterityContent, config: types.ExporterConfig) -> dict:
     """Return constrains info for an object."""
     key = settings.SERIALIZER_CONSTRAINS_KEY
@@ -268,6 +295,7 @@ def fixers() -> List[types.ExportImportHelper]:
         fix_blocks,
         fix_language,
         fix_root_uid,
+        fix_allow_discussion,
     ]
     for func in funcs:
         fixers.append(

src/plone/exportimport/utils/portlets.py

@@ -1,4 +1,3 @@
-from collections import defaultdict
 from plone import api
 from plone.app.portlets.interfaces import IPortletTypeInterface
 from plone.app.textfield.value import RichTextValue
@@ -28,6 +27,8 @@
 from zope.globalrequest import getRequest
 from zope.interface import providedBy
 
+import warnings
+
 
 def portlets_in_context(
     context: DexterityContent, uid: Optional[str] = None
@@ -41,9 +42,9 @@ def portlets_in_context(
     portlets = export_local_portlets(context)
     if portlets:
         result["portlets"] = portlets
-    blacklist = export_portlets_blacklist(context)
-    if blacklist:
-        result["blacklist_status"] = blacklist
+    blocked = export_blocked_portlets(context)
+    if blocked:
+        result["blocked_status"] = blocked
     if result:
         result.update(
             {
@@ -128,8 +129,8 @@ def export_local_portlets(obj: DexterityContent) -> dict:
     return items
 
 
-def export_portlets_blacklist(obj: DexterityContent) -> List[dict]:
-    """Export portlets blacklist for one content object."""
+def export_blocked_portlets(obj: DexterityContent) -> List[dict]:
+    """Export portlets blocked for one content object."""
     results = []
     for manager_name, manager in getUtilitiesFor(IPortletManager):
         assignable = queryMultiAdapter((obj, manager), ILocalPortletAssignmentManager)
@@ -155,25 +156,47 @@ def export_portlets_blacklist(obj: DexterityContent) -> List[dict]:
     return results
 
 
-def _filter_portlets_registrations(base: dict, registrations: dict) -> dict:
-    """Filter new registration with the current registered portlets."""
-    results = defaultdict(dict)
+def _has_new_registrations(base: dict, registrations: dict) -> bool:
+    """Are the new registrations additions/changes to the current ones?
+
+    This is for both portlets and blocked portlets.
+    The import code does not handle removals, so we are not interested in those.
+    """
     key = "portlets"
     current = base.get(key, {})
     to_register = registrations.get(key, {})
     for manager_id, assignments in to_register.items():
         if manager_id not in current:
-            results[manager_id] = assignments
-            continue
+            return True
         manager = current[manager_id]
         for assignment in assignments:
-            new_assignments = []
-            if assignment in manager:
-                continue
-            new_assignments.append(assignment)
-        if new_assignments:
-            results[manager_id] = new_assignments
-    return results
+            if assignment not in manager:
+                return True
+
+    key = "blocked_status"
+    current = base.get(key, [])
+    to_register = registrations.get(key, [])
+    for new_reg in to_register:
+        # Each new_reg is a dict with keys category, manager and status.
+        # Try to find the same registration in the current registrations.
+        found = False
+        for old_reg in current:
+            same = True
+            for key, value in new_reg.items():
+                if old_reg.get(key) != value:
+                    same = False
+                    break
+            if same:
+                # All keys/values are the same.
+                found = True
+                break
+        if not found:
+            # We did not find a duplicate in the current registrations,
+            # so there is a difference.
+            return True
+
+    # no changes
+    return False
 
 
 def set_portlets(data: list) -> int:
@@ -191,7 +214,14 @@ def set_portlets(data: list) -> int:
                 )
                 continue
         existing_registrations = portlets_in_context(obj, item_uid)
-        new_registrations = _filter_portlets_registrations(existing_registrations, item)
+        old_key = "blacklist_status"
+        new_key = "blocked_status"
+        if old_key in item and new_key not in item:
+            warnings.warn(
+                f"{old_key} is deprecated, please use {new_key}.", DeprecationWarning
+            )
+            item[new_key] = item.pop(old_key)
+        new_registrations = _has_new_registrations(existing_registrations, item)
         if new_registrations:
             registered_portlets = import_local_portlets(obj, item)
             results += registered_portlets
@@ -264,18 +294,18 @@ def import_local_portlets(obj: DexterityContent, item: dict) -> int:
             )
             results += 1
 
-    for blacklist_status in item.get("blacklist_status", []):
-        status: bool = blacklist_status["status"].lower() == "block"
-        manager_name = blacklist_status["manager"]
-        category = blacklist_status["category"]
+    for blocked_status in item.get("blocked_status", []):
+        status: bool = blocked_status["status"].lower() == "block"
+        manager_name = blocked_status["manager"]
+        category = blocked_status["category"]
         manager = queryUtility(IPortletManager, manager_name)
         if not manager:
             logger.info(f"No portlet manager {manager_name}")
             continue
         assignable = queryMultiAdapter((obj, manager), ILocalPortletAssignmentManager)
         assignable.setBlacklistStatus(category, status)
         logger.info(
-            f"Added blacklist entry {category} ({status}) to {manager_name} of {obj.absolute_url()}"
+            f"Added blocked entry {category} ({status}) to {manager_name} of {obj.absolute_url()}"
         )
         results += 1
 

src/plone/exportimport/utils/translations.py

@@ -115,10 +115,10 @@ def link_translations(
 
 def set_translations(data: List[dict]) -> List[dict]:
     """Process a list of translations and add them to the Plone site."""
+    results = []
     if not HAS_MULTILINGUAL:
         logger.warning("- Translation: Skipping (plone.app.multilingual not installed)")
-        return
-    results = []
+        return results
     for item in data:
         translation_group = _parse_translation_group(item)
         canonical = translation_group["canonical"]

tests/_resources/base_import/portlets.json

@@ -66,7 +66,7 @@
   },
   {
     "@id": "http://localhost:8080/plone.pdf",
-    "blacklist_status": [
+    "blocked_status": [
       {
         "category": "group",
         "manager": "plone.rightcolumn",

tests/_resources/portlets_import/portlets.json

@@ -1,7 +1,7 @@
 [
   {
     "@id": "http://localhost:8080/Plone",
-    "blacklist_status": [
+    "blocked_status": [
       {
         "category": "context",
         "manager": "plone.rightcolumn",

tests/exporters/test_exporters_portlets.py

@@ -50,15 +50,35 @@ def test_portlets_is_exported(self, export_path, paths_as_relative, path):
         "key,value_type",
         [
             ["@id", str],
-            ["portlets", dict],
             ["UID", str],
         ],
     )
-    def test_portlets_content(self, export_path, load_json, key, value_type):
+    def test_portlets_content_general(self, export_path, load_json, key, value_type):
         exporter = self.exporter
         exporter.export_data(base_path=export_path)
         data = load_json(base_path=export_path, path="portlets.json")
         assert isinstance(data, list)
         portlet = data[0]
         assert key in portlet
         assert isinstance(portlet[key], value_type)
+
+    @pytest.mark.parametrize(
+        "key,value_type",
+        [
+            ["portlets", dict],
+            ["blocked_status", list],
+        ],
+    )
+    def test_portlets_content_specific(self, export_path, load_json, key, value_type):
+        exporter = self.exporter
+        exporter.export_data(base_path=export_path)
+        data = load_json(base_path=export_path, path="portlets.json")
+        assert isinstance(data, list)
+        # We expect at least one to have a blocked_status, and another to have portlets.
+        # The order could possibly differ, so look for the first one that matches.
+        for portlet in data:
+            if key in portlet:
+                assert isinstance(portlet[key], value_type)
+                return
+        # If we end up here, the key was found nowhere.
+        assert False, f"{key} key not found in anywhere in {data}"