From c1547cab8b54454a5ea33859ff5cd6e1d9f19464 Mon Sep 17 00:00:00 2001 From: Adarnof Date: Wed, 6 Sep 2017 23:00:12 -0400 Subject: [PATCH] Fix that state deletion bug. Thanks @basraah Also sets state to guest on inactivation, and reassesses on reactivation. With a unit test to boot! --- authentication/managers.py | 23 +++++-- .../migrations/0015_user_profiles.py | 62 ++++++++++++------- authentication/models.py | 16 +++-- authentication/signals.py | 31 ++++------ authentication/tests.py | 12 ++++ 5 files changed, 91 insertions(+), 53 deletions(-) diff --git a/authentication/managers.py b/authentication/managers.py index 4dc9631b..abac8f70 100755 --- a/authentication/managers.py +++ b/authentication/managers.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals from django.db.models import Manager, QuerySet, Q +from django.db import transaction from eveonline.managers import EveManager from eveonline.models import EveCharacter import logging @@ -36,6 +37,21 @@ class StateQuerySet(QuerySet): else: return self.none() + def get_for_user(self, user): + states = self.available_to_user(user) + if states.exists(): + return states[0] + else: + from authentication.models import get_guest_state + return get_guest_state() + + def delete(self): + with transaction.atomic(): + for state in self: + for profile in state.userprofile_set.all(): + profile.assign_state(state=self.model.objects.exclude(pk=state.pk).get_for_user(profile.user)) + super(StateQuerySet, self).delete() + class StateManager(Manager): def get_queryset(self): @@ -56,9 +72,4 @@ class StateManager(Manager): return get_guest_state() def get_for_user(self, user): - states = self.get_queryset().available_to_user(user) - if states.exists(): - return states[0] - else: - from authentication.models import get_guest_state - return get_guest_state() + return self.get_queryset().get_for_user(user) diff --git a/authentication/migrations/0015_user_profiles.py b/authentication/migrations/0015_user_profiles.py index a0857eb9..c46051fb 100644 --- a/authentication/migrations/0015_user_profiles.py +++ b/authentication/migrations/0015_user_profiles.py @@ -182,12 +182,8 @@ class Migration(migrations.Migration): fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('owner_hash', models.CharField(max_length=28, unique=True)), - ('character', - models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='character_ownership', - to='eveonline.EveCharacter')), - ('user', - models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='character_ownerships', - to=settings.AUTH_USER_MODEL)), + ('character', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='character_ownership', to='eveonline.EveCharacter')), + ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='character_ownerships', to=settings.AUTH_USER_MODEL)), ], options={ 'default_permissions': ('change', 'delete'), @@ -199,18 +195,15 @@ class Migration(migrations.Migration): fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('name', models.CharField(max_length=20, unique=True)), - ('priority', models.IntegerField(unique=True, - help_text="Users get assigned the state with the highest priority available to them.")), - ('public', models.BooleanField(default=False, help_text="Make this state available to any character.")), - ('member_alliances', models.ManyToManyField(blank=True, to='eveonline.EveAllianceInfo', - help_text="Alliances to whose members this state is available.")), - ('member_characters', models.ManyToManyField(blank=True, to='eveonline.EveCharacter', - help_text="Characters to which this state is available.")), - ('member_corporations', models.ManyToManyField(blank=True, to='eveonline.EveCorporationInfo', - help_text="Corporations to whose members this state is available.")), + ('priority', models.IntegerField(help_text='Users get assigned the state with the highest priority available to them.', unique=True)), + ('public', models.BooleanField(default=False, help_text='Make this state available to any character.')), + ('member_alliances', models.ManyToManyField(blank=True, help_text='Alliances to whose members this state is available.', to='eveonline.EveAllianceInfo')), + ('member_characters', models.ManyToManyField(blank=True, help_text='Characters to which this state is available.', to='eveonline.EveCharacter')), + ('member_corporations', models.ManyToManyField(blank=True, help_text='Corporations to whose members this state is available.', to='eveonline.EveCorporationInfo')), ('permissions', models.ManyToManyField(blank=True, to='auth.Permission')), ], options={ + 'default_permissions': ('change',), 'ordering': ['-priority'], }, ), @@ -218,14 +211,9 @@ class Migration(migrations.Migration): name='UserProfile', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('main_character', - models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, - to='eveonline.EveCharacter')), - ('state', models.ForeignKey(on_delete=models.SET(authentication.models.get_guest_state), - to='authentication.State', - default=authentication.models.get_guest_state_pk)), - ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='profile', - to=settings.AUTH_USER_MODEL)), + ('main_character', models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='eveonline.EveCharacter')), + ('state', models.ForeignKey(default=authentication.models.get_guest_state_pk, on_delete=django.db.models.deletion.SET_DEFAULT, to='authentication.State')), + ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='profile', to=settings.AUTH_USER_MODEL)), ], options={ 'default_permissions': ('change',), @@ -244,4 +232,32 @@ class Migration(migrations.Migration): name='AuthServicesInfo', ), migrations.RunPython(disable_passwords, migrations.RunPython.noop), + migrations.CreateModel( + name='ProxyPermission', + fields=[ + ], + options={ + 'proxy': True, + 'verbose_name': 'permission', + 'verbose_name_plural': 'permissions', + }, + bases=('auth.permission',), + managers=[ + ('objects', django.contrib.auth.models.PermissionManager()), + ], + ), + migrations.CreateModel( + name='ProxyUser', + fields=[ + ], + options={ + 'proxy': True, + 'verbose_name': 'user', + 'verbose_name_plural': 'users', + }, + bases=('auth.user',), + managers=[ + ('objects', django.contrib.auth.models.UserManager()), + ], + ), ] diff --git a/authentication/models.py b/authentication/models.py index 1deaa360..4e8bcd11 100755 --- a/authentication/models.py +++ b/authentication/models.py @@ -1,6 +1,6 @@ from __future__ import unicode_literals from django.utils.encoding import python_2_unicode_compatible -from django.db import models +from django.db import models, transaction from django.contrib.auth.models import User, Permission from authentication.managers import CharacterOwnershipManager, StateManager from eveonline.models import EveCharacter, EveCorporationInfo, EveAllianceInfo @@ -30,7 +30,6 @@ class State(models.Model): class Meta: ordering = ['-priority'] - default_permissions = ('change',) def __str__(self): return self.name @@ -41,6 +40,12 @@ class State(models.Model): def available_to_user(self, user): return self in State.objects.available_to_user(user) + def delete(self, **kwargs): + with transaction.atomic(): + for profile in self.userprofile_set.all(): + profile.assign_state(state=State.objects.exclude(pk=self.pk).get_for_user(profile.user)) + super(State, self).delete(**kwargs) + def get_guest_state(): try: @@ -60,10 +65,11 @@ class UserProfile(models.Model): user = models.OneToOneField(User, related_name='profile', on_delete=models.CASCADE) main_character = models.OneToOneField(EveCharacter, blank=True, null=True, on_delete=models.SET_NULL) - state = models.ForeignKey(State, on_delete=models.SET(get_guest_state), default=get_guest_state_pk) + state = models.ForeignKey(State, on_delete=models.SET_DEFAULT, default=get_guest_state_pk) - def assign_state(self, commit=True): - state = State.objects.get_for_user(self.user) + def assign_state(self, state=None, commit=True): + if not state: + state = State.objects.get_for_user(self.user) if self.state != state: self.state = state if commit: diff --git a/authentication/signals.py b/authentication/signals.py index 3fba1f35..6339b0b2 100644 --- a/authentication/signals.py +++ b/authentication/signals.py @@ -1,5 +1,5 @@ from __future__ import unicode_literals -from django.db.models.signals import post_save, pre_delete, m2m_changed +from django.db.models.signals import post_save, pre_delete, m2m_changed, pre_save from django.db.models import Q from django.dispatch import receiver, Signal from django.contrib.auth.models import User @@ -52,18 +52,6 @@ def state_saved(sender, instance, *args, **kwargs): trigger_state_check(instance) -@receiver(pre_delete, sender=State) -def state_deleted(sender, instance, *args, **kwargs): - for profile in instance.userprofile_set.all(): - available = State.objects.exclude(pk=instance.pk).available_to_user(profile.user) - if available: - profile.state = available[0] - else: - profile.state = get_guest_state() - profile.save(update_fields=['state']) - state_changed.send(sender=State, user=profile.user, state=profile.state) - - # Is there a smarter way to intercept pre_save with a diff main_character or state? @receiver(post_save, sender=UserProfile) def reassess_on_profile_save(sender, instance, created, *args, **kwargs): @@ -119,12 +107,17 @@ def validate_main_character_token(sender, instance, *args, **kwargs): profile.save() -@receiver(post_save, sender=User) -def assign_state_on_reactivate(sender, instance, *args, **kwargs): - # There's no easy way to trigger an action upon saving from pre_save signal - # If we're saving a user and that user is in the Guest state, assume is_active was just set to True and assign state - if instance.is_active and instance.profile.state == get_guest_state(): - instance.profile.assign_state() +@receiver(pre_save, sender=User) +def assign_state_on_active_change(sender, instance, *args, **kwargs): + # set to guest state if inactive, assign proper state if reactivated + if instance.pk: + old_instance = User.objects.get(pk=instance.pk) + if old_instance.is_active != instance.is_active: + if instance.is_active: + instance.profile.assign_state() + else: + instance.profile.state = get_guest_state() + instance.profile.save(update_fields=['state']) @receiver(post_save, sender=EveCharacter) diff --git a/authentication/tests.py b/authentication/tests.py index 75e8dd19..28c0f4eb 100644 --- a/authentication/tests.py +++ b/authentication/tests.py @@ -239,6 +239,7 @@ class StateTestCase(TestCase): self._refresh_user() self.assertEquals(higher_state, self.user.profile.state) higher_state.delete() + self.assertFalse(State.objects.filter(name='Higher State').count()) self._refresh_user() self.assertEquals(self.member_state, self.user.profile.state) @@ -258,3 +259,14 @@ class StateTestCase(TestCase): higher_state.save() self._refresh_user() self.assertEquals(self.member_state, self.user.profile.state) + + def test_state_assignment_on_active_changed(self): + self.member_state.member_characters.add(self.test_character) + self.user.is_active = False + self.user.save() + self._refresh_user() + self.assertEquals(self.user.profile.state, self.guest_state) + self.user.is_active = True + self.user.save() + self._refresh_user() + self.assertEquals(self.user.profile.state, self.member_state) \ No newline at end of file