Fix chapter tracking to count unique chapters instead of invocations (#241)

* Initial plan

* Fix added_into_chapters to track unique chapter IDs

Co-authored-by: miroslavpojer <[email protected]>

* Address code review feedback - improve docstrings

Co-authored-by: miroslavpojer <[email protected]>

* Fix integration tests - add added_into_chapters to mock Record

Co-authored-by: miroslavpojer <[email protected]>

* Refactor chapter tracking methods in Record classes for consistency

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: miroslavpojer <[email protected]>
Co-authored-by: miroslavpojer <[email protected]>

Author: Copilot

release_notes_generator/chapters/custom_chapters.py

@@ -99,6 +99,7 @@ def populate(self, records: dict[str, Record]) -> None:
                 # Quick intersection check
                 if any(lbl in ch.labels for lbl in record_labels):
                     if record_id not in ch.rows:
+                        record.add_to_chapter_presence(ch.title)
                         ch.add_row(record_id, record.to_chapter_row(True))
                         # Track for backward compatibility (not used for gating anymore)
                         if record_id not in self.populated_record_numbers_list:

release_notes_generator/chapters/service_chapters.py

@@ -135,6 +135,7 @@ def populate(self, records: dict[str, Record]) -> None:
                         logger.debug("Skipping open HierarchyIssueRecord %s (pr_count=%d)", record_id, pr_count)
                     elif is_issue_like and pr_count > 0:
                         # Open issue/sub-issue with linked PRs → add to the specific chapter
+                        record.add_to_chapter_presence(MERGED_PRS_LINKED_TO_NOT_CLOSED_ISSUES)
                         self.chapters[MERGED_PRS_LINKED_TO_NOT_CLOSED_ISSUES].add_row(
                             record_id, record.to_chapter_row()
                         )
@@ -145,6 +146,7 @@ def populate(self, records: dict[str, Record]) -> None:
                         pass
                 else:
                     if record_id not in self.used_record_numbers:
+                        record.add_to_chapter_presence(OTHERS_NO_TOPIC)
                         self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
                         self.used_record_numbers.append(record_id)
 
@@ -164,6 +166,7 @@ def __populate_closed_issues(self, record: IssueRecord, record_id: int | str) ->
         pulls_count = record.pull_requests_count()
 
         if pulls_count == 0:
+            record.add_to_chapter_presence(CLOSED_ISSUES_WITHOUT_PULL_REQUESTS)
             self.chapters[CLOSED_ISSUES_WITHOUT_PULL_REQUESTS].add_row(record_id, record.to_chapter_row())
             self.used_record_numbers.append(record_id)
             populated = True
@@ -174,6 +177,7 @@ def __populate_closed_issues(self, record: IssueRecord, record_id: int | str) ->
             if self.__is_row_present(record_id) and not self.duplicity_allowed():
                 return
 
+            record.add_to_chapter_presence(CLOSED_ISSUES_WITHOUT_USER_DEFINED_LABELS)
             self.chapters[CLOSED_ISSUES_WITHOUT_USER_DEFINED_LABELS].add_row(record_id, record.to_chapter_row())
             self.used_record_numbers.append(record_id)
             populated = True
@@ -189,6 +193,7 @@ def __populate_closed_issues(self, record: IssueRecord, record_id: int | str) ->
             if record_id in self.used_record_numbers:
                 return
 
+            record.add_to_chapter_presence(OTHERS_NO_TOPIC)
             self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
             self.used_record_numbers.append(record_id)
 
@@ -209,6 +214,7 @@ def __populate_pr(self, record: PullRequestRecord, record_id: int | str) -> None
                 if self.__is_row_present(record_id) and not self.duplicity_allowed():
                     return
 
+                record.add_to_chapter_presence(MERGED_PRS_WITHOUT_ISSUE_AND_USER_DEFINED_LABELS)
                 self.chapters[MERGED_PRS_WITHOUT_ISSUE_AND_USER_DEFINED_LABELS].add_row(
                     record_id, record.to_chapter_row()
                 )
@@ -219,6 +225,7 @@ def __populate_pr(self, record: PullRequestRecord, record_id: int | str) -> None
                 if self.__is_row_present(record_id) and not self.duplicity_allowed():
                     return
 
+                record.add_to_chapter_presence(MERGED_PRS_LINKED_TO_NOT_CLOSED_ISSUES)
                 self.chapters[MERGED_PRS_LINKED_TO_NOT_CLOSED_ISSUES].add_row(record_id, record.to_chapter_row())
                 self.used_record_numbers.append(record_id)
 
@@ -229,6 +236,7 @@ def __populate_pr(self, record: PullRequestRecord, record_id: int | str) -> None
                 if record_id in self.used_record_numbers:
                     return
 
+                record.add_to_chapter_presence(OTHERS_NO_TOPIC)
                 self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
                 self.used_record_numbers.append(record_id)
 
@@ -241,6 +249,7 @@ def __populate_pr(self, record: PullRequestRecord, record_id: int | str) -> None
             if self.__is_row_present(record_id) and not self.duplicity_allowed():
                 return
 
+            record.add_to_chapter_presence(CLOSED_PRS_WITHOUT_ISSUE_AND_USER_DEFINED_LABELS)
             self.chapters[CLOSED_PRS_WITHOUT_ISSUE_AND_USER_DEFINED_LABELS].add_row(record_id, record.to_chapter_row())
             self.used_record_numbers.append(record_id)
 
@@ -252,6 +261,7 @@ def __populate_pr(self, record: PullRequestRecord, record_id: int | str) -> None
                 return
 
             # not record.is_present_in_chapters:
+            record.add_to_chapter_presence(OTHERS_NO_TOPIC)
             self.chapters[OTHERS_NO_TOPIC].add_row(record_id, record.to_chapter_row())
             self.used_record_numbers.append(record_id)
 
@@ -262,6 +272,7 @@ def __populate_direct_commit(self, record: CommitRecord, record_id: int | str) -
         @param record: The CommitRecord object representing the direct commit.
         @return: None
         """
+        record.add_to_chapter_presence(DIRECT_COMMITS)
         self.chapters[DIRECT_COMMITS].add_row(record_id, record.to_chapter_row())
         self.used_record_numbers.append(record_id)
 

release_notes_generator/model/record/commit_record.py

@@ -61,9 +61,7 @@ def commit(self) -> Commit:
     # methods - override Record methods
 
     def to_chapter_row(self, add_into_chapters: bool = True) -> str:
-        if add_into_chapters:
-            self.added_into_chapters()
-        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.present_in_chapters() > 1 else ""
+        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.chapter_presence_count() > 1 else ""
 
         # collecting values for formatting
         commit_message = self._commit.commit.message.replace("\n", " ")

release_notes_generator/model/record/hierarchy_issue_record.py

@@ -116,9 +116,7 @@ def get_labels(self) -> list[str]:
     # methods - override ancestor methods
     def to_chapter_row(self, add_into_chapters: bool = True) -> str:
         logger.debug("Rendering hierarchy issue row for issue #%s", self.issue.number)
-        if add_into_chapters:
-            self.added_into_chapters()
-        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.present_in_chapters() > 1 else ""
+        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.chapter_presence_count() > 1 else ""
         format_values: dict[str, Any] = {}
 
         # collect format values

release_notes_generator/model/record/issue_record.py

@@ -128,9 +128,7 @@ def find_issue(self, issue_number: int) -> Optional["IssueRecord"]:
         return None
 
     def to_chapter_row(self, add_into_chapters: bool = True) -> str:
-        if add_into_chapters:
-            self.added_into_chapters()
-        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.present_in_chapters() > 1 else ""
+        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.chapter_presence_count() > 1 else ""
         format_values: dict[str, Any] = {}
 
         # collect format values

release_notes_generator/model/record/pull_request_record.py

@@ -122,10 +122,7 @@ def get_labels(self) -> list[str]:
         return self.labels
 
     def to_chapter_row(self, add_into_chapters: bool = True) -> str:
-        if add_into_chapters:
-            self.added_into_chapters()
-
-        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.present_in_chapters() > 1 else ""
+        row_prefix = f"{ActionInputs.get_duplicity_icon()} " if self.chapter_presence_count() > 1 else ""
         format_values: dict[str, Any] = {}
 
         # collecting values for formatting

release_notes_generator/model/record/record.py

@@ -15,7 +15,7 @@
 #
 
 """
-This module contains the BaseChapters class, which is responsible for representing the base chapters.
+Defines the abstract base `Record` type used by the release notes generator.
 """
 
 import logging
@@ -37,7 +37,7 @@ class Record(metaclass=ABCMeta):
     RELEASE_NOTE_LINE_MARKS: list[str] = ["-", "*", "+"]
 
     def __init__(self, labels: Optional[list[str]] = None, skip: bool = False):
-        self._present_in_chapters = 0
+        self._chapters_present_in: set[str] = set()
         self._skip = skip
         self._is_cross_repo: bool = False
         self._is_release_note_detected: Optional[bool] = None
@@ -48,11 +48,12 @@ def __init__(self, labels: Optional[list[str]] = None, skip: bool = False):
     @property
     def is_present_in_chapters(self) -> bool:
         """
-        Checks if the record is present in any chapter.
+        Checks if the record is present in at least one chapter.
+
         Returns:
             bool: True if the record is present in at least one chapter, False otherwise.
         """
-        return self._present_in_chapters > 0
+        return len(self._chapters_present_in) > 0
 
     @property
     def is_cross_repo(self) -> bool:
@@ -190,21 +191,23 @@ def get_rls_notes(self, line_marks: Optional[list[str]] = None) -> str:
 
     # shared methods
 
-    def added_into_chapters(self) -> None:
+    def add_to_chapter_presence(self, chapter_id: str) -> None:
         """
-        Increments the count of chapters in which the record is present.
-        Returns: None
+        Marks this record as present in the given chapter.
+
+        Parameters:
+            chapter_id (str): The unique identifier of the chapter.
         """
-        # TODO - fix in #191
-        self._present_in_chapters += 1
+        self._chapters_present_in.add(chapter_id)
 
-    def present_in_chapters(self) -> int:
+    def chapter_presence_count(self) -> int:
         """
-        Gets the count of chapters in which the record is present.
+        Gets the number of unique chapters in which the record is present.
+
         Returns:
-            int: The count of chapters in which the record is present.
+            int: The count of unique chapters containing this record.
         """
-        return self._present_in_chapters
+        return len(self._chapters_present_in)
 
     def contains_min_one_label(self, labels: list[str]) -> bool:
         """

tests/integration/test_release_notes_snapshot.py

@@ -11,13 +11,17 @@ def __init__(self, _id: str, _labels: list[str]):
             self.labels = _labels
             self.skip = False
             self.is_present_in_chapters = False
+            self._chapters = set()
 
         def contains_change_increment(self):  # matches code expectation in populate
             return True
 
         def to_chapter_row(self, _include_prs: bool):  # simplified row rendering
             return f"{self.id} row"
 
+        def add_to_chapter_presence(self, chapter_id: str):  # track chapter additions
+            self._chapters.add(chapter_id)
+
     return R(record_id, labels)
 
 

tests/unit/release_notes_generator/model/test_commit_record.py

@@ -34,11 +34,15 @@ def test_to_chapter_row_duplicate_with_icon(monkeypatch, mock_commit):
         lambda: "[D]",
     )
     rec = CommitRecord(mock_commit)
