Primary address fix (#5592)

* Improve management of primary address for a company

- Simplify approach (remove "confirm_primary" field)
- Remove @receiver hook
- Move all logic into Address.save() method

* Make address primary if it is the only one defined for a company

* Update frontend table

* Fix saving logic

* Actually fix it this time

* Fix for unit test

* Another test fix
This commit is contained in:
Oliver 2023-09-23 10:14:59 +10:00 committed by GitHub
parent 6cde460567
commit 324d5929b5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 35 additions and 103 deletions

View File

@ -9,7 +9,7 @@ from django.core.exceptions import ValidationError
from django.core.validators import MinValueValidator
from django.db import models
from django.db.models import Q, Sum, UniqueConstraint
from django.db.models.signals import post_delete, post_save, pre_save
from django.db.models.signals import post_delete, post_save
from django.dispatch import receiver
from django.urls import reverse
from django.utils.translation import gettext_lazy as _
@ -281,10 +281,13 @@ class Address(models.Model):
link: External link to additional address information
"""
class Meta:
"""Metaclass defines extra model options"""
verbose_name_plural = "Addresses"
def __init__(self, *args, **kwargs):
"""Custom init function"""
if 'confirm_primary' in kwargs:
self.confirm_primary = kwargs.pop('confirm_primary', None)
super().__init__(*args, **kwargs)
def __str__(self):
@ -304,25 +307,32 @@ class Address(models.Model):
return ", ".join(populated_lines)
class Meta:
"""Metaclass defines extra model options"""
verbose_name_plural = "Addresses"
def save(self, *args, **kwargs):
"""Run checks when saving an address:
- If this address is marked as "primary", ensure that all other addresses for this company are marked as non-primary
"""
others = list(Address.objects.filter(company=self.company).exclude(pk=self.pk).all())
# If this is the *only* address for this company, make it the primary one
if len(others) == 0:
self.primary = True
super().save(*args, **kwargs)
# Once this address is saved, check others
if self.primary:
for addr in others:
if addr.primary:
addr.primary = False
addr.save()
@staticmethod
def get_api_url():
"""Return the API URL associated with the Contcat model"""
return reverse('api-address-list')
def validate_unique(self, exclude=None):
"""Ensure that only one primary address exists per company"""
super().validate_unique(exclude=exclude)
if self.primary:
# Check that no other primary address exists for this company
if Address.objects.filter(company=self.company, primary=True).exclude(pk=self.pk).exists():
raise ValidationError({'primary': _('Company already has a primary address')})
company = models.ForeignKey(Company, related_name='addresses',
on_delete=models.CASCADE,
verbose_name=_('Company'),
@ -382,26 +392,6 @@ class Address(models.Model):
help_text=_('Link to address information (external)'))
@receiver(pre_save, sender=Address)
def check_primary(sender, instance, **kwargs):
"""Removes primary flag from current primary address if the to-be-saved address is marked as primary"""
if instance.company.primary_address is None:
instance.primary = True
# If confirm_primary is not present, this function does not need to do anything
if not hasattr(instance, 'confirm_primary') or \
instance.primary is False or \
instance.company.primary_address is None or \
instance.id == instance.company.primary_address.id:
return
if instance.confirm_primary is True:
adr = Address.objects.get(id=instance.company.primary_address.id)
adr.primary = False
adr.save()
class ManufacturerPart(MetadataMixin, models.Model):
"""Represents a unique part as provided by a Manufacturer Each ManufacturerPart is identified by a MPN (Manufacturer Part Number) Each ManufacturerPart is also linked to a Part object. A Part may be available from multiple manufacturers.

View File

@ -67,11 +67,8 @@ class AddressSerializer(InvenTreeModelSerializer):
'shipping_notes',
'internal_shipping_notes',
'link',
'confirm_primary'
]
confirm_primary = serializers.BooleanField(default=False)
class AddressBriefSerializer(InvenTreeModelSerializer):
"""Serializer for Address Model (limited)"""

View File

@ -4,7 +4,6 @@ import os
from decimal import Decimal
from django.core.exceptions import ValidationError
from django.db import transaction
from django.test import TestCase
from part.models import Part
@ -195,40 +194,27 @@ class AddressTest(TestCase):
def test_primary_constraint(self):
"""Test that there can only be one company-'primary=true' pair"""
c2 = Company.objects.create(name='Test Corp2.', description='We make stuff good')
Address.objects.create(company=self.c, primary=True)
Address.objects.create(company=self.c, primary=False)
self.assertEqual(Address.objects.count(), 2)
# Testing the constraint itself
# Intentionally throwing exceptions breaks unit tests unless performed in an atomic block
with transaction.atomic():
with self.assertRaises(ValidationError):
addr = Address(company=self.c, primary=True, confirm_primary=False)
addr.validate_unique()
self.assertTrue(Address.objects.first().primary)
Address.objects.create(company=c2, primary=True, line1="Hellothere", line2="generalkenobi")
with transaction.atomic():
with self.assertRaises(ValidationError):
addr = Address(company=c2, primary=True, confirm_primary=False)
addr.validate_unique()
# Create another address, specify *this* as primary
Address.objects.create(company=self.c, primary=True)
self.assertEqual(Address.objects.count(), 3)
self.assertFalse(Address.objects.first().primary)
self.assertTrue(Address.objects.last().primary)
def test_first_address_is_primary(self):
"""Test that first address related to company is always set to primary"""
addr = Address.objects.create(company=self.c)
self.assertTrue(addr.primary)
# Create another address, which should error out if primary is not set to False
with self.assertRaises(ValidationError):
addr = Address(company=self.c, primary=True)
addr.validate_unique()
def test_model_str(self):
"""Test value of __str__"""
t = "Test address"

View File

@ -27,6 +27,7 @@
showFormInput,
thumbnailImage,
wrapButtons,
yesNoLabel,
*/
/* exported
@ -798,45 +799,7 @@ function addressFields(options={}) {
company: {
icon: 'fa-building',
},
primary: {
onEdit: function(val, name, field, opts) {
if (val === false) {
hideFormInput("confirm_primary", opts);
$('#id_confirm_primary').prop("checked", false);
clearFormErrors(opts);
enableSubmitButton(opts, true);
} else if (val === true) {
showFormInput("confirm_primary", opts);
if($('#id_confirm_primary').prop("checked") === false) {
handleFormErrors({'confirm_primary': 'WARNING: Setting this address as primary will remove primary flag from other addresses'}, field, {});
enableSubmitButton(opts, false);
}
}
}
},
confirm_primary: {
help_text: "Confirm",
onEdit: function(val, name, field, opts) {
if (val === true) {
clearFormErrors(opts);
enableSubmitButton(opts, true);
} else if (val === false) {
handleFormErrors({'confirm_primary': 'WARNING: Setting this address as primary will remove primary flag from other addresses'}, field, {});
enableSubmitButton(opts, false);
}
},
css: {
display: 'none'
}
},
primary: {},
title: {},
line1: {
icon: 'fa-map'
@ -984,11 +947,7 @@ function loadAddressTable(table, options={}) {
title: '{% trans "Primary" %}',
switchable: false,
formatter: function(value) {
let checked = '';
if (value == true) {
checked = 'checked="checked"';
}
return `<input type="checkbox" ${checked} disabled="disabled" value="${value? 1 : 0}">`;
return yesNoLabel(value);
}
},
{