Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions backend/code_review_backend/issues/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,20 @@ 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",
Expand Down
10 changes: 6 additions & 4 deletions backend/code_review_backend/issues/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,12 @@ 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:
provider = request.data["provider"]
provider_id = request.data["provider_id"]
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}
Expand Down Expand Up @@ -166,7 +168,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)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand All @@ -227,7 +227,7 @@ def build_revision_and_diff(self, data, task_id):
id=data["diff_id"],
defaults={
"repository": head_repository,
"phid": data["diff_phid"],
"provider_id": data["diff_phid"],
"review_task_id": task_id,
"mercurial_hash": data["mercurial_revision"],
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# 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",
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.1.6 on 2026-01-27 16:13

from django.db import migrations


class Migration(migrations.Migration):
dependencies = [
("issues", "0016_alter_revision_options_and_more"),
]

operations = [
migrations.RenameField(
model_name="diff",
old_name="phid",
new_name="provider_id",
),
]
47 changes: 28 additions & 19 deletions backend/code_review_backend/issues/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -72,43 +80,44 @@ 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}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an url such as {self.base_repository.url}/pull/{self.provider_id}/ with the task to map to individual commits remaining?

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)
provider_id = models.CharField(max_length=40, unique=True)
created = models.DateTimeField(auto_now_add=True)
updated = models.DateTimeField(auto_now=True)

Expand Down
22 changes: 11 additions & 11 deletions backend/code_review_backend/issues/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,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,
Expand All @@ -90,13 +90,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",
)


Expand All @@ -107,21 +107,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",
)


Expand All @@ -142,7 +142,7 @@ class Meta:
model = Diff
fields = (
"id",
"phid",
"provider_id",
"review_task_id",
"repository",
"mercurial_hash",
Expand Down Expand Up @@ -187,7 +187,7 @@ class Meta:
fields = (
"id",
"revision",
"phid",
"provider_id",
"review_task_id",
"repository",
"mercurial_hash",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading