Issue with Updating Django Model Object using Form and Custom Update Function

Using Django Forms & APIs
1

Hello everyone,

I am experiencing an issue where passing the form.cleaned_data fields to a service function does not update the model object’s fields. Take this snippet from my code as an example:

def customer_package_edit(request, package_id, *args, **kwargs):
    current_user = request.user
    package = consignee_package_get(package_id=package_id, consignee=current_user.consignee)

    if request.method == "POST":
        form = CustomerPackageUpdateForm(data=request.POST, instance=package)
        if form.is_valid():
            package_update(
                package=package,
                data={
                    "description": form.data.get("description"),
                    "shipper": form.data.get("shipper"),
                    "merchant": form.data.get("merchant"),
                },
            )
            return HttpResponse(
                status=204,
                headers={
                    "HX-Trigger": "package_list_changed",
                },
            )
        else:
            return TemplateResponse(
                request,
                "shipments/packages/customer/package_update_form.html",
                context={
                    "form": form,
                    "package": package,
                },
            )

    else:
        form = CustomerPackageUpdateForm(instance=package)
        return TemplateResponse(
            request,
            "shipments/packages/customer/package_update_form.html",
            context={
                "form": form,
                "package": package,
            },
        )

So I have this package_update function that accepts the package object and a dict of the changes to be made. However, this doesn’t actually update the object.

What am i missing?

2

I don’t see where you’re doing that. I see where you’re directly accessing the data that was posted. (If nothing is translated or transformed in the clean process, they’re generally the same except that the data is going to be all strings while the cleaned_data is the native datatype for that field.)

What does your package_update function look like?

3

Hi Ken,

Thanks for responding. I just realized that during troubleshooting, I changed from form.cleaned_data to calling form.data directly.

This is an excerpt from my package_update function:

def package_update(*, package: Package, data: dict) -> Package:
    non_side_effect_fields = [
        "description",
        "shipper",
        "merchant",
        "tracking_number",
        "weight",
        "declared_value",
        "customs_duties",
        "length",
        "width",
        "height",
        "detained",
        "flagged",
        "flagged_message",
        "commercial",
        "status",
        "consignee",
        "type_code",
    ]

    invoice_required_statuses = (
        PackageStatus.READY,
        PackageStatus.DISPOSED,
        PackageStatus.SOLD,
    )

    validate_package_fields(
        data={
            "flagged": data.get("flagged", None),
            "detained": data.get("detained", None),
            "commercial": data.get("commercial", None),
            "consignee": data.get("consignee", None),
            "declared_value": data.get("declared_value", None),
        }
    )

    # Check if the package's new status allows it to be updated without an invoice
    status = data.get("status")
    original_status = package.status

    if status in [PackageStatus.DELIVERED, PackageStatus.COLLECTED]:
        raise ValidationError("The status of the cannot cannot be manually set to COLLECTED or DELIVERED.")

    if status in invoice_required_statuses and not hasattr(package, "invoice"):
        raise ValidationError("You cannot assign this status without first generating an invoice for this package.")

    package, has_updated = model_update(
        instance=package,
        fields=non_side_effect_fields,
        data=data,
    )

    if has_updated and "status" in data and data["status"] != original_status:
        if package.status_history is None:
            package.status_history = []
        package_status_history_update(package=package, status=status)

        if package.consignee:
            email = package_notification_template_build(package=package)
            ransaction.on_commit(lambda: email_send_task.delay(email.id))

    return package
4

And the model_update method?
(And any other methods that we need to see where you’re actually trying to save this data?)

5

This is the model_update method:

def model_update(
    *, instance: DjangoModelType, fields: List[str], data: Dict[str, Any], auto_updated_at=True
) -> Tuple[DjangoModelType, bool]:
    """
    Generic update service meant to be reused in local update services.

    For example:

    def user_update(*, user: User, data) -> User:
        fields = ['first_name', 'last_name']
        user, has_updated = model_update(instance=user, fields=fields, data=data)

        // Do other actions with the user here

        return user

    Return value: Tuple with the following elements:
        1. The instance we updated.
        2. A boolean value representing whether we performed an update or not.

    Some important notes:

        - Only keys present in `fields` will be taken from `data`.
        - If something is present in `fields` but not present in `data`, we simply skip.
        - There's a strict assertion that all values in `fields` are actual fields in `instance`.
        - `fields` can support m2m fields, which are handled after the update on `instance`.
        - If `auto_updated_at` is True, we'll try bumping `updated_at` with the current timestmap.
    """
    has_updated = False
    m2m_data = {}
    update_fields = []

    model_fields = {field.name: field for field in instance._meta.get_fields()}

    for field in fields:
        # Skip if a field is not present in the actual data
        if field not in data:
            continue

        # If field is not an actual model field, raise an error
        model_field = model_fields.get(field)

        assert model_field is not None, f"{field} is not part of {instance.__class__.__name__} fields."

        # If we have m2m field, handle differently
        if isinstance(model_field, models.ManyToManyField):
            m2m_data[field] = data[field]
            continue

        if getattr(instance, field) != data[field]:
            has_updated = True
            update_fields.append(field)
            setattr(instance, field, data[field])

    # Perform an update only if any of the fields were actually changed
    if has_updated:
        if auto_updated_at:
            # We want to take care of the `updated_at` field,
            # Only if the models has that field
            # And if no value for updated_at has been provided
            if "updated_at" in model_fields and "updated_at" not in update_fields:
                update_fields.append("updated_at")
                instance.updated_at = timezone.now()  # type: ignore

        instance.full_clean()
        # Update only the fields that are meant to be updated.
        # Django docs reference:
        # https://docs.djangoproject.com/en/dev/ref/models/instances/#specifying-which-fields-to-save
        instance.save(update_fields=update_fields)

    for field_name, value in m2m_data.items():
        related_manager = getattr(instance, field_name)
        related_manager.set(value)

        has_updated = True

    return instance, has_updated

