Skip to content

Commit

Permalink
diff_warnings: port functionality (bug 1941342)
Browse files Browse the repository at this point in the history
  • Loading branch information
zzzeid committed Jan 20, 2025
1 parent 07c2ef0 commit e45d7fa
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 112 deletions.
67 changes: 0 additions & 67 deletions src/lando/api/legacy/api/diff_warnings.py

This file was deleted.

87 changes: 47 additions & 40 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,88 @@
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",
"/api/diff_warnings/",
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",
"/api/diff_warnings/",
json={"revision_id": 1, "diff_id": 1, "group": "LINT", "data": {}},
headers=phab_header,
**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/",
{
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
**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/",
{
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
**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}/",
**phab_header,
)

assert response.status_code == 200
Expand All @@ -92,66 +98,67 @@ 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/",
{
"revision_id": 1,
"diff_id": 1,
"group": "LINT",
"data": diff_warning_data,
},
headers=phab_header,
**phab_header,
)
assert response.status_code == 201

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

# Create another diff warning in a different group.
response = client.post(
"/diff_warnings",
json={
"/api/diff_warnings/",
{
"revision_id": 1,
"diff_id": 1,
"group": "GENERAL",
"data": diff_warning_data,
},
headers=phab_header,
**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),
},
]
112 changes: 111 additions & 1 deletion src/lando/api/views.py
Original file line number Diff line number Diff line change
@@ -1 +1,111 @@
# Create your views here.
from functools import wraps
from typing import Callable

from django import forms
from django.http import JsonResponse
from django.views import View

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


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.
"""

@classmethod
def _get_form(cls, method: str, data: dict):
"""Helper method to return validate data."""

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

_forms = {
# Used to filter DiffWarning objects.
"GET": {
"revision_id": forms.IntegerField(),
"diff_id": forms.IntegerField(),
"group": forms.CharField(),
},
# Used to create new DiffWarning objects.
"POST": {
"revision_id": forms.IntegerField(),
"diff_id": forms.IntegerField(),
"group": forms.CharField(),
"data": forms.JSONField(validators=[data_validator]),
},
}
return type(f"{cls.__name__}_{method}", (forms.Form,), _forms[method])(data)

@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.
"""
# TODO: validate whether revision/diff exist or not.
form = self._get_form(request.method, request.POST)
if form.is_valid():
data = form.cleaned_data
warning = DiffWarning(**data)
warning.save()
return JsonResponse(warning.serialize(), status=201)
else:
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."""
form = self._get_form(request.method, 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
)
else:
return JsonResponse({"errors": dict(form.errors)}, status=400)
Loading

0 comments on commit e45d7fa

Please sign in to comment.