feat(open-pr-comments): add a column to pr comment (#53900)

This column helps differentiate between open and merged PR comments.
Changes are also needed in the PR comment tasks as the unique constraint
is now on PR + comment type

migrations_lockfile.txt

@@ -7,5 +7,5 @@ will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
 replays: 0003_add_size_to_recording_segment
-sentry: 0525_add_next_checkin_latest
+sentry: 0526_pr_comment_type_column
 social_auth: 0002_default_auto_field

src/sentry/migrations/0526_pr_comment_type_column.py

@@ -0,0 +1,62 @@
+# Generated by Django 3.2.20 on 2023-07-31 23:14
+
+import django.db.models.deletion
+from django.db import migrations
+
+import sentry.db.models.fields.bounded
+import sentry.db.models.fields.foreignkey
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0525_add_next_checkin_latest"),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.RunSQL(
+                    """
+                    ALTER TABLE "sentry_pullrequest_comment" ADD COLUMN "comment_type" integer NOT NULL DEFAULT 0;
+                    """,
+                    reverse_sql="""
+                    ALTER TABLE "sentry_pullrequest_comment" DROP COLUMN "comment_type";
+                    """,
+                    hints={"tables": ["sentry_pullrequest_comment"]},
+                )
+            ],
+            state_operations=[
+                migrations.AddField(
+                    model_name="pullrequestcomment",
+                    name="comment_type",
+                    field=sentry.db.models.fields.bounded.BoundedPositiveIntegerField(
+                        default=0, null=False
+                    ),
+                ),
+                migrations.AlterField(
+                    model_name="pullrequestcomment",
+                    name="pull_request",
+                    field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE, to="sentry.pullrequest"
+                    ),
+                ),
+            ],
+        ),
+        migrations.AlterUniqueTogether(
+            name="pullrequestcomment",
+            unique_together={("pull_request", "comment_type")},
+        ),
+    ]

src/sentry/models/pullrequest.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from typing import Any, Mapping
+from typing import Any, Mapping, Sequence, Tuple
 
 from django.contrib.postgres.fields import ArrayField as DjangoArrayField
 from django.db import models
@@ -91,17 +91,30 @@ class PullRequestCommit(Model):
         unique_together = (("pull_request", "commit"),)
 
 
+class CommentType:
+    MERGED_PR = 0
+    OPEN_PR = 1
+
+    @classmethod
+    def as_choices(cls) -> Sequence[Tuple[int, str]]:
+        return ((cls.MERGED_PR, "merged_pr"), (cls.OPEN_PR, "open_pr"))
+
+
 @region_silo_only_model
 class PullRequestComment(Model):
     __include_in_export__ = False
 
     external_id = BoundedBigIntegerField()
-    pull_request = FlexibleForeignKey("sentry.PullRequest", unique=True)
+    pull_request = FlexibleForeignKey("sentry.PullRequest")
     created_at = models.DateTimeField()
     updated_at = models.DateTimeField()
     group_ids = DjangoArrayField(BoundedBigIntegerField())
     reactions = JSONField(null=True)
+    comment_type = BoundedPositiveIntegerField(
+        default=CommentType.MERGED_PR, choices=CommentType.as_choices(), null=False
+    )
 
     class Meta:
         app_label = "sentry"
         db_table = "sentry_pullrequest_comment"
+        unique_together = ("pull_request", "comment_type")

src/sentry/tasks/integrations/github/pr_comment.py

@@ -16,7 +16,7 @@ from sentry.integrations.github.client import GitHubAppsClient
 from sentry.models import Group, GroupOwnerType, Project
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.models.organization import Organization
-from sentry.models.pullrequest import PullRequestComment
+from sentry.models.pullrequest import CommentType, PullRequestComment
 from sentry.models.repository import Repository
 from sentry.services.hybrid_cloud.integration import integration_service
 from sentry.shared_integrations.exceptions.base import ApiError
@@ -138,6 +138,7 @@ def create_or_update_comment(
     comment_body: str,
     pullrequest_id: int,
     issue_list: List[int],
+    comment_type: CommentType = CommentType.MERGED_PR,
 ):
     # client will raise ApiError if the request is not successful
     if pr_comment is None:
@@ -150,6 +151,7 @@ def create_or_update_comment(
             created_at=current_time,
             updated_at=current_time,
             group_ids=issue_list,
+            comment_type=comment_type,
         )
     else:
         resp = client.update_comment(
@@ -197,7 +199,9 @@ def github_comment_workflow(pullrequest_id: int, project_id: int):
         return
 
     pr_comment = None
-    pr_comment_query = PullRequestComment.objects.filter(pull_request__id=pullrequest_id)
+    pr_comment_query = PullRequestComment.objects.filter(
+        pull_request__id=pullrequest_id, comment_type=CommentType.MERGED_PR
+    )
     if pr_comment_query.exists():
         pr_comment = pr_comment_query[0]
 

tests/sentry/tasks/integrations/github/test_pr_comment.py

@@ -10,7 +10,7 @@ from sentry.integrations.github.integration import GitHubIntegrationProvider
 from sentry.models import Commit, Group, GroupOwner, GroupOwnerType, PullRequest
 from sentry.models.options.organization_option import OrganizationOption
 from sentry.models.project import Project
-from sentry.models.pullrequest import PullRequestComment, PullRequestCommit
+from sentry.models.pullrequest import CommentType, PullRequestComment, PullRequestCommit
 from sentry.models.repository import Repository
 from sentry.shared_integrations.exceptions.base import ApiError
 from sentry.snuba.sessions_v2 import isoformat_z
@@ -364,6 +364,7 @@ class TestCommentWorkflow(GithubCommentTestCase):
         pull_request_comment_query = PullRequestComment.objects.all()
         assert len(pull_request_comment_query) == 1
         assert pull_request_comment_query[0].external_id == 1
+        assert pull_request_comment_query[0].comment_type == CommentType.MERGED_PR
         mock_metrics.incr.assert_called_with(
             "github_pr_comment.rate_limit_remaining", tags={"remaining": 59}
         )
@@ -385,6 +386,16 @@ class TestCommentWorkflow(GithubCommentTestCase):
             group_ids=[1, 2, 3, 4],
         )
 
+        # An Open PR comment should not affect the rest of the test as the filter should ignore it.
+        PullRequestComment.objects.create(
+            external_id=2,
+            pull_request_id=self.pr.id,
+            created_at=timezone.now() - timedelta(hours=1),
+            updated_at=timezone.now() - timedelta(hours=1),
+            group_ids=[],
+            comment_type=CommentType.OPEN_PR,
+        )
+
         responses.add(
             responses.POST,
             self.base_url + f"/app/installations/{self.installation_id}/access_tokens",