Skip to content

Commit

Permalink
Add alternate accounts to the user model
Browse files Browse the repository at this point in the history
Introduce a way to store alternate accounts on the user, and add the
`PATCH /bot/users/<id:str>/alts` endpoint, which allows updating the
user's alt accounts to the alt accounts in the request..
  • Loading branch information
jchristgit committed Dec 12, 2023
1 parent 6ec6fff commit a67f5f2
Show file tree
Hide file tree
Showing 6 changed files with 306 additions and 7 deletions.
37 changes: 37 additions & 0 deletions pydis_site/apps/api/migrations/0093_user_alts.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Generated by Django 5.0 on 2023-12-12 09:01

import django.db.models.deletion
import pydis_site.apps.api.models.mixins
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('api', '0092_remove_redirect_filter_list'),
]

operations = [
migrations.CreateModel(
name='UserAltRelationship',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('source', models.ForeignKey(help_text='The source of this user to alternate account relationship', on_delete=django.db.models.deletion.CASCADE, to='api.user', verbose_name='Source')),
('target', models.ForeignKey(help_text='The target of this user to alternate account relationship', on_delete=django.db.models.deletion.CASCADE, related_name='+', to='api.user', verbose_name='Target')),
],
bases=(pydis_site.apps.api.models.mixins.ModelReprMixin, models.Model),
),
migrations.AddField(
model_name='user',
name='alts',
field=models.ManyToManyField(help_text='Known alternate accounts of this user. Manually linked.', through='api.UserAltRelationship', to='api.user', verbose_name='Alternative accounts'),
),
migrations.AddConstraint(
model_name='useraltrelationship',
constraint=models.UniqueConstraint(fields=('source', 'target'), name='api_useraltrelationship_unique_relationships'),
),
migrations.AddConstraint(
model_name='useraltrelationship',
constraint=models.CheckConstraint(check=models.Q(('source', models.F('target')), _negated=True), name='api_useraltrelationship_prevent_alt_to_self'),
),
]
37 changes: 37 additions & 0 deletions pydis_site/apps/api/models/bot/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ class User(ModelReprMixin, models.Model):
help_text="Whether this user is in our server.",
verbose_name="In Guild"
)
alts = models.ManyToManyField(
'self',
through='UserAltRelationship',
help_text="Known alternate accounts of this user. Manually linked.",
verbose_name="Alternative accounts"
)

def __str__(self):
"""Returns the name and discriminator for the current user, for display purposes."""
Expand All @@ -86,3 +92,34 @@ def username(self) -> str:
For usability in read-only fields such as Django Admin.
"""
return str(self)

class UserAltRelationship(ModelReprMixin, models.Model):
"""A relationship between a Discord user and its alts."""

source = models.ForeignKey(
User,
on_delete=models.CASCADE,
verbose_name="Source",
help_text="The source of this user to alternate account relationship",
)
target = models.ForeignKey(
User,
on_delete=models.CASCADE,
verbose_name="Target",
related_name='+',
help_text="The target of this user to alternate account relationship",
)

class Meta:
"""Add constraints to prevent users from being an alt of themselves."""

constraints = [
models.UniqueConstraint(
name="%(app_label)s_%(class)s_unique_relationships",
fields=["source", "target"]
),
models.CheckConstraint(
name="%(app_label)s_%(class)s_prevent_alt_to_self",
check=~models.Q(source=models.F("target")),
),
]
6 changes: 4 additions & 2 deletions pydis_site/apps/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ def update(self, queryset: QuerySet, validated_data: list) -> list:
return updated


class UserSerializer(ModelSerializer):
class UserSerializer(FrozenFieldsMixin, ModelSerializer):
"""A class providing (de-)serialization of `User` instances."""

# ID field must be explicitly set as the default id field is read-only.
Expand All @@ -679,7 +679,9 @@ class Meta:
"""Metadata defined for the Django REST Framework."""

model = User
fields = ('id', 'name', 'discriminator', 'roles', 'in_guild')
fields = ('id', 'name', 'discriminator', 'roles', 'in_guild', 'alts')
# This should be edited on a separate endpoint only
frozen_fields = ('alts',)
depth = 1
list_serializer_class = UserListSerializer

Expand Down
2 changes: 1 addition & 1 deletion pydis_site/apps/api/tests/test_infractions.py
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ def setUpTestData(cls):
def check_expanded_fields(self, infraction):
for key in ('user', 'actor'):
obj = infraction[key]
for field in ('id', 'name', 'discriminator', 'roles', 'in_guild'):
for field in ('id', 'name', 'discriminator', 'roles', 'in_guild', 'alts'):
self.assertTrue(field in obj, msg=f'field "{field}" missing from {key}')

def test_list_expanded(self):
Expand Down
148 changes: 148 additions & 0 deletions pydis_site/apps/api/tests/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def setUpTestData(cls):
def test_accepts_valid_data(self):
url = reverse('api:bot:user-list')
data = {
'alts': [],
'id': 42,
'name': "Test",
'discriminator': 42,
Expand Down Expand Up @@ -199,13 +200,15 @@ def test_multiple_users_patch(self):
url = reverse("api:bot:user-bulk-patch")
data = [
{
"alts": [],
"id": 1,
"name": "User 1 patched!",
"discriminator": 1010,
"roles": [self.role_developer.id],
"in_guild": False
},
{
"alts": [],
"id": 2,
"name": "User 2 patched!"
}
Expand Down Expand Up @@ -646,3 +649,148 @@ def test_search_lookup_of_unknown_user(self) -> None:
result = response.json()
self.assertEqual(result['count'], 0)
self.assertEqual(result['results'], [])


class UserAltUpdateTests(AuthenticatedAPITestCase):
@classmethod
def setUpTestData(cls):
cls.user_1 = User.objects.create(
id=12095219,
name=f"Test user {random.randint(100, 1000)}",
discriminator=random.randint(1, 9999),
in_guild=True,
)
cls.user_2 = User.objects.create(
id=18259125,
name=f"Test user {random.randint(100, 1000)}",
discriminator=random.randint(1, 9999),
in_guild=True,
)

def test_adding_existing_alt(self) -> None:
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
data = self.user_2.id
response = self.client.post(url, data)
self.assertEqual(response.status_code, 201)

updated_user = response.json()
self.assertEqual(len(updated_user['alts']), 1)
self.assertEqual(updated_user['alts'][0]['id'], self.user_2.id)

self.user_1.refresh_from_db()
self.user_2.refresh_from_db()

self.assertQuerySetEqual(self.user_1.alts.all(), [self.user_2])
self.assertQuerySetEqual(self.user_2.alts.all(), [self.user_1])

def test_adding_existing_alt_twice(self) -> None:
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
data = self.user_2.id
initial_response = self.client.post(url, data)
self.assertEqual(initial_response.status_code, 201)

response = self.client.post(url, data)
self.assertEqual(response.status_code, 201)

def test_removing_existing_alt(self) -> None:
self.user_1.alts.set((self.user_2.id,))
self.user_1.save()

data = self.user_2.id
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
response = self.client.delete(url, data)

self.assertEqual(response.status_code, 204)

self.user_1.refresh_from_db()
self.user_2.refresh_from_db()

self.assertFalse(self.user_1.alts.all().exists())
self.assertFalse(self.user_2.alts.all().exists())

def test_removing_unknown_alt(self) -> None:
data = self.user_1.id + self.user_2.id
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
response = self.client.delete(url, data)
self.assertEqual(response.status_code, 400)
self.assertEqual(response.json(), {
'non_field_errors': ["Specified account is not a known alternate account of this user"]
})

def test_add_alt_returns_error_for_non_int_request_body(self) -> None:
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
data = {'hello': 'joe'}
response = self.client.post(url, data)
self.assertEqual(response.status_code, 400)

def test_remove_alt_returns_error_for_non_int_request_body(self) -> None:
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
data = {'hello': 'joe'}
response = self.client.delete(url, data)
self.assertEqual(response.status_code, 400)

def test_adding_alt_to_user_that_does_not_exist(self) -> None:
"""Patching a user's alts for a user that doesn't exist should return a 404."""
url = reverse('api:bot:user-alts', args=(self.user_1.id + self.user_2.id,))
data = self.user_2.id
response = self.client.post(url, data)
self.assertEqual(response.status_code, 404)

def test_adding_alt_that_does_not_exist_to_user(self) -> None:
"""Patching a user's alts with an alt that is unknown should return a 400."""
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
data = self.user_1.id + self.user_2.id
response = self.client.post(url, data)
self.assertEqual(response.status_code, 400)
self.assertEqual(response.json(), {
'non_field_errors': ["Unknown alternate account ID"]
})

def test_cannot_add_self_as_alt_account(self) -> None:
"""The existing account may not be an alt of itself."""
url = reverse('api:bot:user-alts', args=(self.user_1.id,))
data = self.user_1.id
response = self.client.post(url, data)
self.assertEqual(response.status_code, 400)
self.assertEqual(response.json(), {
'non_field_errors': ["The user may not be an alternate account of itself"]
})

def test_cannot_update_alts_on_regular_user_patch_route(self) -> None:
"""The regular user update route does not allow editing the alts."""
url = reverse('api:bot:user-detail', args=(self.user_1.id,))
data = {'alts': [self.user_2.id]}
response = self.client.patch(url, data)
self.assertEqual(response.status_code, 200) # XXX: This seems to be a DRF bug
self.assertEqual(response.json()['alts'], [])

self.user_1.refresh_from_db()
self.assertQuerySetEqual(self.user_1.alts.all(), ())
self.user_2.refresh_from_db()
self.assertQuerySetEqual(self.user_2.alts.all(), ())

def test_cannot_update_alts_on_bulk_user_patch_route(self) -> None:
"""The bulk user update route does not allow editing the alts."""
url = reverse('api:bot:user-bulk-patch')
data = [{'id': self.user_1.id, 'alts': [self.user_2.id]}]
response = self.client.patch(url, data)
self.assertEqual(response.status_code, 400)
self.assertEqual(response.json(), {'non_field_errors': ['Insufficient data provided.']})

