diff --git a/backend/code_review_backend/app/settings.py b/backend/code_review_backend/app/settings.py index 78e026789..7a25eb384 100644 --- a/backend/code_review_backend/app/settings.py +++ b/backend/code_review_backend/app/settings.py @@ -212,7 +212,7 @@ PHABRICATOR_HOST = "https://phabricator.services.mozilla.com" # Limit the automatic creation of reositories to allowed hosts -ALLOWED_REPOSITORY_HOSTS = ["hg.mozilla.org"] +ALLOWED_REPOSITORY_HOSTS = ["hg.mozilla.org", "github.com"] DYNO = env("DYNO") # Heroku settings override to run the web app through dyno diff --git a/backend/code_review_backend/issues/admin.py b/backend/code_review_backend/issues/admin.py index a29efb634..775098467 100644 --- a/backend/code_review_backend/issues/admin.py +++ b/backend/code_review_backend/issues/admin.py @@ -14,19 +14,30 @@ class RepositoryAdmin(admin.ModelAdmin): class DiffInline(admin.TabularInline): # Read only inline model = Diff - readonly_fields = ("id", "repository", "mercurial_hash", "phid", "review_task_id") + readonly_fields = ( + "id", + "repository", + "mercurial_hash", + "provider_id", + "review_task_id", + ) class RevisionAdmin(admin.ModelAdmin): list_display = ( "id", - "phabricator_id", + "provider", + "provider_id", "title", "bugzilla_id", "base_repository", "head_repository", ) - list_filter = ("base_repository", "head_repository") + list_filter = ( + "base_repository", + "head_repository", + "provider", + ) inlines = (DiffInline,) diff --git a/backend/code_review_backend/issues/api.py b/backend/code_review_backend/issues/api.py index e9fc5966f..671f61e36 100644 --- a/backend/code_review_backend/issues/api.py +++ b/backend/code_review_backend/issues/api.py @@ -6,6 +6,7 @@ from datetime import date, datetime, timedelta from django.conf import settings +from django.core.exceptions import BadRequest from django.db.models import BooleanField, Count, ExpressionWrapper, Prefetch, Q from django.db.models.functions import TruncDate from django.shortcuts import get_object_or_404 @@ -77,10 +78,15 @@ def create(self, request, *args, **kwargs): # When a revision already exists with that phabricator ID we return its data without creating a new one # This value is used by the bot to identify a revision and publish new Phabricator diffs. # The phabricator ID can be null (on mozilla-central) so we must always try to create a revision for that case - phabricator_id = request.data["phabricator_id"] - if phabricator_id is not None: + try: + provider = request.data["provider"] + provider_id = request.data["provider_id"] + except KeyError: + raise BadRequest("Invalid provider identification") + if provider_id is not None: if revision := Revision.objects.filter( - phabricator_id=phabricator_id + provider=provider, + provider_id=provider_id, ).first(): serializer = RevisionSerializer( instance=revision, context={"request": request} @@ -166,7 +172,7 @@ def get_queryset(self): if query is not None: search_query = ( Q(id__icontains=query) - | Q(revision__phabricator_id__icontains=query) + | Q(revision__provider_id__icontains=query) | Q(revision__bugzilla_id__icontains=query) | Q(revision__title__icontains=query) ) @@ -200,14 +206,14 @@ class IssueViewSet( def get_queryset(self): # Required to generate the OpenAPI documentation - if not self.kwargs.get("diff_id"): + if not self.kwargs.get("diff_provider_id"): return Issue.objects.none() - diff = get_object_or_404(Diff, id=self.kwargs["diff_id"]) + diff = get_object_or_404(Diff, provider_id=self.kwargs["diff_provider_id"]) return ( Issue.objects.filter(issue_links__diff=diff) .annotate(publishable=Q(issue_links__in_patch=True) & Q(level=LEVEL_ERROR)) .values( - "id", + "provider_id", "hash", "analyzer", "analyzer_check", @@ -497,7 +503,9 @@ def get_queryset(self): basename="revision-diffs", ) router.register(r"diff", DiffViewSet, basename="diffs") -router.register(r"diff/(?P\d+)/issues", IssueViewSet, basename="issues") +router.register( + r"diff/(?P[0-9a-zA-Z-]+)/issues", IssueViewSet, basename="issues" +) urls = router.urls + [ path( "revision//issues/", diff --git a/backend/code_review_backend/issues/compare.py b/backend/code_review_backend/issues/compare.py index ed753f703..eb0b56207 100644 --- a/backend/code_review_backend/issues/compare.py +++ b/backend/code_review_backend/issues/compare.py @@ -12,7 +12,8 @@ def detect_new_for_revision(diff: Diff, path: str, hash: str) -> bool: assert diff is not None, "Missing diff" return not IssueLink.objects.filter( revision_id=diff.revision_id, - diff_id__lt=diff.id, + # Ideally we would rely on the diff integer ID, but that information was lost adding the Github support + diff__created__lt=diff.created, issue__path=path, issue__hash=hash, ).exists() diff --git a/backend/code_review_backend/issues/management/commands/load_in_patch.py b/backend/code_review_backend/issues/management/commands/load_in_patch.py index dc9d571d0..5fc38307f 100644 --- a/backend/code_review_backend/issues/management/commands/load_in_patch.py +++ b/backend/code_review_backend/issues/management/commands/load_in_patch.py @@ -79,11 +79,11 @@ def process_diff(diff: Diff): detect_in_patch(issue_link, lines) for issue_link in diff.issue_links.all() ] logging.info( - f"Found {len([i for i in issue_links if i.in_patch])} issue link in patch for {diff.id}" + f"Found {len([i for i in issue_links if i.in_patch])} issue link in patch for {diff.provider_id}" ) IssueLink.objects.bulk_update(issue_links, ["in_patch"]) except Exception as e: - logging.info(f"Failure on diff {diff.id}: {e}") + logging.info(f"Failure on diff {diff.provider_id}: {e}") class Command(BaseCommand): diff --git a/backend/code_review_backend/issues/management/commands/load_issues.py b/backend/code_review_backend/issues/management/commands/load_issues.py index 76e2f7eaa..a7dd5b202 100644 --- a/backend/code_review_backend/issues/management/commands/load_issues.py +++ b/backend/code_review_backend/issues/management/commands/load_issues.py @@ -213,9 +213,9 @@ def build_revision_and_diff(self, data, task_id): return None, None revision, _ = head_repository.head_revisions.get_or_create( - phabricator_id=data["id"], + provider="phabricator", + provider_id=data["id"], defaults={ - "phabricator_phid": data["phid"], "title": data["title"], "bugzilla_id": int(data["bugzilla_id"]) if data["bugzilla_id"] @@ -224,10 +224,9 @@ def build_revision_and_diff(self, data, task_id): }, ) diff, _ = revision.diffs.get_or_create( - id=data["diff_id"], + provider_id=data["diff_phid"], defaults={ "repository": head_repository, - "phid": data["diff_phid"], "review_task_id": task_id, "mercurial_hash": data["mercurial_revision"], }, diff --git a/backend/code_review_backend/issues/migrations/0016_revision_diff_generic_provider.py b/backend/code_review_backend/issues/migrations/0016_revision_diff_generic_provider.py new file mode 100644 index 000000000..d29df6802 --- /dev/null +++ b/backend/code_review_backend/issues/migrations/0016_revision_diff_generic_provider.py @@ -0,0 +1,55 @@ +# Generated by Django 5.1.6 on 2026-01-21 16:01 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("issues", "0015_remove_repository_phid_alter_repository_id"), + ] + + operations = [ + migrations.AlterModelOptions( + name="revision", + options={"ordering": ("provider", "provider_id", "id")}, + ), + migrations.RemoveConstraint( + model_name="revision", + name="revision_unique_phab_id", + ), + migrations.RenameField( + model_name="revision", + old_name="phabricator_id", + new_name="provider_id", + ), + migrations.AddField( + model_name="revision", + name="provider", + field=models.CharField( + choices=[("phabricator", "Phabricator"), ("github", "Github")], + default="phabricator", + max_length=20, + ), + ), + migrations.AddConstraint( + model_name="revision", + constraint=models.UniqueConstraint( + condition=models.Q(("provider_id__isnull", False)), + fields=("provider_id",), + name="revision_unique_phab_id", + ), + ), + migrations.RemoveConstraint( + model_name="revision", + name="revision_unique_phab_phabid", + ), + migrations.RemoveField( + model_name="revision", + name="phabricator_phid", + ), + migrations.RenameField( + model_name="diff", + old_name="phid", + new_name="provider_id", + ), + ] diff --git a/backend/code_review_backend/issues/migrations/0017_diff_auto_pk.py b/backend/code_review_backend/issues/migrations/0017_diff_auto_pk.py new file mode 100644 index 000000000..4521580d0 --- /dev/null +++ b/backend/code_review_backend/issues/migrations/0017_diff_auto_pk.py @@ -0,0 +1,67 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +from django.db import migrations, models +from django.db.models import F + + +def _update_provider_id(apps, schema_editor): + """ + Update all diffs, setting the current ID as the provider ID. + This step is required no to loose the Phabricator integer ID, + which is the reference used by most developers. + The previous value (PHID-DIFF-xxxxxxxxxxxxxxxxxxxx) is dropped, + then the PK is replaced by a random UUID, allowing the support + of other providers without a known unique integer identifier. + """ + Diff = apps.get_model("issues", "Diff") + Diff.objects.update(provider_id=F("id")) + + +def _reset_postgres_sequense(apps, schema_editor): + """ + PostgreSQL needs to update the auto increment sequence value, otherwise + conflicts would happen when we reach the initial Diff ID. + + This command was initially generated with ./manage.py sqlsequencereset issues + """ + if schema_editor.connection.vendor != "postgresql": + # The SQLite backend automatically handles the auto increment update + return + schema_editor.execute( + """SELECT setval( + pg_get_serial_sequence('"issues_diff"','id'), + coalesce(max("id"), 1), + max("id") IS NOT null + ) FROM "issues_diff"; + """, + ) + + +class Migration(migrations.Migration): + dependencies = [ + ("issues", "0016_revision_diff_generic_provider"), + ] + + operations = [ + # Save the Phabricator integer ID in Diff.provider_id + migrations.RunPython( + _update_provider_id, + reverse_code=migrations.RunPython.noop, + ), + migrations.AlterField( + model_name="diff", + name="id", + field=models.AutoField(primary_key=True, serialize=False), + ), + migrations.AlterModelOptions( + name="diff", + options={"ordering": ("created",)}, + ), + # Restore the sequence with PostgreSQL backend + migrations.RunPython( + _reset_postgres_sequense, + reverse_code=migrations.RunPython.noop, + ), + ] diff --git a/backend/code_review_backend/issues/models.py b/backend/code_review_backend/issues/models.py index da457656b..098f431ef 100644 --- a/backend/code_review_backend/issues/models.py +++ b/backend/code_review_backend/issues/models.py @@ -13,6 +13,13 @@ LEVEL_ERROR = "error" ISSUE_LEVELS = ((LEVEL_WARNING, "Warning"), (LEVEL_ERROR, "Error")) +PROVIDER_PHABRICATOR = "phabricator" +PROVIDER_GITHUB = "github" +PROVIDERS = ( + (PROVIDER_PHABRICATOR, "Phabricator"), + (PROVIDER_GITHUB, "Github"), +) + class Repository(models.Model): id = models.AutoField(primary_key=True) @@ -33,11 +40,12 @@ def __str__(self): class Revision(models.Model): id = models.BigAutoField(primary_key=True) + # Phabricator references will be left empty when ingesting a decision task (e.g. from MC or autoland) - phabricator_id = models.PositiveIntegerField(unique=True, null=True, blank=True) - phabricator_phid = models.CharField( - max_length=40, unique=True, null=True, blank=True + provider = models.CharField( + max_length=20, choices=PROVIDERS, default=PROVIDER_PHABRICATOR ) + provider_id = models.PositiveIntegerField(unique=True, null=True, blank=True) created = models.DateTimeField(auto_now_add=True) updated = models.DateTimeField(auto_now=True) @@ -72,43 +80,45 @@ class Revision(models.Model): bugzilla_id = models.PositiveIntegerField(null=True) class Meta: - ordering = ("phabricator_id", "id") + ordering = ("provider", "provider_id", "id") indexes = (models.Index(fields=["head_repository", "head_changeset"]),) constraints = [ models.UniqueConstraint( - fields=["phabricator_id"], + fields=["provider_id"], name="revision_unique_phab_id", - condition=Q(phabricator_id__isnull=False), - ), - models.UniqueConstraint( - fields=["phabricator_phid"], - name="revision_unique_phab_phabid", - condition=Q(phabricator_phid__isnull=False), + condition=Q(provider_id__isnull=False), ), ] def __str__(self): - if self.phabricator_id is not None: - return f"D{self.phabricator_id} - {self.title}" + if self.provider == PROVIDER_PHABRICATOR and self.provider_id is not None: + return f"Phabricator D{self.provider_id} - {self.title}" return f"#{self.id} - {self.title}" @property - def phabricator_url(self): - if self.phabricator_id is None: + def url(self): + if self.provider_id is None: return - parser = urllib.parse.urlparse(settings.PHABRICATOR_HOST) - return f"{parser.scheme}://{parser.netloc}/D{self.phabricator_id}" + + if self.provider == PROVIDER_PHABRICATOR: + parser = urllib.parse.urlparse(settings.PHABRICATOR_HOST) + return f"{parser.scheme}://{parser.netloc}/D{self.provider_id}" + elif self.provider == PROVIDER_GITHUB: + return f"{self.base_repository.url}/issues/{self.provider_id}" + else: + raise NotImplementedError class Diff(models.Model): - """Reference of a specific code patch (diff) in Phabricator. + """Reference of a specific code patch (diff) in Phabricator or Github. A revision can be linked to multiple successive diffs, or none in case of a repository push. """ - # Phabricator's attributes - id = models.PositiveIntegerField(primary_key=True) - phid = models.CharField(max_length=40, unique=True) + id = models.AutoField(primary_key=True) + + # Provider ID is unique among all providers (integer for Phabricator, hash for github) + provider_id = models.CharField(max_length=40, unique=True) created = models.DateTimeField(auto_now_add=True) updated = models.DateTimeField(auto_now=True) @@ -126,10 +136,10 @@ class Diff(models.Model): ) def __str__(self): - return f"Diff {self.id}" + return f"Diff {self.provider_id}" class Meta: - ordering = ("id",) + ordering = ("created",) class IssueLink(models.Model): diff --git a/backend/code_review_backend/issues/serializers.py b/backend/code_review_backend/issues/serializers.py index 888fd52ff..7bb0c2324 100644 --- a/backend/code_review_backend/issues/serializers.py +++ b/backend/code_review_backend/issues/serializers.py @@ -35,7 +35,7 @@ class RepositoryGetOrCreateField(serializers.SlugRelatedField): ) default_error_messages = { **serializers.SlugRelatedField.default_error_messages, - "invalid_url": "Repository URL must match hg.mozilla.org.", + "invalid_url": "Repository base URL is not allowed.", } queryset = Repository.objects.all() @@ -52,9 +52,12 @@ def to_internal_value(self, url): except (TypeError, ValueError): self.fail("invalid") return + # Github repositories uses a path like /, so we need to replace the + # inner slash to have a slug usable within URLs (e.g. to list issues on a repo). + slug = parsed.path.lstrip("/").replace("/", "-") try: repo, _ = self.get_queryset().get_or_create( - url=url, defaults={"slug": parsed.path.lstrip("/")} + url=url, defaults={"slug": slug} ) return repo except (TypeError, ValueError): @@ -74,8 +77,8 @@ class RevisionSerializer(serializers.ModelSerializer): issues_bulk_url = serializers.HyperlinkedIdentityField( view_name="revision-issues-bulk", lookup_url_kwarg="revision_id" ) - phabricator_url = serializers.URLField(read_only=True) - phabricator_id = serializers.IntegerField( + url = serializers.URLField(read_only=True) + provider_id = serializers.IntegerField( required=False, allow_null=True, min_value=1, @@ -90,13 +93,13 @@ class Meta: "head_repository", "base_changeset", "head_changeset", - "phabricator_id", - "phabricator_phid", + "provider", + "provider_id", "title", "bugzilla_id", "diffs_url", "issues_bulk_url", - "phabricator_url", + "url", ) @@ -107,21 +110,21 @@ class RevisionLightSerializer(serializers.ModelSerializer): base_repository = RepositoryGetOrCreateField() head_repository = RepositoryGetOrCreateField() - phabricator_url = serializers.URLField(read_only=True) + url = serializers.URLField(read_only=True) class Meta: model = Revision fields = ( "id", - "phabricator_id", + "provider", + "provider_id", "base_repository", "head_repository", "base_changeset", "head_changeset", - "phabricator_id", "title", "bugzilla_id", - "phabricator_url", + "url", ) @@ -135,14 +138,15 @@ class DiffSerializer(serializers.ModelSerializer): queryset=Repository.objects.all(), slug_field="url" ) issues_url = serializers.HyperlinkedIdentityField( - view_name="issues-list", lookup_url_kwarg="diff_id" + view_name="issues-list", + lookup_url_kwarg="diff_provider_id", + lookup_field="provider_id", ) class Meta: model = Diff fields = ( - "id", - "phid", + "provider_id", "review_task_id", "repository", "mercurial_hash", @@ -163,7 +167,7 @@ class DiffLightSerializer(serializers.ModelSerializer): class Meta: model = Diff - fields = ("id", "repository", "revision") + fields = ("provider_id", "repository", "revision") class DiffFullSerializer(serializers.ModelSerializer): @@ -175,7 +179,9 @@ class DiffFullSerializer(serializers.ModelSerializer): revision = RevisionSerializer(read_only=True) repository = RepositorySerializer(read_only=True) issues_url = serializers.HyperlinkedIdentityField( - view_name="issues-list", lookup_url_kwarg="diff_id" + view_name="issues-list", + lookup_url_kwarg="diff_provider_id", + lookup_field="provider_id", ) nb_issues = serializers.IntegerField(read_only=True) nb_issues_publishable = serializers.IntegerField(read_only=True) @@ -185,9 +191,8 @@ class DiffFullSerializer(serializers.ModelSerializer): class Meta: model = Diff fields = ( - "id", + "provider_id", "revision", - "phid", "review_task_id", "repository", "mercurial_hash", @@ -264,7 +269,8 @@ class SingleIssueBulkSerializer(IssueSerializer): class IssueBulkSerializer(serializers.Serializer): - diff_id = serializers.PrimaryKeyRelatedField( + diff_provider_id = serializers.SlugRelatedField( + slug_field="provider_id", # Initialized depending on the revision used for the creation queryset=Diff.objects.none(), style={"base_template": "input.html"}, @@ -277,11 +283,11 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if not self.context.get("revision"): return - self.fields["diff_id"].queryset = self.context["revision"].diffs.all() + self.fields["diff_provider_id"].queryset = self.context["revision"].diffs.all() @transaction.atomic def create(self, validated_data): - diff = validated_data.get("diff_id", None) + diff = validated_data.get("diff_provider_id", None) link_attrs = defaultdict(list) # Separate attributes that are specific to the IssueLink M2M for issue in validated_data["issues"]: @@ -340,7 +346,7 @@ def create(self, validated_data): output.append(output_link) return { - "diff_id": diff, + "diff_provider_id": diff, "issues": output, } diff --git a/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py b/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py index b6ba5fe40..135d92362 100644 --- a/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py +++ b/backend/code_review_backend/issues/tests/commands/test_cleanup_issues.py @@ -56,8 +56,8 @@ def setUp(self): [ Revision( id=i, - phabricator_id=i, - phabricator_phid=i, + provider="phabricator", + provider_id=i, title=f"Revision {i}", base_repository=repo, head_repository=repo, diff --git a/backend/code_review_backend/issues/tests/test_api.py b/backend/code_review_backend/issues/tests/test_api.py index 09d3e8ff4..0293d4a52 100644 --- a/backend/code_review_backend/issues/tests/test_api.py +++ b/backend/code_review_backend/issues/tests/test_api.py @@ -31,15 +31,14 @@ def setUp(self): ) # Create revision and diff self.revision = self.repo_try.head_revisions.create( - phabricator_id=456, - phabricator_phid="PHID-REV-XXX", + provider="phabricator", + provider_id=456, title="Bug XXX - Yet Another bug", bugzilla_id=78901, base_repository=self.repo, ) self.diff = self.revision.diffs.create( - id=1234, - phid="PHID-DIFF-xxx", + provider_id="PHID-DIFF-1234", review_task_id="deadbeef123", mercurial_hash="coffee12345", repository=self.repo_try, @@ -51,8 +50,8 @@ def test_create_revision(self): """ self.revision.delete() data = { - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "provider": "phabricator", + "provider_id": 123, "title": "Bug XXX - Some bug", "bugzilla_id": 123456, "base_repository": "http://repo.test/myrepo", @@ -78,9 +77,9 @@ def test_create_revision(self): "bugzilla_id": 123456, "diffs_url": f"http://testserver/v1/revision/{revision_id}/diffs/", "issues_bulk_url": f"http://testserver/v1/revision/{revision_id}/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D123", - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "url": "https://phabricator.services.mozilla.com/D123", + "provider": "phabricator", + "provider_id": 123, "base_repository": "http://repo.test/myrepo", "head_repository": "http://repo.test/myrepo", "base_changeset": "123456789ABCDEF", @@ -88,7 +87,7 @@ def test_create_revision(self): "title": "Bug XXX - Some bug", } self.assertEqual(Revision.objects.count(), 1) - revision = Revision.objects.get(phabricator_id=123) + revision = Revision.objects.get(provider_id=123) self.assertEqual(revision.title, "Bug XXX - Some bug") self.assertEqual(revision.bugzilla_id, 123456) self.assertDictEqual(response.json(), expected_response) @@ -103,8 +102,8 @@ def test_create_revision(self): def test_create_revision_wrong_new_repo(self): self.revision.delete() data = { - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "provider": "phabricator", + "provider_id": 123, "title": "Bug XXX - Some bug", "bugzilla_id": 123456, "base_repository": "http://notamozrepo.test/myrepo", @@ -120,16 +119,16 @@ def test_create_revision_wrong_new_repo(self): self.assertDictEqual( response.json(), { - "base_repository": ["Repository URL must match hg.mozilla.org."], - "head_repository": ["Repository URL must match hg.mozilla.org."], + "base_repository": ["Repository base URL is not allowed."], + "head_repository": ["Repository base URL is not allowed."], }, ) def test_create_revision_creates_new_repo(self): self.revision.delete() data = { - "phabricator_id": 123, - "phabricator_phid": "PHID-REV-xxx", + "provider": "phabricator", + "provider_id": 123, "title": "Bug XXX - Some bug", "bugzilla_id": 123456, "base_repository": "http://repo.test/myrepo", @@ -144,7 +143,7 @@ def test_create_revision_creates_new_repo(self): self.assertEqual(response.status_code, status.HTTP_201_CREATED) new_repo = Repository.objects.get(url="http://hg.mozilla.org/a/new/repo") self.assertIsNotNone(new_repo) - self.assertEqual(new_repo.slug, "a/new/repo") + self.assertEqual(new_repo.slug, "a-new-repo") def test_create_diff(self): """ @@ -153,7 +152,7 @@ def test_create_diff(self): self.diff.delete() data = { "id": 1234, - "phid": "PHID-DIFF-xxx", + "provider_id": "PHID-DIFF-1234", "review_task_id": "deadbeef123", "mercurial_hash": "coffee12345", "repository": "http://repo.test/try", @@ -181,12 +180,13 @@ def test_create_diff(self): # Response should have url to create issues self.assertEqual( - response.json()["issues_url"], "http://testserver/v1/diff/1234/issues/" + response.json()["issues_url"], + "http://testserver/v1/diff/PHID-DIFF-1234/issues/", ) # Check a diff has been created self.assertEqual(Diff.objects.count(), 1) - diff = Diff.objects.get(pk=1234) + diff = Diff.objects.get(provider_id="PHID-DIFF-1234") self.assertEqual(diff.mercurial_hash, "coffee12345") self.assertEqual(diff.revision, self.revision) @@ -204,13 +204,17 @@ def test_create_issue_disabled(self): } # No auth will give a permission denied - response = self.client.post("/v1/diff/1234/issues/", data, format="json") + response = self.client.post( + "/v1/diff/PHID-DIFF-1234/issues/", data, format="json" + ) self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) # Once authenticated, creation will work self.assertEqual(Issue.objects.count(), 0) self.client.force_authenticate(user=self.user) - response = self.client.post("/v1/diff/1234/issues/", data, format="json") + response = self.client.post( + "/v1/diff/PHID-DIFF-1234/issues/", data, format="json" + ) self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED) # Do not check the content of issue created as it's a random UUID @@ -281,7 +285,7 @@ def test_create_issue_bulk(self): self.assertDictEqual( issue_data, { - "diff_id": None, + "diff_provider_id": None, "issues": [ { "analyzer": "remote-flake8", @@ -334,7 +338,7 @@ def test_create_issue_bulk_with_diff(self): Check we can create issues on a revision with a reference to a diff """ data = { - "diff_id": 1234, + "diff_provider_id": "PHID-DIFF-1234", "issues": [ { "hash": "somemd5hash", @@ -354,13 +358,13 @@ def test_create_issue_bulk_with_diff(self): ) self.assertEqual(response.status_code, status.HTTP_201_CREATED) - self.assertEqual(Issue.objects.filter(revisions__phabricator_id=456).count(), 1) + self.assertEqual(Issue.objects.filter(revisions__provider_id=456).count(), 1) - issue = Issue.objects.filter(revisions__phabricator_id=456).get() + issue = Issue.objects.filter(revisions__provider_id=456).get() self.assertDictEqual( response.json(), { - "diff_id": 1234, + "diff_provider_id": "PHID-DIFF-1234", "issues": [ { "analyzer": "remote-flake8", @@ -551,7 +555,7 @@ def test_create_issue_bulk_duplicate(self): self.assertDictEqual( issue_data, { - "diff_id": None, + "diff_provider_id": None, # The same issue link is returned twice in the resulting payload "issues": 2 * [ @@ -665,7 +669,7 @@ def test_create_issue_already_referenced(self): self.client.force_authenticate(user=self.user) another_diff = self.revision.diffs.create( id=56748, - phid="PHID-DIFF-yyy", + provider_id="PHID-DIFF-yyy", review_task_id="deadbeef456", mercurial_hash="coffee67890", repository=self.repo_try, diff --git a/backend/code_review_backend/issues/tests/test_compare.py b/backend/code_review_backend/issues/tests/test_compare.py index 78c05ef0b..6c163633c 100644 --- a/backend/code_review_backend/issues/tests/test_compare.py +++ b/backend/code_review_backend/issues/tests/test_compare.py @@ -27,8 +27,8 @@ def setUp(self): # Create a simple stack with 2 diffs self.revision = self.repo_try.head_revisions.create( - phabricator_id=1, - phabricator_phid="PHID-DREV-1", + provider="phabricator", + provider_id=1, title="Revision XYZ", bugzilla_id=1234567, base_repository=self.repo, @@ -36,7 +36,7 @@ def setUp(self): for i in range(2): self.revision.diffs.create( id=i + 1, - phid=f"PHID-DIFF-{i+1}", + provider_id=f"PHID-DIFF-{i+1}", review_task_id=f"task-{i}", mercurial_hash=hashlib.sha1(f"hg {i}".encode()).hexdigest(), repository=self.repo_try, diff --git a/backend/code_review_backend/issues/tests/test_diff.py b/backend/code_review_backend/issues/tests/test_diff.py index 6359ec73b..0779be28f 100644 --- a/backend/code_review_backend/issues/tests/test_diff.py +++ b/backend/code_review_backend/issues/tests/test_diff.py @@ -30,8 +30,8 @@ def setUpTestData(cls): for i in range(2): cls.repo_try.head_revisions.create( id=i + 1, - phabricator_id=i + 1, - phabricator_phid=f"PHID-DREV-{i+1}", + provider="phabricator", + provider_id=i + 1, title=f"Revision {i+1}", bugzilla_id=10000 + i, base_repository=cls.repo, @@ -39,7 +39,7 @@ def setUpTestData(cls): for i in range(3): Diff.objects.create( id=i + 1, - phid=f"PHID-DIFF-{i+1}", + provider_id=f"PHID-DIFF-{i+1}", revision_id=(i % 2) + 1, review_task_id=f"task-{i}", mercurial_hash=hashlib.sha1(f"hg {i}".encode()).hexdigest(), @@ -66,30 +66,29 @@ def test_list_diffs(self): "previous": None, "results": [ { - "id": 3, + "provider_id": "PHID-DIFF-3", "revision": { "id": 1, "base_repository": "http://repo.test/myrepo", "head_repository": "http://repo.test/try", "base_changeset": None, "head_changeset": None, - "phabricator_id": 1, - "phabricator_phid": "PHID-DREV-1", + "provider": "phabricator", + "provider_id": 1, "title": "Revision 1", "bugzilla_id": 10000, "diffs_url": "http://testserver/v1/revision/1/diffs/", "issues_bulk_url": "http://testserver/v1/revision/1/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D1", + "url": "https://phabricator.services.mozilla.com/D1", }, "repository": { "id": 2, "slug": "myrepo-try", "url": "http://repo.test/try", }, - "phid": "PHID-DIFF-3", "review_task_id": "task-2", "mercurial_hash": "30b501affc4d3b9c670fc297ab903b406afd5f04", - "issues_url": "http://testserver/v1/diff/3/issues/", + "issues_url": "http://testserver/v1/diff/PHID-DIFF-3/issues/", "nb_issues": 0, "nb_issues_publishable": 0, "nb_warnings": 0, @@ -97,30 +96,29 @@ def test_list_diffs(self): "created": self.now, }, { - "id": 2, + "provider_id": "PHID-DIFF-2", "revision": { "id": 2, "base_repository": "http://repo.test/myrepo", "head_repository": "http://repo.test/try", "base_changeset": None, "head_changeset": None, - "phabricator_id": 2, - "phabricator_phid": "PHID-DREV-2", + "provider": "phabricator", + "provider_id": 2, "title": "Revision 2", "bugzilla_id": 10001, "diffs_url": "http://testserver/v1/revision/2/diffs/", "issues_bulk_url": "http://testserver/v1/revision/2/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D2", + "url": "https://phabricator.services.mozilla.com/D2", }, "repository": { "id": 2, "slug": "myrepo-try", "url": "http://repo.test/try", }, - "phid": "PHID-DIFF-2", "review_task_id": "task-1", "mercurial_hash": "32d2a594cfef74fcb524028d1521d0d4bd98bd35", - "issues_url": "http://testserver/v1/diff/2/issues/", + "issues_url": "http://testserver/v1/diff/PHID-DIFF-2/issues/", "nb_issues": 0, "nb_issues_publishable": 0, "nb_warnings": 0, @@ -128,30 +126,29 @@ def test_list_diffs(self): "created": self.now, }, { - "id": 1, + "provider_id": "PHID-DIFF-1", "revision": { "id": 1, "base_repository": "http://repo.test/myrepo", "head_repository": "http://repo.test/try", "base_changeset": None, "head_changeset": None, - "phabricator_id": 1, - "phabricator_phid": "PHID-DREV-1", + "provider": "phabricator", + "provider_id": 1, "title": "Revision 1", "bugzilla_id": 10000, "diffs_url": "http://testserver/v1/revision/1/diffs/", "issues_bulk_url": "http://testserver/v1/revision/1/issues/", - "phabricator_url": "https://phabricator.services.mozilla.com/D1", + "url": "https://phabricator.services.mozilla.com/D1", }, "repository": { "id": 2, "slug": "myrepo-try", "url": "http://repo.test/try", }, - "phid": "PHID-DIFF-1", "review_task_id": "task-0", "mercurial_hash": "a2ac78b7d12d6e55b9b15c1c2048a16c58c6c803", - "issues_url": "http://testserver/v1/diff/1/issues/", + "issues_url": "http://testserver/v1/diff/PHID-DIFF-1/issues/", "nb_issues": 0, "nb_issues_publishable": 0, "nb_warnings": 0, @@ -171,7 +168,10 @@ def test_filter_repo(self): response = self.client.get("/v1/diff/?repository=myrepo") self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["count"], 3) - self.assertEqual([d["id"] for d in response.json()["results"]], [3, 2, 1]) + self.assertEqual( + [d["provider_id"] for d in response.json()["results"]], + ["PHID-DIFF-3", "PHID-DIFF-2", "PHID-DIFF-1"], + ) # Missing repo response = self.client.get("/v1/diff/?repository=missing") @@ -187,13 +187,18 @@ def test_search(self): response = self.client.get("/v1/diff/?search=10001") self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["count"], 1) - self.assertEqual([d["id"] for d in response.json()["results"]], [2]) + self.assertEqual( + [d["provider_id"] for d in response.json()["results"]], ["PHID-DIFF-2"] + ) # In title response = self.client.get("/v1/diff/?search=revision 1") self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["count"], 2) - self.assertEqual([d["id"] for d in response.json()["results"]], [3, 1]) + self.assertEqual( + [d["provider_id"] for d in response.json()["results"]], + ["PHID-DIFF-3", "PHID-DIFF-1"], + ) def test_filter_issues(self): """ @@ -204,7 +209,10 @@ def test_filter_issues(self): response = self.client.get("/v1/diff/?issues=no") self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json()["count"], 3) - self.assertEqual([d["id"] for d in response.json()["results"]], [3, 2, 1]) + self.assertEqual( + [d["provider_id"] for d in response.json()["results"]], + ["PHID-DIFF-3", "PHID-DIFF-2", "PHID-DIFF-1"], + ) # Any issues response = self.client.get("/v1/diff/?issues=any") diff --git a/backend/code_review_backend/issues/tests/test_issue.py b/backend/code_review_backend/issues/tests/test_issue.py index 070ca4679..a4c5c2311 100644 --- a/backend/code_review_backend/issues/tests/test_issue.py +++ b/backend/code_review_backend/issues/tests/test_issue.py @@ -24,16 +24,16 @@ def setUp(self): mock_now.return_value = datetime.fromisoformat("2010-01-01:10") self.revision = self.repo.base_revisions.create( id=4000, - phabricator_id=1111, - phabricator_phid="PH-1111", + provider="phabricator", + provider_id=1111, head_changeset="1" * 40, head_repository=self.repo, ) mock_now.return_value = datetime.fromisoformat("2000-01-01:10") self.old_revision = self.repo.base_revisions.create( id=4001, - phabricator_id=2222, - phabricator_phid="PH-2222", + provider="phabricator", + provider_id=2222, head_changeset="2" * 40, head_repository=self.repo, ) diff --git a/backend/code_review_backend/issues/tests/test_revision.py b/backend/code_review_backend/issues/tests/test_revision.py index fb1f93668..1fb6f4265 100644 --- a/backend/code_review_backend/issues/tests/test_revision.py +++ b/backend/code_review_backend/issues/tests/test_revision.py @@ -18,21 +18,19 @@ def setUp(self): def test_phabricator_url(self): rev = Revision.objects.create( - phabricator_id=12, - phabricator_phid="PHID-REV-12345", + provider="phabricator", + provider_id=12, base_repository=self.repo, head_repository=self.repo, ) # Default settings - self.assertEqual( - rev.phabricator_url, "https://phabricator.services.mozilla.com/D12" - ) + self.assertEqual(rev.url, "https://phabricator.services.mozilla.com/D12") # Override host with /api settings.PHABRICATOR_HOST = "http://phab.test/api" - self.assertEqual(rev.phabricator_url, "http://phab.test/D12") + self.assertEqual(rev.url, "http://phab.test/D12") # Override host with complex url settings.PHABRICATOR_HOST = "http://anotherphab.test/api123/?custom" - self.assertEqual(rev.phabricator_url, "http://anotherphab.test/D12") + self.assertEqual(rev.url, "http://anotherphab.test/D12") diff --git a/backend/code_review_backend/issues/tests/test_stats.py b/backend/code_review_backend/issues/tests/test_stats.py index 1746b64ca..191ae93fe 100644 --- a/backend/code_review_backend/issues/tests/test_stats.py +++ b/backend/code_review_backend/issues/tests/test_stats.py @@ -29,8 +29,8 @@ def setUp(self): # Create a revision revision = self.repo_try.head_revisions.create( - phabricator_id=10, - phabricator_phid="PHID-DREV-arev", + provider="phabricator", + provider_id=10, title="Revision A", bugzilla_id=None, base_repository=self.repo, @@ -40,7 +40,7 @@ def setUp(self): for i in range(10): revision.diffs.create( id=i + 1, - phid=f"PHID-DIFF-{i+1}", + provider_id=f"PHID-DIFF-{i+1}", review_task_id=f"task-{i}", mercurial_hash=hashlib.sha1(f"hg {i}".encode()).hexdigest(), repository=self.repo_try, @@ -245,18 +245,18 @@ def check_issue(issue): diffs = issue["diffs"] self.assertEqual(len(diffs), 1) diff = diffs[0] - self.assertTrue(diff["id"] > 0) self.assertEqual(diff["repository"], "http://repo.test/myrepo-try") # Revision rev = diff["revision"] self.assertTrue(rev["id"] > 0) - self.assertTrue(rev["phabricator_id"] > 0) + self.assertEqual(rev["provider"], "phabricator") + self.assertTrue(rev["provider_id"] > 0) self.assertEqual(rev["base_repository"], "http://repo.test/myrepo") self.assertEqual(rev["head_repository"], "http://repo.test/myrepo-try") self.assertEqual( - rev["phabricator_url"], - f"http://anotherphab.test/D{rev['phabricator_id']}", + rev["url"], + f"http://anotherphab.test/D{rev['provider_id']}", ) return True