diff --git a/backend/api/tasks.py b/backend/api/tasks.py index 69dff99..f701cbd 100644 --- a/backend/api/tasks.py +++ b/backend/api/tasks.py @@ -11,6 +11,7 @@ from celery.utils.log import get_task_logger from channels.layers import get_channel_layer from django.core.management import call_command +from django.db.models import Q from django.utils import timezone # local from . import notifier @@ -22,7 +23,10 @@ def assimilate_proxies(real_char: Character): # Find all Proxy characters that have the same lodestone ID as this one - proxies = Character.objects.filter(user__isnull=True, lodestone_id=real_char.lodestone_id) + proxies = Character.objects.filter( + Q(user__isnull=True) | Q(user_id=real_char.user_id), + lodestone_id=real_char.lodestone_id, + ) # For each Character (which should only ever be in one team each); # - Notify the Team Leader that the claim has happened @@ -30,7 +34,8 @@ def assimilate_proxies(real_char: Character): # - Update the TeamMember object to point to this character for char in proxies: for tm in char.teammember_set.all(): - notifier.team_proxy_claim(tm) + if char.user is None: + notifier.team_proxy_claim(tm) bis = tm.bis_list bis.owner = real_char @@ -63,7 +68,7 @@ def verify_character(pk: int): if err is not None: notifier.verify_fail(obj, err) - logger.info(f'Character #{pk} could not be verified. Exiting.') + logger.info(f'Character #{pk} could not be verified. Exiting. ({err})') return logger.info(f'Character #{pk} verified. Updating DB.') @@ -75,9 +80,14 @@ def verify_character(pk: int): assimilate_proxies(obj) # Next delete all unverified instances of the character (this includes proxies) - logger.debug(f'Deleting unverified instances of Character #{obj.lodestone_id} (#{pk}).') - objs = Character.objects.filter(verified=False, lodestone_id=obj.lodestone_id).exclude(pk=pk) - logger.debug(f'Found {objs.count()} instances of Character #{obj.lodestone_id} to delete.') + logger.info(f'Deleting unverified instances of Character #{obj.lodestone_id} (#{pk}) owned by {obj.user_id}.') + objs = Character.objects.filter( + Q(user__isnull=True) | Q(user_id=obj.user_id), + verified=False, + lodestone_id=obj.lodestone_id, + ).exclude(pk=pk) + ids_to_delete = [o.pk for o in objs] + logger.info(f'Found {objs.count()} instances of Character #{obj.lodestone_id} to delete.\n{ids_to_delete}') objs.delete() # Then we're done! notifier.verify_success(obj) diff --git a/backend/api/tests/test_tasks.py b/backend/api/tests/test_tasks.py index b2803a8..10493b3 100644 --- a/backend/api/tests/test_tasks.py +++ b/backend/api/tests/test_tasks.py @@ -8,6 +8,7 @@ from django.core.management import call_command from django.utils import timezone # local +from api.lodestone_scraper import LodestoneScraper from api.models import BISList, Character, Gear, Job, Notification, Team, Tier from api.tasks import cleanup, verify_character, remind_users_to_verify from .test_base import SavageAimTestCase @@ -78,6 +79,10 @@ class TasksTestSuite(SavageAimTestCase): Mock requests to return pre-determined html bodies """ + def setUp(self): + # Call LodestoneScraper.get_instance here so it's unaffected by mocks + LodestoneScraper.get_instance() + def tearDown(self): Notification.objects.all().delete() Team.objects.all().delete() @@ -225,13 +230,25 @@ def test_verify_character(self, mocked_get): verified=False, token=Character.generate_token(), ) + third_version = Character.objects.create( + avatar_url='https://img.savageaim.com/abcde', + lodestone_id=1234567890, + user=char.user, + name='Char 3', + world='Lich', + verified=False, + token=Character.generate_token(), + ) verify_character(char.pk) char.refresh_from_db() self.assertTrue(char.verified) self.assertEqual(Notification.objects.count(), 1) with self.assertRaises(Character.DoesNotExist): - Character.objects.get(pk=other_version.pk) + Character.objects.get(pk=third_version.pk) + + # Ensure that the one owned by a different user is still present to be deleted + Character.objects.get(pk=other_version.pk) # Check for Notification notif = Notification.objects.first() @@ -381,3 +398,138 @@ def test_verify_reminder(self): remind_users_to_verify() self.assertEqual(Notification.objects.count(), 1) self.assertEqual(Notification.objects.first().pk, notif.pk) + + @patch('requests.get', side_effect=get_desktop_response) + @patch('api.notifier.team_proxy_claim') + def test_verify_bug_where_other_unverified_characters_are_used_for_something(self, *args): + """ + Test Plan: + - Replicate the bug from Jan 7th + - Cannot delete some instances of model 'Character' because they are referenced through protected foreign keys: 'BISList.owner'." + - Appears to happen as a result of the other unverified characters being owned and being in use as opposed to being proxies + """ + call_command('seed', stdout=StringIO()) + lodestone_id = '1234567890' + # in the bug, the proxy was created before the character being verified + other_owned_character = Character.objects.create( + avatar_url='https://img.savageaim.com/abcde', + lodestone_id=lodestone_id, + user=self._get_user(), + name='Char 1', + world='Lich', + verified=False, + token=Character.generate_token(), + ) + # Create a Team first + team = Team.objects.create( + invite_code=Team.generate_invite_code(), + name='Les Jambons', + tier=Tier.objects.get(max_item_level=605), + ) + team2 = Team.objects.create( + invite_code=Team.generate_invite_code(), + name='Les Jambons', + tier=Tier.objects.get(max_item_level=605), + ) + + # Create the Character that we will be verifying + char = Character.objects.create( + avatar_url='https://img.savageaim.com/abcde', + lodestone_id=lodestone_id, + user=other_owned_character.user, + name='Char 2', + verified=False, + world='Lich', + ) + proxy = Character.objects.create( + avatar_url='https://img.savageaim.com/abcde', + lodestone_id=lodestone_id, + user=None, + name='Char 3', + world='Lich', + verified=False, + token=Character.generate_token(), + ) + raid_weapon = Gear.objects.get(item_level=605, name='Asphodelos') + raid_gear = Gear.objects.get(item_level=600, has_weapon=False) + tome_gear = Gear.objects.get(item_level=600, has_weapon=True) + crafted = Gear.objects.get(name='Classical') + bis = BISList.objects.create( + bis_body=raid_gear, + bis_bracelet=raid_gear, + bis_earrings=raid_gear, + bis_feet=raid_gear, + bis_hands=tome_gear, + bis_head=tome_gear, + bis_left_ring=tome_gear, + bis_legs=tome_gear, + bis_mainhand=raid_weapon, + bis_necklace=tome_gear, + bis_offhand=raid_weapon, + bis_right_ring=raid_gear, + current_body=crafted, + current_bracelet=crafted, + current_earrings=crafted, + current_feet=crafted, + current_hands=crafted, + current_head=crafted, + current_left_ring=crafted, + current_legs=crafted, + current_mainhand=crafted, + current_necklace=crafted, + current_offhand=crafted, + current_right_ring=crafted, + job_id='SGE', + owner=other_owned_character, + ) + bis2 = BISList.objects.create( + bis_body=raid_gear, + bis_bracelet=raid_gear, + bis_earrings=raid_gear, + bis_feet=raid_gear, + bis_hands=tome_gear, + bis_head=tome_gear, + bis_left_ring=tome_gear, + bis_legs=tome_gear, + bis_mainhand=raid_weapon, + bis_necklace=tome_gear, + bis_offhand=raid_weapon, + bis_right_ring=raid_gear, + current_body=crafted, + current_bracelet=crafted, + current_earrings=crafted, + current_feet=crafted, + current_hands=crafted, + current_head=crafted, + current_left_ring=crafted, + current_legs=crafted, + current_mainhand=crafted, + current_necklace=crafted, + current_offhand=crafted, + current_right_ring=crafted, + job_id='PLD', + owner=proxy, + ) + + # Put the proxy character on the team + tm = team.members.create(character=other_owned_character, bis_list=bis, lead=True) + tm2 = team2.members.create(character=proxy, bis_list=bis2, lead=False) + + # Attempt to verify the proxy character, which for some reason is causing a protected error + self.assertEqual(Character.objects.count(), 3) + verify_character(char.pk) + self.assertEqual(Character.objects.count(), 1) + with self.assertRaises(Character.DoesNotExist): + Character.objects.get(pk=proxy.pk) + with self.assertRaises(Character.DoesNotExist): + Character.objects.get(pk=other_owned_character.pk) + + tm.refresh_from_db() + self.assertEqual(tm.character_id, char.pk) + tm2.refresh_from_db() + self.assertEqual(tm2.character_id, char.pk) + + bis.refresh_from_db() + self.assertEqual(bis.owner_id, char.id) + bis2.refresh_from_db() + self.assertEqual(bis2.owner_id, char.id) diff --git a/frontend/src/components/modals/changelog.vue b/frontend/src/components/modals/changelog.vue index e40abc0..b14f40a 100644 --- a/frontend/src/components/modals/changelog.vue +++ b/frontend/src/components/modals/changelog.vue @@ -12,14 +12,17 @@

{{ version }}

-
expand_more Loot Solver Greed Improvements expand_more
-

- Made it so that when people have Greed BIS Lists, you can still generically assign items to them without updating one of their BIS Lists if the item is not relevant. +

expand_more Character Verify Bugfix expand_more
+

Fixed a bug during the Character Verification process where it couldn't do cleanup, which first occurred yesterday.

+

For an explanation of the bug and what was done to fix it;

+

If anyone would like to share feedback / give ideas on this matter, please let me know on Github or in the Discord. Hopefully this is an acceptable solution!