self.user_1.refresh_from_db()
self.assertQuerySetEqual(self.user_1.alts.all(), ())
self.user_2.refresh_from_db()
self.assertQuerySetEqual(self.user_2.alts.all(), ())

def test_user_bulk_patch_does_not_discard_alts(self) -> None:
"""The bulk user update route should not modify the alts."""
url = reverse('api:bot:user-bulk-patch')
self.user_1.alts.set((self.user_2,))
data = [{'id': self.user_1.id, 'name': "Joe Armstrong"}]
response = self.client.patch(url, data)
self.assertEqual(response.status_code, 200)

self.user_1.refresh_from_db()
self.assertQuerySetEqual(self.user_1.alts.all(), (self.user_2,))
self.user_2.refresh_from_db()
self.assertQuerySetEqual(self.user_2.alts.all(), (self.user_1,))
83 changes: 79 additions & 4 deletions pydis_site/apps/api/viewsets/bot/user.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from collections import OrderedDict

from django.core.exceptions import ObjectDoesNotExist
from django.db import IntegrityError
from django.db.models import Q
from django_filters.rest_framework import DjangoFilterBackend
from rest_framework import fields, status
Expand Down Expand Up @@ -203,8 +205,9 @@ class UserViewSet(ModelViewSet):
- 404: if the user with the given `snowflake` could not be found
### PATCH /bot/users/<snowflake:int>
Update the user with the given `snowflake`.
All fields in the request body are optional.
Update the user with the given `snowflake`. All fields in the request body
are optional. Note that editing the `'alts'` field is not possible this
way, use the `alts` endpoint documented below for this.
#### Request body
>>> {
Expand All @@ -220,9 +223,39 @@ class UserViewSet(ModelViewSet):
- 400: if the request body was invalid, see response body for details
- 404: if the user with the given `snowflake` could not be found
### POST /bot/users/<snowflake:int>/alts
Add the alternative account given in the request body to the alternative
accounts for the given user snowflake. The request body contains the user
IDs to link to this user, as a plain integer. Users will be linked
symmetrically. Returns the updated user.
#### Request body
>>> int
#### Status codes
- 200: returned on success
- 400: if the request body was invalid, including if the user in the
request body could not be found in the database
- 404: if the user with the given `snowflake` could not be found
### DELETE /bot/users/<snowflake:int>/alts
Remove the alternative account given in the request body from the
alternative accounts of the given user snowflake. The request body contains
the user ID to remove from the association to this user, as a plain
integer. Users will be linked symmetrically. Returns the updated user.
#### Request body
>>> int
#### Status codes
- 204: returned on success
- 400: if the user in the request body was not found as an alt account
- 404: if the user with the given `snowflake` could not be found
### BULK PATCH /bot/users/bulk_patch
Update users with the given `ids` and `details`.
`id` field and at least one other field is mandatory.
Update users with the given `ids` and `details`. `id` field and at least
one other field is mandatory. Note that editing the `'alts'` field is not
possible using this endpoint.
#### Request body
>>> [
Expand Down Expand Up @@ -284,6 +317,48 @@ def bulk_patch(self, request: Request) -> Response:

return Response(serializer.data, status=status.HTTP_200_OK)

@action(detail=True, methods=['POST'], name="Add alternate account",
url_name='alts', url_path='alts')
def add_alt(self, request: Request, pk: str) -> Response:
"""Associate the given account to the user's alternate accounts."""
user = self.get_object()
if not isinstance(request.data, int):
raise ParseError(detail={"non_field_errors": ["Request body should be an integer"]})

try:
alt = User.objects.get(id=request.data)
except ObjectDoesNotExist:
raise ParseError(detail={"non_field_errors": ["Unknown alternate account ID"]})
try:
user.alts.add(alt)
except IntegrityError as err:
if err.__cause__.diag.constraint_name == 'api_useraltrelationship_prevent_alt_to_self':
raise ParseError(detail={
"non_field_errors": ["The user may not be an alternate account of itself"]
})

raise # pragma: no cover

serializer = self.get_serializer(instance=user)
return Response(serializer.data, status=status.HTTP_201_CREATED)

# See https://www.django-rest-framework.org/api-guide/viewsets/#routing-additional-http-methods-for-extra-actions
@add_alt.mapping.delete
def remove_alt(self, request: Request, pk: str) -> Response:
"""Disassociate the given account from the user's alternate accounts."""
user = self.get_object()
if not isinstance(request.data, int):
raise ParseError(detail={"non_field_errors": ["Request body should be an integer"]})
try:
alt = user.alts.get(id=request.data)
except ObjectDoesNotExist:
raise ParseError(detail={
'non_field_errors': ["Specified account is not a known alternate account of this user"]
})

user.alts.remove(alt)
return Response('', status=status.HTTP_204_NO_CONTENT)

@action(detail=True)
def metricity_data(self, request: Request, pk: str | None = None) -> Response:
"""Request handler for metricity_data endpoint."""
Expand Down

0 comments on commit a67f5f2

Please sign in to comment.