Merge pull request #1716 from FrNecas/frnecas-reporting-duplicates

Do not repeat custom copr restriction message

Some slight refactoring of duplicate message check into status reporter using an optional argument + then using it in appropriate places.
TODO:

 Write new tests or update the old ones to cover new functionality.
 Update doc-strings where appropriate.

Fixes #1686
RELEASE NOTES BEGIN
Packit now won't repeatedly comment in pull requests about the need to migrate configuration of allowed forge projects to Copr.
RELEASE NOTES END

Reviewed-by: Laura Barcziová <None>
Reviewed-by: František Nečas <[email protected]>
Reviewed-by: Matej Focko <None>
Reviewed-by: None <None>

packit_service/worker/handlers/copr.py

@@ -63,7 +63,7 @@
     RetriableJobHandler,
 )
 from packit_service.worker.monitoring import measure_time
-from packit_service.worker.reporting import BaseCommitStatus
+from packit_service.worker.reporting import BaseCommitStatus, DuplicateCheckMode
 from packit_service.worker.result import TaskResults
 
 logger = logging.getLogger(__name__)
@@ -339,20 +339,6 @@ class CoprBuildEndHandler(AbstractCoprBuildReportHandler):
     topic = "org.fedoraproject.prod.copr.build.end"
     task_name = TaskName.copr_build_end
 
-    def was_last_packit_comment_with_congratulation(self):
-        """
-        Check if the last comment by the packit app
-        was about successful build to not duplicate it.
-
-        :return: bool
-        """
-        comments = self.project.get_pr(self.copr_event.pr_id).get_comments(reverse=True)
-        for comment in comments:
-            if comment.author.startswith("packit-as-a-service"):
-                return "Congratulations!" in comment.body
-        # if there is no comment from p-s
-        return False
-
     def set_srpm_url(self) -> None:
         # TODO how to do better
         srpm_build = (
@@ -448,7 +434,6 @@ def report_successful_build(self):
             == JobConfigTriggerType.pull_request
             and self.copr_event.pr_id
             and isinstance(self.project, (GithubProject, GitlabProject))
-            and not self.was_last_packit_comment_with_congratulation()
             and self.job_config.notifications.pull_request.successful_build
         ):
             msg = (
@@ -460,7 +445,9 @@ def report_successful_build(self):
                 "* And now you can install the packages.\n"
                 "\nPlease note that the RPMs should be used only in a testing environment."
             )
-            self.project.get_pr(self.copr_event.pr_id).comment(msg)
+            self.copr_build_helper.status_reporter.comment(
+                msg, duplicate_check=DuplicateCheckMode.check_last_comment
+            )
 
         url = get_copr_build_info_url(self.build.id)
 

packit_service/worker/handlers/testing_farm.py

@@ -393,6 +393,7 @@ def run(self) -> TaskResults:
         status_reporter = StatusReporter.get_instance(
             project=self.project,
             commit_sha=self.data.commit_sha,
+            packit_user=self.service_config.get_github_account_name(),
             trigger_id=trigger.id if trigger else None,
             pr_id=self.data.pr_id,
         )

packit_service/worker/helpers/build/copr_build.py

@@ -57,7 +57,7 @@
 from packit_service.worker.helpers.build.build_helper import BaseBuildJobHelper
 from packit_service.worker.events import EventData
 from packit_service.worker.monitoring import Pushgateway, measure_time
-from packit_service.worker.reporting import BaseCommitStatus
+from packit_service.worker.reporting import BaseCommitStatus, DuplicateCheckMode
 from packit_service.worker.result import TaskResults
 
 logger = logging.getLogger(__name__)
@@ -409,7 +409,10 @@ def check_if_custom_copr_can_be_used_and_report(self) -> bool:
                 forge_project=self.forge_project,
                 copr_settings_url=self.copr_settings_url,
             )