+    # Simulate adding to first chapter
+    rec.add_to_chapter_presence("chapter1")
     first = rec.to_chapter_row(True)
+    # Simulate adding to second chapter
+    rec.add_to_chapter_presence("chapter2")
     second = rec.to_chapter_row(True)
     assert not first.startswith("[D] ")
     assert second.startswith("[D] ")
-    assert rec.present_in_chapters() == 2
+    assert rec.chapter_presence_count() == 2
 
 def test_to_chapter_row_with_release_notes_injected(monkeypatch, mock_commit):
     # Force contains_release_notes to True and provide fake release notes

tests/unit/release_notes_generator/model/test_record.py

@@ -70,7 +70,7 @@ def contains_change_increment(self) -> bool:
 def test_is_present_in_chapters():
     rec = DummyRecord()
     assert not rec.is_present_in_chapters
-    rec.added_into_chapters()
+    rec.add_to_chapter_presence("chapter1")
     assert rec.is_present_in_chapters
 
 def test_skip_property():
@@ -81,10 +81,20 @@ def test_skip_property():
 
 def test_present_in_chapters_count():
     rec = DummyRecord()
-    assert rec.present_in_chapters() == 0
-    rec.added_into_chapters()
-    rec.added_into_chapters()
-    assert rec.present_in_chapters() == 2
+    assert rec.chapter_presence_count() == 0
+    rec.add_to_chapter_presence("chapter1")
+    rec.add_to_chapter_presence("chapter2")
+    assert rec.chapter_presence_count() == 2
+
+def test_present_in_chapters_unique():
+    """Test that adding the same chapter multiple times doesn't increase the count."""
+    rec = DummyRecord()
+    assert rec.chapter_presence_count() == 0
+    rec.add_to_chapter_presence("chapter1")
+    rec.add_to_chapter_presence("chapter1")  # Same chapter again
+    assert rec.chapter_presence_count() == 1  # Should still be 1
+    rec.add_to_chapter_presence("chapter2")
+    assert rec.chapter_presence_count() == 2
 
 def test_contains_min_one_label():
     rec = DummyRecord(labels=["bug", "feature"])