#21361 Expand unit tests for ObjectChange and testing asserts

[arthanson]

Fixes: #21361

Adds a new assert assertObjectChangeData and checks this in the standard API/View test cases. It is just extra assert so the overhead is minimal. The new assertions added to every ObjectChange verification block across views.py and api.py check:

• CREATE - prechange_data is None, postchange_data is populated
• UPDATE - both fields are populated, and they differ from each other
• DELETE - prechange_data is populated, postchange_data is None

This applies to single-object operations (create, edit, delete) and bulk operations (bulk import, bulk edit, bulk delete) in both the UI view tests and API tests.

The primary bug class caught: any code path where snapshot() is not called before a modification, which would leave prechange_data = None on an UPDATE or DELETE record.

[arthanson]

@claude /review

[claude]

Claude finished @arthanson's task in 1m 14s —— View job

---

Code Review

• Gather context and read changed files
• Analyze code quality and correctness
• Post review feedback

---

Overview

This PR adds assertions to verify ObjectChange.prechange_data and ObjectChange.postchange_data are correctly populated across all create/update/delete operations in both the UI view tests and API tests. It introduces a new assertObjectChangeData helper on the TestCase base class. The goal is to catch any code path where snapshot() isn't called before a modification, which would leave prechange_data = None on an UPDATE or DELETE.

The changes are correct, useful, and well-scoped. A few observations below:

---

Findings

1. Inconsistent use of the helper method (style)

The new assertObjectChangeData helper is used for single-object tests but bypassed in all bulk operation tests, which use direct assertIsNone/assertIsNotNone calls instead.

Single-object tests — uses helper (api.py:257, api.py:372, api.py:475; views.py:197, views.py:305, views.py:402):

self.assertObjectChangeData(objectchange, prechange_data=None, postchange_data=True)

Bulk operation tests — uses direct assertions (api.py:311-312, api.py:424-426, api.py:517-518; views.py:721-722, views.py:876-878, views.py:975-976):

self.assertIsNone(oc.prechange_data)
self.assertIsNotNone(oc.postchange_data)

For consistency and maintainability, the helper should be used uniformly. This would also mean a single failure message format ("Expected prechange_data to be None") rather than the default assertion messages.
Fix this →

2. Unintuitive parameter convention in assertObjectChangeData (base.py:86-98)

The method signature accepts None to mean "assert null" and any truthy value (callers pass True) to mean "assert populated":

def assertObjectChangeData(self, objectchange, prechange_data, postchange_data):

Passing True as a sentinel for "should be populated" is a bit misleading — it looks like you're asserting equality with the actual data, not just checking for presence. A clearer interface would use explicit boolean flags:

def assertObjectChangeData(self, objectchange, *, prechange_populated: bool, postchange_populated: bool):

Or using Optional[bool] with None meaning "don't check". This is a minor style concern but could confuse future contributors.

3. The assertNotEqual(prechange_data, postchange_data) for UPDATE is not encapsulated in the helper

For update operations, there's an extra assertion outside the helper:

# api.py:373
self.assertObjectChangeData(objectchange, prechange_data=True, postchange_data=True)
self.assertNotEqual(objectchange.prechange_data, objectchange.postchange_data)

This is reasonable as-is, but if the helper were ever extended to support this (e.g., a prechange_differs parameter), it would be cleaner. Not a required change.

4. Potential flakiness: assertNotEqual(prechange_data, postchange_data) for bulk update (api.py:426, views.py:878)

This assertion could fail if bulk_update_data doesn't actually change any snapshotted fields (i.e., if the "update" applies values already stored on the object). This is unlikely in practice if test data is set up correctly, but worth noting as a potential edge case.

---

Summary

The PR is well-targeted and adds meaningful coverage for a real bug class. The logic is correct. The main actionable feedback is using assertObjectChangeData consistently for all bulk operations, which would make the intent uniform and reduce duplication. The parameter convention of None vs. True could also be improved for clarity, but is workable as-is.

[arthanson]

re claude review: fixed #1, #2 - others are being pedantic.

[jeremystretch]

This feels like a half-measure IMO: It's odd that we'd have a dedicated method solely to evaluate pre-/post-change data and not other commonly tested attributes of a change record. Have you considered expanding the method to also facilitate testing e.g. the action and changelog message?

Personally, I'd prefer we either implement a convenience method to test all of the prudent attributes, or skip it and test each individually with the built-in assertions as we do currently for action and message.