Skip to content

Commit

Permalink
diff_warnings: port functionality (bug 1941342) (#197)
Browse files Browse the repository at this point in the history
- port diff warnings endpoint handling
- unskip tests
- update tests, views, forms to work with new implementation
- rename diff_warning.error_breakdown back to diff_warning.data
  • Loading branch information
zzzeid authored Jan 24, 2025
1 parent 76a6c2c commit 09d8542
Show file tree
Hide file tree
Showing 6 changed files with 196 additions and 115 deletions.
67 changes: 0 additions & 67 deletions src/lando/api/legacy/api/diff_warnings.py

This file was deleted.

99 changes: 57 additions & 42 deletions src/lando/api/tests/test_diff_warnings.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import json

import pytest

from lando.main.models.revision import (
Expand All @@ -6,84 +8,93 @@
DiffWarningStatus,
)

pytest.skip(allow_module_level=True)
# NOTE: this should be ported as an API endpoint.


@pytest.fixture
def phab_header(phabdouble):
user = phabdouble.user(username="test")
return {"X-Phabricator-API-Key": user["apiKey"]}
return {"HTTP_X_Phabricator_API_Key": user["apiKey"]}


@pytest.fixture
def diff_warning_data():
return {"message": "this is a test warning"}
return json.dumps({"message": "this is a test warning"})


def test_diff_warning_create_bad_request(db, client):
@pytest.mark.django_db(transaction=True)
def test_diff_warning_create_bad_request(client):
"""Ensure a request that is missing required data returns an error."""
response = client.post(
"/diff_warnings",
json={},
"/api/diff_warnings/",
data={},
content_type="application/json",
)
assert response.status_code == 400


def test_diff_warning_create_bad_request_no_message(db, client, phab_header):
@pytest.mark.django_db(transaction=True)
def test_diff_warning_create_bad_request_no_message(client, phab_header):
"""Ensure a request with incorrect data returns an error."""
response = client.post(
"/diff_warnings",
json={"revision_id": 1, "diff_id": 1, "group": "LINT", "data": {}},
headers=phab_header,
"/api/diff_warnings/",
data={"revision_id": 1, "diff_id": 1, "group": "LINT", "data": {}},
content_type="application/json",
**phab_header,
)
assert response.status_code == 400


def test_diff_warning_create(db, client, diff_warning_data, phab_header):
@pytest.mark.django_db(transaction=True)
def test_diff_warning_create(client, diff_warning_data, phab_header):
"""Ensure that a warning is created correctly according to provided parameters."""
response = client.post(
"/diff_warnings",
json={
"/api/diff_warnings/",
data={
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
content_type="application/json",
**phab_header,
)

assert response.status_code == 201
assert "id" in response.json

pk = response.json["id"]
json_response = response.json()
assert "id" in json_response

pk = json_response["id"]
warning = DiffWarning.objects.get(pk=pk)
assert warning.group == DiffWarningGroup.LINT
assert warning.revision_id == 1
assert warning.diff_id == 1
assert warning.status == DiffWarningStatus.ACTIVE
assert warning.data == diff_warning_data
assert warning.data == json.loads(diff_warning_data)


def test_diff_warning_delete(db, client, diff_warning_data, phab_header):
@pytest.mark.django_db(transaction=True)
def test_diff_warning_delete(client, diff_warning_data, phab_header):
"""Ensure that a DELETE request will archive a warning."""
response = client.post(
"/diff_warnings",
json={
"/api/diff_warnings/",
data={
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
content_type="application/json",
**phab_header,
)
assert response.status_code == 201
pk = response.json["id"]
pk = response.json()["id"]
warning = DiffWarning.objects.get(pk=pk)
assert warning.status == DiffWarningStatus.ACTIVE

response = client.delete(
f"/diff_warnings/{pk}",
headers=phab_header,
f"/api/diff_warnings/{pk}/",
content_type="application/json",
**phab_header,
)

assert response.status_code == 200
Expand All @@ -92,66 +103,70 @@ def test_diff_warning_delete(db, client, diff_warning_data, phab_header):
assert warning.status == DiffWarningStatus.ARCHIVED


def test_diff_warning_get(db, client, diff_warning_data, phab_header):
@pytest.mark.django_db(transaction=True, reset_sequences=True)
def test_diff_warning_get(client, diff_warning_data, phab_header):
"""Ensure that the API returns a properly serialized list of warnings."""
response = client.post(
"/diff_warnings",
json={
"/api/diff_warnings/",
data={
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
content_type="application/json",
**phab_header,
)
assert response.status_code == 201

response = client.post(
"/diff_warnings",
json={
"/api/diff_warnings/",
data={
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
content_type="application/json",
**phab_header,
)
assert response.status_code == 201

# Create another diff warning in a different group.
response = client.post(
"/diff_warnings",
json={
"/api/diff_warnings/",
data={
"revision_id": 1,
"diff_id": 1,
"group": "GENERAL",
"data": diff_warning_data,
},
headers=phab_header,
content_type="application/json",
**phab_header,
)
assert response.status_code == 201

response = client.get(
"/diff_warnings",
query_string={"revision_id": 1, "diff_id": 1, "group": "LINT"},
headers=phab_header,
"/api/diff_warnings/",
query_params={"revision_id": 1, "diff_id": 1, "group": "LINT"},
**phab_header,
)
assert response.status_code == 200
assert response.json == [
assert response.json() == [
{
"diff_id": 1,
"group": "LINT",
"id": 1,
"revision_id": 1,
"status": "ACTIVE",
"data": diff_warning_data,
"data": json.loads(diff_warning_data),
},
{
"diff_id": 1,
"group": "LINT",
"id": 2,
"revision_id": 1,
"status": "ACTIVE",
"data": diff_warning_data,
"data": json.loads(diff_warning_data),
},
]
107 changes: 106 additions & 1 deletion src/lando/api/views.py
Original file line number Diff line number Diff line change
@@ -1 +1,106 @@
# Create your views here.
import json
from functools import wraps
from typing import Callable

from django import forms
from django.http import JsonResponse
from django.utils.decorators import method_decorator
from django.views import View
from django.views.decorators.csrf import csrf_exempt

from lando.main.models.revision import DiffWarning, DiffWarningStatus
from lando.utils.phabricator import get_phabricator_client


def phabricator_api_key_required(func: callable) -> Callable:
"""A simple wrapper that checks for a valid Phabricator API token."""

@wraps(func)
def _wrapper(self, request, *args, **kwargs):
HEADER = "X-Phabricator-API-Key"
if HEADER not in request.headers:
return JsonResponse({"error": f"{HEADER} missing."}, status=400)

api_key = request.headers[HEADER]
client = get_phabricator_client(api_key=api_key)
has_valid_token = client.verify_api_token()

if not has_valid_token:
return JsonResponse({}, 401)

return func(self, request, *args, **kwargs)

return _wrapper


@method_decorator(csrf_exempt, name="dispatch")
class LegacyDiffWarningView(View):
"""
This class provides the API controllers for the legacy `DiffWarning` model.
These API endpoints can be used by clients (such as Code Review bot) to
get, create, or archive warnings.
"""

@phabricator_api_key_required
def post(self, request):
"""Create a new `DiffWarning` based on provided revision and diff IDs.
Args:
data (dict): A dictionary containing data to store in the warning. `data`
should contain at least a `message` key that contains the message to
show in the warning.
Returns:
dict: a dictionary representation of the object that was created.
"""

class Form(forms.Form):
def data_validator(data):
if not data or "message" not in data:
raise forms.ValidationError(
"Provided data is missing the message value"
)

revision_id = forms.IntegerField()
diff_id = forms.IntegerField()
group = forms.CharField()
data = forms.JSONField(validators=[data_validator])

# TODO: validate whether revision/diff exist or not.
form = Form(json.loads(request.body))
if form.is_valid():
data = form.cleaned_data
warning = DiffWarning.objects.create(**data)
return JsonResponse(warning.serialize(), status=201)

return JsonResponse({"errors": dict(form.errors)}, status=400)

@phabricator_api_key_required
def delete(self, request, diff_warning_id):
"""Archive a `DiffWarning` based on provided pk."""
warning = DiffWarning.objects.get(pk=diff_warning_id)
if not warning:
return JsonResponse({}, status=404)

warning.status = DiffWarningStatus.ARCHIVED
warning.save()
return JsonResponse(warning.serialize(), status=200)

@phabricator_api_key_required
def get(self, request, **kwargs):
"""Return a list of active revision diff warnings, if any."""

class Form(forms.Form):
revision_id = forms.IntegerField()
diff_id = forms.IntegerField()
group = forms.CharField()

form = Form(request.GET)
if form.is_valid():
warnings = DiffWarning.objects.filter(**form.cleaned_data).all()
return JsonResponse(
[warning.serialize() for warning in warnings], status=200, safe=False
)

return JsonResponse({"errors": dict(form.errors)}, status=400)
Loading

0 comments on commit 09d8542

Please sign in to comment.