From faf2470ae183d089209c4681205ec71120fd6599 Mon Sep 17 00:00:00 2001 From: Jack Cushman Date: Fri, 1 Nov 2024 09:39:54 -0400 Subject: [PATCH 1/2] WIP --- perma_web/api/views.py | 8 +--- ...historicallinkuser_bonus_links_and_more.py | 33 +++++++++++++++ perma_web/perma/models.py | 42 ++++++++++++------- 3 files changed, 63 insertions(+), 20 deletions(-) create mode 100644 perma_web/perma/migrations/0052_alter_historicallinkuser_bonus_links_and_more.py diff --git a/perma_web/api/views.py b/perma_web/api/views.py index d25ebaf54..a3e49a0e1 100644 --- a/perma_web/api/views.py +++ b/perma_web/api/views.py @@ -497,10 +497,7 @@ def post(self, request, format=None): if not folder.organization and not folder.sponsored_by: links_remaining, _ , bonus_links = user.get_links_remaining() if bonus_links and not links_remaining: - # (this works because it's part of the same transaction with the select_for_update -- - # we don't have to use the same object) - request.user.bonus_links = bonus_links - 1 - request.user.save(update_fields=['bonus_links']) + user.update_bonus_links(-1) bonus_link = True link = serializer.save(created_by=request.user, bonus_link=bonus_link) @@ -665,8 +662,7 @@ def delete(self, request, guid, format=None): link.save() if link.bonus_link: - link.created_by.bonus_links = (link.created_by.bonus_links or 0) + 1 - link.created_by.save(update_fields=['bonus_links']) + link.created_by.update_bonus_links(1) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/perma_web/perma/migrations/0052_alter_historicallinkuser_bonus_links_and_more.py b/perma_web/perma/migrations/0052_alter_historicallinkuser_bonus_links_and_more.py new file mode 100644 index 000000000..a22befaf1 --- /dev/null +++ b/perma_web/perma/migrations/0052_alter_historicallinkuser_bonus_links_and_more.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.16 on 2024-11-01 13:38 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('perma', '0051_auto_20241030_1732'), + ] + + operations = [ + migrations.AlterField( + model_name='historicallinkuser', + name='bonus_links', + field=models.PositiveIntegerField(default=0), + ), + migrations.AlterField( + model_name='historicalregistrar', + name='bonus_links', + field=models.PositiveIntegerField(default=0), + ), + migrations.AlterField( + model_name='linkuser', + name='bonus_links', + field=models.PositiveIntegerField(default=0), + ), + migrations.AlterField( + model_name='registrar', + name='bonus_links', + field=models.PositiveIntegerField(default=0), + ), + ] diff --git a/perma_web/perma/models.py b/perma_web/perma/models.py index e117a69d8..7034180cc 100755 --- a/perma_web/perma/models.py +++ b/perma_web/perma/models.py @@ -28,7 +28,7 @@ from django.conf import settings from django.core.files.storage import storages from django.db import models, transaction -from django.db.models import Q, Max, Count, Sum, JSONField, F, Exists, OuterRef, When, Case +from django.db.models import Q, Max, Count, Sum, JSONField, F, Exists, OuterRef, When, Case, Greatest from django.db.models.functions import Now, Upper, TruncDate from django.db.models.query import QuerySet from django.contrib.postgres.indexes import GistIndex, GinIndex, OpClass @@ -198,7 +198,7 @@ class Meta: unlimited = models.BooleanField(default=False, help_text="If unlimited, link_limit and related fields are ignored.") link_limit = models.IntegerField(default=settings.DEFAULT_CREATE_LIMIT) link_limit_period = models.CharField(max_length=8, default=settings.DEFAULT_CREATE_LIMIT_PERIOD, choices=(('once','once'),('monthly','monthly'),('annually','annually'))) - bonus_links = models.PositiveIntegerField(blank=True, null=True) + bonus_links = models.PositiveIntegerField(default=0) @cached_property def customer_type(self): @@ -504,8 +504,7 @@ def credit_for_purchased_links(self, purchases): try: with transaction.atomic(): link_quantity = int(purchase["link_quantity"]) - self.bonus_links = (self.bonus_links or 0) + link_quantity - self.save(update_fields=['bonus_links']) + self.update_bonus_links(link_quantity) try: r = requests.post( settings.ACKNOWLEDGE_PURCHASE_URL, @@ -879,11 +878,11 @@ def save(self, *args, **kwargs): # make sure email is still formatted correctly. self.format_email_fields() - with transaction.atomic(): - super().save(*args, **kwargs) - + if not self.root_folder_id: # make sure root folder is created for each user. - if not self.root_folder_id: + with transaction.atomic(): + super().save(*args, **kwargs) + root_folder = Folder.objects.create( name='Personal Links', created_by=self, @@ -894,6 +893,10 @@ def save(self, *args, **kwargs): # so we don't run through our custom logic twice super().save() + else: + # regular save, no transaction + super().save(*args, **kwargs) + def get_full_name(self): """ Use either First Last or first half of email address as user's name. """ return f"{self.first_name} {self.last_name}" if self.first_name or self.last_name else self.email.split('@')[0] @@ -1141,8 +1144,8 @@ def get_links_remaining(self): # Special handling for non-trial users who lack active paid subscriptions: # apply the same rules that are applied to new users if not self.in_trial and not self.nonpaying and self.subscription_status != 'active': - return (self.links_remaining_in_period(settings.DEFAULT_CREATE_LIMIT_PERIOD, settings.DEFAULT_CREATE_LIMIT, unlimited=False), settings.DEFAULT_CREATE_LIMIT_PERIOD, self.bonus_links or 0) - return (self.links_remaining_in_period(self.link_limit_period, self.link_limit), self.link_limit_period, self.bonus_links or 0) + return (self.links_remaining_in_period(settings.DEFAULT_CREATE_LIMIT_PERIOD, settings.DEFAULT_CREATE_LIMIT, unlimited=False), settings.DEFAULT_CREATE_LIMIT_PERIOD, self.bonus_links) + return (self.links_remaining_in_period(self.link_limit_period, self.link_limit), self.link_limit_period, self.bonus_links) def link_creation_allowed(self): links_remaining, _, bonus_links = self.get_links_remaining() @@ -1218,6 +1221,19 @@ def remove_line_from_notes(self, containing): if self.notes: self.notes = re.sub(f"\n*{containing}.*", '', self.notes) + def update_bonus_links(self, count): + # Use Greatest to ensure bonus_links doesn't go below 0 + LinkUser.objects.filter(id=self.id).update( + bonus_links=Greatest(F('bonus_links') + count, 0) + ) + + # mark field as deferred so we don't rely on an outdated value + if not hasattr(self, '_deferred_fields'): + self._deferred_fields = set() + self._deferred_fields.add('bonus_links') + if hasattr(self, 'bonus_links'): + delattr(self, 'bonus_links') + class UserOrganizationAffiliation(models.Model): organization = models.ForeignKey(Organization, on_delete=models.CASCADE) @@ -1493,8 +1509,7 @@ def update_parents_cached_has_children(parent_id=None, previous_parent_id=None): if (parent.organization_id or parent.sponsored_by_id) and (any_link := bonus_links.first()): user = any_link.created_by count = bonus_links.update(bonus_link=False) - user.bonus_links = F('bonus_links') + count - user.save(update_fields=['bonus_links']) + user.update_bonus_links(count) # update the cached paths of this folder and all its descendants update_cached_path(subtree_ids, parent.tree_root_id) @@ -1840,10 +1855,9 @@ def move_to_folder_for_user(self, folder, user): self.organization = folder.organization if self.bonus_link and (folder.organization or folder.sponsored_by): self.bonus_link = False - user.bonus_links = F('bonus_links') + 1 + user.update_bonus_links(1) self.save(update_fields=['organization', 'bonus_link']) - user.save(update_fields=['bonus_links']) def guid_as_path(self): # For a GUID like ABCD-1234, return a path like AB/CD/12. From 5e7d49371be3a00fa063822431f3ae0bdfd98eef Mon Sep 17 00:00:00 2001 From: Jack Cushman Date: Fri, 1 Nov 2024 10:10:33 -0400 Subject: [PATCH 2/2] select_for_update in single query --- perma_web/perma/models.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/perma_web/perma/models.py b/perma_web/perma/models.py index 7034180cc..10da1776a 100755 --- a/perma_web/perma/models.py +++ b/perma_web/perma/models.py @@ -1842,8 +1842,7 @@ def move_to_folder_for_user(self, folder, user): # Don't let anybody move folders around, until this link is # safely inside its destination folder, lest denormalized # ownership-related fields get out of sync - for folder in itertools.chain(self.folders.all(), [folder]): - Folder.objects.select_for_update().get(pk=folder.tree_root_id) + Folder.objects.filter(pk_in=itertools.chain(self.folders.all(), [folder])).select_for_update().values_list('id') # remove this link from any folders it's in for this user self.folders.remove(*self.folders.accessible_to(user))