diff --git a/backend/code_review_backend/app/urls.py b/backend/code_review_backend/app/urls.py index f6f4cd8c8..b7862ab28 100644 --- a/backend/code_review_backend/app/urls.py +++ b/backend/code_review_backend/app/urls.py @@ -11,6 +11,7 @@ from rest_framework import permissions from code_review_backend.issues import api +from code_review_backend.issues.v2 import api as api_v2 # Build Swagger schema view schema_view = get_schema_view( @@ -28,6 +29,7 @@ urlpatterns = [ path("", lambda request: redirect("docs/", permanent=False)), path("v1/", include(api.urls)), + path("v2/", include(api_v2.urls)), path("admin/", admin.site.urls), path( r"docs\.json|\.yaml)", diff --git a/backend/code_review_backend/issues/v2/__init__.py b/backend/code_review_backend/issues/v2/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/backend/code_review_backend/issues/v2/api.py b/backend/code_review_backend/issues/v2/api.py new file mode 100644 index 000000000..8cee8f27c --- /dev/null +++ b/backend/code_review_backend/issues/v2/api.py @@ -0,0 +1,127 @@ +# 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.core.exceptions import BadRequest +from django.db.models import Exists, OuterRef +from django.shortcuts import get_object_or_404 +from django.urls import path +from django.utils.functional import cached_property +from rest_framework.exceptions import ValidationError +from rest_framework.generics import ListAPIView + +from code_review_backend.issues.models import Diff, IssueLink +from code_review_backend.issues.v2.serializers import DiffIssuesSerializer + + +class IssueList(ListAPIView): + """ + Returns issues that are linked to a diff, and depending on the selected mode: + * known: Issues that have also being detected from the base repository (e.g. mozilla-central). + `previous_diff_id` is always returned as null when listing known issues. + * unresolved: Issues that were linked to the previous diff of the same parent revision, and are + still present on the current diff. Filters out issues that are known by the backend. + * closed: Issues that were listed on the previous diff of the same parent revision, but does + not exist on the current diff. + + This endpoint does not uses pagination. + """ + + allowed_modes = ["unresolved", "known", "closed"] + pagination_class = None + + def get_serializer(self, queryset, many=True): + return DiffIssuesSerializer( + { + "previous_diff_id": self.previous_diff and self.previous_diff.id, + "issues": queryset, + }, + context=self.get_serializer_context(), + ) + + @cached_property + def diff(self): + return get_object_or_404( + Diff.objects.select_related("revision"), id=self.kwargs["diff_id"] + ) + + @cached_property + def previous_diff(self): + if self.mode == "known": + # No need to search for a previous diff for known issues + return None + return ( + self.diff.revision.diffs.filter(created__lt=self.diff.created) + .order_by("created") + .last() + ) + + @cached_property + def mode(self): + if (mode := self.kwargs["mode"]) not in self.allowed_modes: + raise ValidationError( + detail=f"mode argument must be one of {self.allowed_modes}" + ) + return mode + + def get_queryset(self): + return getattr(self, f"{self.mode}_issues").filter(**{self.mode: True}) + + def distinct_issues(self, qs): + """ + Convert a list of issue links to unique couples of (issue_id, issue_hash) + """ + attributes = ("issue_id", "issue__hash") + return qs.order_by(*attributes).values(*attributes).distinct(*attributes) + + @property + def known_issues(self): + return self.distinct_issues( + self.diff.issue_links.annotate( + known=Exists( + IssueLink.objects.filter( + revision__head_repository_id=self.diff.revision.base_repository_id, + issue=OuterRef("issue"), + ) + ) + ) + ) + + @property + def unresolved_issues(self): + if not self.previous_diff: + raise BadRequest("No previous diff was found to compare issues") + return self.distinct_issues( + self.known_issues.filter(known=False).annotate( + unresolved=Exists( + IssueLink.objects.filter( + diff=self.previous_diff, + issue=OuterRef("issue"), + ) + ) + ) + ) + + @property + def closed_issues(self): + if not self.previous_diff: + raise BadRequest("No previous diff was found to compare issues") + return self.distinct_issues( + self.previous_diff.issue_links.annotate( + closed=~Exists( + IssueLink.objects.filter( + diff=self.diff, + issue=OuterRef("issue"), + ) + ) + ) + ) + + +urls = [ + path( + "diff//issues//", + IssueList.as_view(), + name="issue-list", + ), +] diff --git a/backend/code_review_backend/issues/v2/serializers.py b/backend/code_review_backend/issues/v2/serializers.py new file mode 100644 index 000000000..c384c4efa --- /dev/null +++ b/backend/code_review_backend/issues/v2/serializers.py @@ -0,0 +1,19 @@ +from rest_framework import serializers + +from code_review_backend.issues.models import IssueLink + + +class IssueLinkHashSerializer(serializers.ModelSerializer): + """Serialize an Issue hash from the IssueLink M2M""" + + id = serializers.UUIDField(source="issue_id", read_only=True) + hash = serializers.CharField(source="issue__hash", max_length=32, read_only=True) + + class Meta: + model = IssueLink + fields = ("id", "hash") + + +class DiffIssuesSerializer(serializers.Serializer): + previous_diff_id = serializers.UUIDField(allow_null=True, read_only=True) + issues = IssueLinkHashSerializer(many=True) diff --git a/backend/code_review_backend/issues/v2/tests/__init__.py b/backend/code_review_backend/issues/v2/tests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/backend/code_review_backend/issues/v2/tests/test_list_issues.py b/backend/code_review_backend/issues/v2/tests/test_list_issues.py new file mode 100644 index 000000000..fc0939268 --- /dev/null +++ b/backend/code_review_backend/issues/v2/tests/test_list_issues.py @@ -0,0 +1,175 @@ +from rest_framework import status +from rest_framework.test import APITestCase + +from code_review_backend.issues.models import Issue, Repository + + +class ListIssuesTestCase(APITestCase): + def setUp(self): + # Create the main repository + self.repo_main = Repository.objects.create( + slug="mozilla-central", url="https://hg.mozilla.org/mozilla-central" + ) + self.repo_try = Repository.objects.create( + slug="myrepo-try", url="http://repo.test/try" + ) + + self.issues = Issue.objects.bulk_create( + [ + Issue(**attrs) + for attrs in [ + {"hash": "0" * 32}, + {"hash": "1" * 32}, + {"hash": "2" * 32}, + {"hash": "3" * 32}, + ] + ] + ) + + # Create historic revision with some issues + self.revision_main = self.repo_main.head_revisions.create( + phabricator_id=456, + phabricator_phid="PHID-REV-XXX", + title="Main revision", + base_repository=self.repo_main, + head_repository=self.repo_main, + ) + self.revision_main.issue_links.create(issue=self.issues[0]) + self.revision_main.issue_links.create(issue=self.issues[1]) + + # Create a new revions, with two successive diffs + self.revision_last = self.repo_try.head_revisions.create( + phabricator_id=1337, + phabricator_phid="PHID-REV-YYY", + title="Bug XXX - Yet another bug", + base_repository=self.repo_main, + head_repository=self.repo_try, + ) + self.diff1 = self.revision_last.diffs.create( + id=1, + repository=self.repo_try, + review_task_id="deadbeef123", + phid="PHID-DIFF-xxx", + ) + self.diff1.issue_links.create(revision=self.revision_last, issue=self.issues[0]) + self.diff1.issue_links.create(revision=self.revision_last, issue=self.issues[2]) + self.diff1.issue_links.create(revision=self.revision_last, issue=self.issues[3]) + + self.diff2 = self.revision_last.diffs.create( + id=2, + repository=self.repo_try, + review_task_id="coffee12345", + phid="PHID-DIFF-yyy", + ) + self.diff2.issue_links.create(revision=self.revision_last, issue=self.issues[0]) + self.diff2.issue_links.create(revision=self.revision_last, issue=self.issues[2]) + + def test_list_issues_invalid_mode(self): + with self.assertNumQueries(0): + response = self.client.get( + f"/v2/diff/{self.diff2.id}/issues/test/", format="json" + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertListEqual( + response.json(), + ["mode argument must be one of ['unresolved', 'known', 'closed']"], + ) + + def test_list_issues_invalid_diff(self): + with self.assertNumQueries(1): + response = self.client.get("/v2/diff/424242/issues/known/") + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + + def test_list_issues_known(self): + with self.assertNumQueries(2): + response = self.client.get(f"/v2/diff/{self.diff2.id}/issues/known/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.maxDiff = None + self.assertDictEqual( + response.json(), + { + "previous_diff_id": None, + "issues": [ + {"id": str(self.issues[0].id), "hash": self.issues[0].hash}, + ], + }, + ) + + def test_list_issues_unresolved_first_diff(self): + """No issue can be marked as unresolved on a new diff""" + with self.assertNumQueries(2): + response = self.client.get( + f"/v2/diff/{self.diff1.id}/issues/unresolved/", format="json" + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_list_issues_unresolved(self): + with self.assertNumQueries(3): + response = self.client.get(f"/v2/diff/{self.diff2.id}/issues/unresolved/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertDictEqual( + response.json(), + { + "previous_diff_id": "1", + "issues": [ + {"id": str(self.issues[2].id), "hash": self.issues[2].hash}, + ], + }, + ) + + def test_list_issues_closed_first_diff(self): + """No issue can be marked as closed on a new diff""" + with self.assertNumQueries(2): + response = self.client.get(f"/v2/diff/{self.diff1.id}/issues/closed/") + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + def test_list_issues_closed(self): + with self.assertNumQueries(3): + response = self.client.get(f"/v2/diff/{self.diff2.id}/issues/closed/") + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertDictEqual( + response.json(), + { + "previous_diff_id": "1", + "issues": [{"id": str(self.issues[3].id), "hash": self.issues[3].hash}], + }, + ) + + def test_list_issues_new_diff_reopen(self): + """ + Open a new diff with 1 known issue and one that has been closed in the previous diff. + Both are listed as new, and the two issues of the previous diff listed as closed. + """ + diff = self.revision_last.diffs.create( + id=3, + repository=self.repo_try, + review_task_id="42", + phid="PHID-DIFF-42", + ) + new_issue = Issue.objects.create(hash="4" * 32) + diff.issue_links.create(revision=self.revision_last, issue=new_issue) + diff.issue_links.create(revision=self.revision_last, issue=self.issues[3]) + self.assertDictEqual( + self.client.get(f"/v2/diff/{diff.id}/issues/unresolved/").json(), + { + "previous_diff_id": "2", + "issues": [], + }, + ) + self.assertDictEqual( + self.client.get(f"/v2/diff/{diff.id}/issues/known/").json(), + { + "previous_diff_id": None, + "issues": [], + }, + ) + self.assertDictEqual( + self.client.get(f"/v2/diff/{diff.id}/issues/closed/").json(), + { + "previous_diff_id": "2", + "issues": [ + {"id": str(self.issues[0].id), "hash": self.issues[0].hash}, + {"id": str(self.issues[2].id), "hash": self.issues[2].hash}, + ], + }, + )