Adjust changes according to MR review. Not ready to merge due to a c.…

…matchAll TypeError that seems to have been caused by merging RBS/main -> this

frontend/src/ofrak/remote_resource.js

@@ -468,13 +468,13 @@ export class RemoteResource extends Resource {
     return find_replace_results;
   }
 
-  async add_comment(optional_range, comment) {
+  async add_comment(optional_range, comment_text) {
     await fetch(`${this.uri}/add_comment`, {
       method: "POST",
       headers: {
         "Content-Type": "application/json",
       },
-      body: JSON.stringify([optional_range, comment]),
+      body: JSON.stringify([optional_range, comment_text]),
     }).then(async (r) => {
       if (!r.ok) {
         throw Error(JSON.stringify(await r.json(), undefined, 2));
@@ -508,13 +508,13 @@ export class RemoteResource extends Resource {
     await this.update_script();
   }
 
-  async delete_comment(optional_range, comment) {
+  async delete_comment(optional_range, comment_text) {
     await fetch(`${this.uri}/delete_comment`, {
       method: "POST",
       headers: {
         "Content-Type": "application/json",
       },
-      body: JSON.stringify([optional_range, comment]),
+      body: JSON.stringify([optional_range, comment_text]),
     }).then(async (r) => {
       if (!r.ok) {
         throw Error(JSON.stringify(await r.json(), undefined, 2));

frontend/src/ofrak/resource.js

@@ -365,11 +365,11 @@ export class Resource {
     throw new NotImplementedError("summarize_tree");
   }
 
-  async add_comment(optional_range, comment) {
+  async add_comment(optional_range, comment_text) {
     throw new NotImplementedError("add_comment");
   }
 
-  async delete_comment(optional_range, comment) {
+  async delete_comment(optional_range, comment_text) {
     throw new NotImplementedError("delete_comment");
   }
 

ofrak_core/ofrak/core/comments.py

@@ -47,8 +47,6 @@ async def modify(self, resource: Resource, config: AddCommentModifierConfig) ->
         except NotFoundError:
             comments = {}
 
-        # Here I'm appending appending overlapping comments with a new line.
-        # Overwriting comments that share a range is counter intuitive and not easily understood without digging into the code.
         if config.comment[0] not in comments:
             comments[config.comment[0]] = []
 
@@ -59,36 +57,24 @@ async def modify(self, resource: Resource, config: AddCommentModifierConfig) ->
 @dataclass
 class DeleteCommentModifierConfig(ComponentConfig):
     """
-    comment_range: Tuple[Optional[Range], comment_text=Optional[str]]
-    If comment_text is provided, deletes the matching comment with the same Optional[Range]
-    If comment_text is None, deletes ALL comments with the same Optional[Range]
+    If comment_text is provided, deletes the matching comment in that comment_range
+    If comment_text is None, deletes ALL comments in that comment_range
     """
 
-    comment_range: Tuple[Optional[Range], Optional[str]]
-
-    def __post_init__(self):
-        # Ensure there's always a two-element Tuple
-        if type(self.comment_range) == tuple:
-            # New format
-            self.comment_range = (*self.comment_range, None)[:2]
-        elif type(self.comment_range) == Range:
-            # Old format: Range
-            self.comment_range = (self.comment_range, None)
-        else:
-            # Old format: None (no Range provided)
-            self.comment_range = (None, None)
+    comment_range: Optional[Range]
+    comment_text: Optional[str] = None
 
 
 class DeleteCommentModifier(Modifier[DeleteCommentModifierConfig]):
     """
-    Modifier to delete a comment from a resource.
+    Modifier to delete comment(s) from a resource.
     """
 
     targets = ()
 
     async def modify(self, resource: Resource, config: DeleteCommentModifierConfig) -> None:
         """
-        Delete the comment associated with the given range.
+        Delete the comment(s) associated with the given range.
 
         :raises NotFoundError: if the comment range is not associated with a comment.
         """
@@ -97,24 +83,22 @@ async def modify(self, resource: Resource, config: DeleteCommentModifierConfig)
         except NotFoundError:
             comments = {}
         try:
-            if len(config.comment_range) == 1:
-                config.comment_range = (config.comment_range[0], None)
-
-            if config.comment_range[1] is None:
-                del comments[config.comment_range[0]]
+            if config.comment_text is None:
+                # Delete ALL comments in this range
+                del comments[config.comment_range]
             else:
-                comments[config.comment_range[0]].remove(config.comment_range[1])
-                # Clean up if this was the last comment at this range
-                if len(comments[config.comment_range[0]]) == 0:
-                    del comments[config.comment_range[0]]
+                comments[config.comment_range].remove(config.comment_text)
+                # Clean up if this was the last comment in this range
+                if len(comments[config.comment_range]) == 0:
+                    del comments[config.comment_range]
         except KeyError:
             raise NotFoundError(
-                f"Comment range {config.comment_range[0]} not found in "
+                f"Comment range {config.comment_range} not found in "
                 f"resource {resource.get_id().hex()}"
             )
         except ValueError:
             raise NotFoundError(
-                f"Comment {config.comment_range[1]} with range {config.comment_range[0]}"
+                f"Comment {config.comment_text} with range {config.comment_range}"
                 f" not found in resource {resource.get_id().hex()}"
             )
         resource.add_attributes(CommentsAttributes(comments=comments))

ofrak_core/ofrak/gui/server.py

@@ -735,19 +735,30 @@ async def add_comment(self, request: Request) -> Response:
     @exceptions_to_http(SerializedError)
     async def delete_comment(self, request: Request) -> Response:
         resource = await self._get_resource_for_request(request)
-        comment = self._serializer.from_pjson(
+        comment_data = self._serializer.from_pjson(
             await request.json(), Union[Tuple[Optional[Range], Optional[str]], Optional[Range]]
         )
+        comment_range: Optional[Range] = None
+        comment_text: Optional[str] = None
+
+        if type(comment_data) == tuple:
+            comment_range = comment_data[0]
+            comment_text = comment_data[1]
+        else:
+            comment_range = comment_data
+
         script_str = (
             """
         await {resource}.run"""
             f"""(
-            DeleteCommentModifier, DeleteCommentModifierConfig({comment})
+            DeleteCommentModifier, DeleteCommentModifierConfig({comment_range}, {comment_text})
         )"""
         )
         await self.script_builder.add_action(resource, script_str, ActionType.MOD)
         try:
-            result = await resource.run(DeleteCommentModifier, DeleteCommentModifierConfig(comment))
+            result = await resource.run(
+                DeleteCommentModifier, DeleteCommentModifierConfig(comment_range, comment_text)
+            )
             await self.script_builder.commit_to_script(resource)
         except Exception as e:
             await self.script_builder.clear_script_queue(resource)

ofrak_core/test_ofrak/components/test_comments.py

@@ -196,7 +196,9 @@ async def test_deleting_comments(executable_resource: Resource):
     # Test deletion of specific comments
     await executable_resource.run(
         DeleteCommentModifier,
-        DeleteCommentModifierConfig(comment_range=(Range(0, 1), "first range comment 1")),
+        DeleteCommentModifierConfig(
+            comment_range=Range(0, 1), comment_text="first range comment 1"
+        ),
     )
     comment_attributes = executable_resource.get_attributes(CommentsAttributes)
     comments = comment_attributes.comments
@@ -206,7 +208,9 @@ async def test_deleting_comments(executable_resource: Resource):
     # Test specific deletion of last comment in a range
     await executable_resource.run(
         DeleteCommentModifier,
-        DeleteCommentModifierConfig(comment_range=(Range(0, 1), "first range comment 2")),
+        DeleteCommentModifierConfig(
+            comment_range=Range(0, 1), comment_text="first range comment 2"
+        ),
     )
     comment_attributes = executable_resource.get_attributes(CommentsAttributes)
     comments = comment_attributes.comments
@@ -217,7 +221,7 @@ async def test_deleting_comments(executable_resource: Resource):
     # Test deletion of entire ranges with new DeleteCommentModifierConfig format
     await executable_resource.run(
         DeleteCommentModifier,
-        DeleteCommentModifierConfig(comment_range=(Range(1, 2), None)),
+        DeleteCommentModifierConfig(comment_range=Range(1, 2), comment_text=None),
     )
     comment_attributes = executable_resource.get_attributes(CommentsAttributes)
     assert get_comment_count(comment_attributes) == 5
@@ -255,12 +259,12 @@ async def test_deleting_non_existing_comment(executable_resource: Resource):
     with pytest.raises(NotFoundError):
         await executable_resource.run(
             DeleteCommentModifier,
-            DeleteCommentModifierConfig(comment_range=(None, "this doesn't exist")),
+            DeleteCommentModifierConfig(comment_range=None, comment_text="this doesn't exist"),
         )
 
     await executable_resource.run(
         DeleteCommentModifier,
-        DeleteCommentModifierConfig(comment_range=(None, "this exists")),
+        DeleteCommentModifierConfig(comment_range=None, comment_text="this exists"),
     )
 
     comment_attributes = executable_resource.get_attributes(CommentsAttributes)