-            self.status_reporter.comment(body=markdown_content)
+            self.status_reporter.comment(
+                body=markdown_content,
+                duplicate_check=DuplicateCheckMode.check_all_comments,
+            )
             return True
 
         self.report_status_to_build(

packit_service/worker/helpers/job_helper.py

@@ -141,6 +141,7 @@ def status_reporter(self) -> StatusReporter:
             self._status_reporter = StatusReporter.get_instance(
                 project=self.project,
                 commit_sha=self.metadata.commit_sha,
+                packit_user=self.service_config.get_github_account_name(),
                 trigger_id=trigger.id if trigger else None,
                 pr_id=self.metadata.pr_id,
             )

packit_service/worker/reporting.py

@@ -2,7 +2,7 @@
 # SPDX-License-Identifier: MIT
 
 import logging
-from enum import Enum
+from enum import Enum, auto
 from typing import Optional, Union, Dict
 
 from ogr.abstract import CommitStatus, GitProject
@@ -33,6 +33,17 @@ class BaseCommitStatus(Enum):
     error = "error"
 
 
+class DuplicateCheckMode(Enum):
+    """Enum of possible behaviour for handling duplicates when commenting."""
+
+    # Do not check for duplicates
+    do_not_check = auto()
+    # Check only last comment from us for duplicate
+    check_last_comment = auto()
+    # Check the whole comment list for duplicate
+    check_all_comments = auto()
+
+
 MAP_TO_COMMIT_STATUS: Dict[BaseCommitStatus, CommitStatus] = {
     BaseCommitStatus.pending: CommitStatus.pending,
     BaseCommitStatus.running: CommitStatus.running,
@@ -59,6 +70,7 @@ def __init__(
         self,
         project: GitProject,
         commit_sha: str,
+        packit_user: str,
         trigger_id: int = None,
         pr_id: Optional[int] = None,
     ):
@@ -67,6 +79,7 @@ def __init__(
         )
         self.project: GitProject = project
         self._project_with_commit: Optional[GitProject] = None
+        self._packit_user = packit_user
 
         self.commit_sha: str = commit_sha
         self.trigger_id: int = trigger_id
@@ -77,6 +90,7 @@ def get_instance(
         cls,
         project: GitProject,
         commit_sha: str,
+        packit_user,
         trigger_id: Optional[int] = None,
         pr_id: Optional[int] = None,
     ) -> "StatusReporter":
@@ -90,7 +104,7 @@ def get_instance(
             reporter = StatusReporterGitlab
         elif isinstance(project, PagureProject):
             reporter = StatusReporterPagure
-        return reporter(project, commit_sha, trigger_id, pr_id)
+        return reporter(project, commit_sha, packit_user, trigger_id, pr_id)
 
     @property
     def project_with_commit(self) -> GitProject:
@@ -186,7 +200,7 @@ def is_final_state(state: BaseCommitStatus) -> bool:
 
     def _commit_comment_already_added(self, comment: str) -> bool:
         commit_comments = self.project.get_commit_comments(self.commit_sha)
-        if exists := any(c.comment == comment for c in commit_comments):
+        if exists := any(c.body == comment for c in commit_comments):
             logger.debug(
                 "The following comment already exists for commit "
                 f"{self.commit_sha}\n{comment}"
@@ -240,7 +254,47 @@ def report_status_by_comment(
     def get_statuses(self):
         self.project_with_commit.get_commit_statuses(commit=self.commit_sha)
 
-    def comment(self, body: str):
+    def _has_identical_comment(self, body: str, mode: DuplicateCheckMode) -> bool:
+        """Checks if the body is the same as the last or any comment based on mode."""
+        comments = (
+            self.project.get_pr(pr_id=self.pr_id).get_comments(reverse=True)
+            if self.pr_id
+            else reversed(self.project.get_commit_comments(self.commit_sha))
+        )
+        for comment in comments:
+            if comment.author.startswith(self._packit_user):
+                if mode == DuplicateCheckMode.check_last_comment:
+                    return body == comment.body
+                elif (
+                    mode == DuplicateCheckMode.check_all_comments
+                    and body == comment.body
+                ):
+                    return True
+
+        return False
+
+    def comment(
+        self,
+        body: str,
+        duplicate_check: DuplicateCheckMode = DuplicateCheckMode.do_not_check,
+    ):
+        """Reports status by adding a comment.
+
+        If the instance is tied to a PR, the comment is added to the PR,
+        otherwise, it is added to the tied commit (if the forge supports
+        commit comments).
+
+        Args:
+            body: The comment text.
+            duplicate_check: Determines if the comment will be added if the same comment
+                is already present in the PR.
+        """
+        if (
+            duplicate_check != DuplicateCheckMode.do_not_check
+            and self._has_identical_comment(body, duplicate_check)
+        ):
+            return
+
         if self.pr_id:
             self.project.get_pr(pr_id=self.pr_id).comment(body=body)
         else:

tests/integration/test_listen_to_fedmsg.py

@@ -195,10 +195,11 @@ def copr_build_release():
 
 
 @pytest.mark.parametrize(
-    "pc_comment_pr_succ,pr_comment_called",
+    "pc_comment_pr_succ,pr_comment_called,pr_comment_exists",
     (
-        (True, True),
-        (False, False),
+        (True, True, True),
+        (True, True, False),
+        (False, False, False),
     ),
 )
 def test_copr_build_end(
@@ -207,8 +208,26 @@ def test_copr_build_end(
     copr_build_pr,
     pc_comment_pr_succ,
     pr_comment_called,
+    pr_comment_exists,
 ):
-    pr = flexmock(source_project=flexmock())
+    def get_comments(*args, **kwargs):
+        if pr_comment_exists:
+            return [
+                flexmock(
+                    author="packit-as-a-service[bot]",
+                    body="Congratulations! One of the builds has completed. :champagne:\n\n"
+                    "You can install the built RPMs by following these steps:\n\n* "
+                    "`sudo yum install -y dnf-plugins-core` on RHEL 8\n* "
+                    "`sudo dnf install -y dnf-plugins-core` on Fedora\n* "
+                    "`dnf copr enable packit/packit-service-hello-world-24`\n* "
+                    "And now you can install the packages.\n\n"
+                    "Please note that the RPMs should be used only in a testing environment.",
+                )
+            ]
+        else:
+            return []
+
+    pr = flexmock(source_project=flexmock(), get_comments=get_comments)
     flexmock(GithubProject).should_receive("is_private").and_return(False)
     flexmock(GithubProject).should_receive("get_pr").and_return(pr)
     pc_build_pr.jobs[0].notifications.pull_request.successful_build = pc_comment_pr_succ
@@ -218,10 +237,7 @@ def test_copr_build_end(
     flexmock(CoprHelper).should_receive("get_copr_client").and_return(
         Client(config={"username": "packit", "copr_url": "https://dummy.url"})
     )
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(False)
-    if pr_comment_called:
+    if pr_comment_called and not pr_comment_exists:
         pr.should_receive("comment")
     else:
         pr.should_receive("comment").never()
@@ -292,9 +308,6 @@ def test_copr_build_end_push(copr_build_end, pc_build_push, copr_build_branch_pu
     flexmock(CoprHelper).should_receive("get_copr_client").and_return(
         Client(config={"username": "packit", "copr_url": "https://dummy.url"})
     )
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(False)
 
     flexmock(CoprBuildTargetModel).should_receive("get_by_build_id").and_return(
         copr_build_branch_push
@@ -353,9 +366,6 @@ def test_copr_build_end_release(copr_build_end, pc_build_release, copr_build_rel
     flexmock(CoprHelper).should_receive("get_copr_client").and_return(
         Client(config={"username": "packit", "copr_url": "https://dummy.url"})
     )
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(False)
 
     flexmock(CoprBuildTargetModel).should_receive("get_by_build_id").and_return(
         copr_build_release
@@ -448,9 +458,6 @@ def test_copr_build_end_testing_farm(copr_build_end, copr_build_pr):
     flexmock(PackageConfigGetter).should_receive(
         "get_package_config_from_repo"
     ).and_return(config)
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(False)
 
     flexmock(LocalProject).should_receive("refresh_the_arguments").and_return(None)
 
@@ -657,9 +664,6 @@ def test_copr_build_end_failed_testing_farm(copr_build_end, copr_build_pr):
     flexmock(PackageConfigGetter).should_receive(
         "get_package_config_from_repo"
     ).and_return(config)
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(False)
 
     flexmock(LocalProject).should_receive("refresh_the_arguments").and_return(None)
 
@@ -798,9 +802,6 @@ def test_copr_build_end_failed_testing_farm_no_json(copr_build_end, copr_build_p
     flexmock(PackageConfigGetter).should_receive(
         "get_package_config_from_repo"
     ).and_return(config)
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(False)
 
     flexmock(LocalProject).should_receive("refresh_the_arguments").and_return(None)
 
@@ -1024,10 +1025,6 @@ def test_copr_build_not_comment_on_success(copr_build_end, pc_build_pr, copr_bui
         EXPECTED_BUILD_CHECK_NAME
     )
 
-    flexmock(CoprBuildEndHandler).should_receive(
-        "was_last_packit_comment_with_congratulation"
-    ).and_return(True)
-
     flexmock(CoprBuildTargetModel).should_receive("get_by_build_id").and_return(
         copr_build_pr
     )