Skip to content

Character Verify Bugfix #89

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
freyamade merged 1 commit into from
Jan 8, 2025
Merged

Character Verify Bugfix #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions backend/api/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -22,15 +23,19 @@

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
# - Move the BIS List to the real Character, name it using the Team's name
# - 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
Expand Down Expand Up @@ -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.')
Expand All @@ -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)
Expand Down
154 changes: 153 additions & 1 deletion backend/api/tests/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
13 changes: 8 additions & 5 deletions frontend/src/components/modals/changelog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,17 @@
</div>
<div class="card-content content">
<h2 class="has-text-primary subtitle">{{ version }}</h2>
<div class="divider"><i class="material-icons icon">expand_more</i> Loot Solver Greed Improvements <i class="material-icons icon">expand_more</i></div>
<p>
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.
<div class="divider"><i class="material-icons icon">expand_more</i> Character Verify Bugfix <i class="material-icons icon">expand_more</i></div>
<p>Fixed a bug during the Character Verification process where it couldn't do cleanup, which first occurred yesterday.</p>
<p>For an explanation of the bug and what was done to fix it;
<ul>
<li>For example, currently, if a WHM weapon drops from a fourth fight, and the person who gets it has Greed BIS Lists, but none for WHM, the only way to track the item was to track it manually via the form in the History section.</li>
<li>Now there is a generic "Give Item to Character" button even if they have Greed lists, to give them an item that a Character obtained without it being relevant to their Greed lists.</li>
<li>Previously, unverified Characters could not be used to make BIS Lists or join Teams or anything, so when you verified a Character the system would delete all the unverified versions of the same Character by Lodestone ID.</li>
<li>Now however, unverified Characters can be used for a week before everything gets deleted, but this caused a bug in the cleanup process where if an unverified version had a BIS List it couldn't be deleted.</li>
<li>Also, any proxies using the same Character get merged into the verified Character, consolidating all BIS Lists and Team Membership into the verified Character.</li>
<li class="has-text-primary">The fix I have implemented is to do this consolidation for un-verified Characters belonging to the same User as well, and leaving unverified versions owned by other Users to be deleted after the week deadline.</li>
</ul>
</p>
<p>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!</p>
</div>
</div>
</template>
Expand Down
Loading