The strange thing is the model object gets updated in the unit tests, and if the package_update function gets used outside of the view with the forms.

6 
In [73]: p = Package.objects.first()

In [74]: print(p.tracking_number)
CZH2329808477445

In [75]: p = package_update(package=p, data={"tracking_number": "TBA1234567890"})

In [76]: print(p.tracking_number)
TBA1234567890

In [77]:

This is an example if me running this same package_update function in the command line.

7

That’s useful to know. What does your working unit test for this update look like? (Are your testing the customer_package_edit view or the package_update function? Or both?)

Using that as a starting point, I’d want to see (and compare) what the “data” dict looks like that’s being supplied to package_update under both conditions.

.

8

Side note: The reuse of the package variable name can create some confusion. It can hide effects of what’s going on. (You’re passing it in as a parameter name and reusing it here.) This comment applies to all such cases in your code.

9

Ok, I will update these to be more distinct and prevent shadowing of other instances.

To the previous point you made, I do not have a test for the view as yet. Would a copy of the values in debug mode suffice?

10

I’d probably just add a couple of print statements in the function to print the contents of data along with the keys and data types of the elements of that dict.

11

Ok, great. This is what calling the view function prints to the debug console:

[02/Feb/2024 18:57:40] "GET /customer/packages/ HTTP/1.1" 200 36598
[02/Feb/2024 18:57:40] "GET /customer/packages/? HTTP/1.1" 200 15204
[02/Feb/2024 18:57:41] "GET /customer/packages/edit/7/ HTTP/1.1" 200 1912
[02/Feb/2024 18:57:41] "GET /__debug__/history_sidebar/?store_id=63aa5b65168f4958b4b67069896b5ecf HTTP/1.1" 200 9548
{'description': 'Updated Description', 'shipper': 'USPS (Updated)', 'merchant': 'Amazon (Updated)'}
[02/Feb/2024 18:58:31] "POST /customer/packages/edit/7/ HTTP/1.1" 204 0
[02/Feb/2024 18:58:31] "GET /customer/packages/? HTTP/1.1" 200 15204
[02/Feb/2024 18:58:31] "GET /__debug__/history_sidebar/?store_id=e50efaf5cfb9443a90035c7546b11561 HTTP/1.1" 200 9536
12

I’m not seeing anything obviously wrong.

It’s just a matter at this point then of following the chain. Verify what you have at each step to see if you’ve got the right data at the right points each place along the way.

Using a debugger may be easier than scattering print statements throughout.

13

Ok, thank you Ken. I will continue to troubleshoot.

14

I figured out the bug! It is this block of code:

if getattr(instance, field) != data[field]:
    has_updated = True
    update_fields.append(field)
    setattr(instance, field, data[field])

It seems because I am using aforms.ModelForm, the fields of the package object have been updated (but not commited to the database). So when the aforementioned block of code runs, the value will always be the same (i.e., the condition of the getattr line will never be True), so has_updated will never be set to True, and the subsequent

if has_updated:
    if auto_updated_at:
        # We want to take care of the `updated_at` field,
        # Only if the models has that field
        # And if no value for updated_at has been provided
        if "updated_at" in model_fields and "updated_at" not in update_fields:
            update_fields.append("updated_at")
            instance.updated_at = timezone.now()  # type: ignore

    instance.full_clean()
    # Update only the fields that are meant to be updated.
    # Django docs reference:
    # https://docs.djangoproject.com/en/dev/ref/models/instances/#specifying-which-fields-to-save
    instance.save(update_fields=update_fields)

blocks will never run, therefore, the model object will never be updated. And because I am not calling form.save() in my view, which the form is expecting, then this doesn’t get updated there either.

1 Like
15

Is there a way to prevent this default behavior of ModelForms?

16

Not that I can think of - it would kinda be contrary to what a ModelForm is supposed to do. I think you should be able to check the changed_data attribute on the form to see what was updated.

1 Like