From a0129d35be59d987025276a2b2d8e6d1e624d4df Mon Sep 17 00:00:00 2001
From: ewuerger <ernst.wuerger@gmail.com>
Date: Mon, 26 Aug 2024 04:47:00 +0200
Subject: [PATCH] fix: Apply suggestions from code review

---
 .../connectors/polarion_worker.py             | 62 +++++++++++++------
 .../converters/element_converter.py           |  6 --
 2 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/capella2polarion/connectors/polarion_worker.py b/capella2polarion/connectors/polarion_worker.py
index 1e221f24..1c04fe65 100644
--- a/capella2polarion/connectors/polarion_worker.py
+++ b/capella2polarion/connectors/polarion_worker.py
@@ -175,12 +175,7 @@ def compare_and_update_work_item(
             "Update work item %r for model element %s %r...", *log_args
         )
 
-        old_checksum = old.get_current_checksum() or "None"
-        if old_checksum[0] != "{":  # XXX: Remove in next release
-            old_checksums = {"__C2P__WORK_ITEM": old_checksum}
-        else:
-            old_checksums = json.loads(old_checksum)
-
+        old_checksums = {"__C2P__WORK_ITEM": old.get_current_checksum() or ""}
         new_checksum = new.get_current_checksum()
         assert new_checksum is not None
         new_checksums = json.loads(new_checksum)
@@ -211,7 +206,7 @@ def compare_and_update_work_item(
                 work_item_changed |= self.update_attachments(
                     new, old_checksums, new_checksums, old_attachments
                 )
-        except polarion_api.PolarionApiException as error:
+        except (polarion_api.PolarionApiException, ValueError) as error:
             logger.error(
                 "Updating attachments for WorkItem %r (%s %s) failed. %s",
                 *log_args,
@@ -338,19 +333,25 @@ def update_attachments(
         Returns True if new attachments were created. After execution
         all attachments of the new work item should have IDs.
         """
-        new_attachment_dict = {
-            attachment.file_name: attachment
-            for attachment in new.attachments
-            if attachment.file_name is not None
-        }
-        old_attachment_dict = {
-            attachment.file_name: attachment
-            for attachment in old_attachments
-            if attachment.file_name is not None
-        }
+        new_attachment_dict: dict[str, polarion_api.WorkItemAttachment] = {}
+        for attachment in new.attachments:
+            if attachment.file_name is None:
+                raise ValueError(
+                    f"Found new attachment without filename: {new.id!r} with "
+                    + attachment.id
+                )
+            new_attachment_dict[attachment.file_name] = attachment
 
-        created = False
+        old_attachment_dict: dict[str, polarion_api.WorkItemAttachment] = {}
+        for attachment in old_attachments:
+            if attachment.file_name is None:
+                raise ValueError(
+                    f"Found old attachment without filename: {new!r} with"
+                    + attachment.id
+                )
+            old_attachment_dict[attachment.file_name] = attachment
 
+        created = False
         for attachment in old_attachments:
             if attachment not in old_attachment_dict.values():
                 logger.error(
@@ -409,10 +410,12 @@ def get_missing_link_ids(
     ) -> dict[str, polarion_api.WorkItemLink]:
         """Return an ID-Link dict of links present in left and not in right."""
         left_id_map = {
-            CapellaPolarionWorker._get_link_id(link): link for link in left
+            CapellaPolarionWorker._get_link_id(link): link
+            for link in CapellaPolarionWorker._filter_all_link_ids(left)
         }
         right_id_map = {
-            CapellaPolarionWorker._get_link_id(link): link for link in right
+            CapellaPolarionWorker._get_link_id(link): link
+            for link in CapellaPolarionWorker._filter_all_link_ids(right)
         }
         return {
             lid: left_id_map[lid]
@@ -426,6 +429,25 @@ def _get_link_id(link: polarion_api.WorkItemLink) -> str:
             secondary_id = f"{link.secondary_work_item_project}/{secondary_id}"
         return "/".join((link.primary_work_item_id, link.role, secondary_id))
 
+    @staticmethod
+    def _filter_all_link_ids(
+        links: cabc.Iterable[polarion_api.WorkItemLink],
+    ) -> cabc.Iterator[polarion_api.WorkItemLink]:
+        for link in links:
+            if link.primary_work_item_id is None:
+                logger.error(  # type: ignore[unreachable]
+                    "Found Work Item Link without source ID: %r", link
+                )
+                continue
+
+            if link.secondary_work_item_id is None:
+                logger.error(  # type: ignore[unreachable]
+                    "Found Work Item Link without target ID: %r", link
+                )
+                continue
+
+            yield link
+
     def compare_and_update_work_items(
         self, converter_session: data_session.ConverterSession
     ) -> None:
diff --git a/capella2polarion/converters/element_converter.py b/capella2polarion/converters/element_converter.py
index 51b3409c..c96eaf1f 100644
--- a/capella2polarion/converters/element_converter.py
+++ b/capella2polarion/converters/element_converter.py
@@ -20,8 +20,6 @@
 from capellambse import helpers as chelpers
 from capellambse.model import common
 from capellambse.model import diagram as diag
-from capellambse.model.crosslayer import interaction
-from capellambse.model.layers import oa
 from lxml import etree
 
 from capella2polarion import data_models
@@ -33,10 +31,6 @@
 )
 RE_CAMEL_CASE_2ND_WORD_PATTERN = re.compile(r"([a-z]+)([A-Z][a-z]+)")
 
-PrePostConditionElement: t.TypeAlias = t.Union[
-    oa.OperationalCapability, interaction.Scenario
-]
-
 logger = logging.getLogger(__name__)
 C2P_IMAGE_PREFIX = "__C2P__"
 JINJA_RENDERED_IMG_CLS = "jinja-rendered-image"