Skip to content

Conversation

@jeremystretch
Copy link
Member

@jeremystretch jeremystretch commented Jul 17, 2025

Closes: #19713

  • Add a message field on the ObjectChange model
  • Add a changelog_message field to the object edit & delete forms
  • Capture the changelog_message value on the resulting ObjectChange record when an object is saved or deleted via the web UI
  • Expand the search filter for ObjectChange to match against the message value
  • Expand all model forms & serializers to support changelog messages as appropriate
  • Add the changelog_message field to the necessary view templates
  • Define fieldsets on VirtualChassisCreateForm & discontinue use of the custom HTML template (to ensure consistent display of the changelog message field form)
  • Extend object & bulk views to support changelog messages
  • Extend the view and REST API test suite to validate the changelog message functionality for all create/edit/delete operations
  • Document the new changelog message functionality

@jeremystretch jeremystretch force-pushed the 19713-objectchange-message branch from 27d026d to 1b11895 Compare July 18, 2025 13:43

cls.bulk_update_data = {
'policy': vlan_translation_policies[1].pk,
'policy': vlan_translation_policies[2].pk,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, the test was not producing the expected number of ObjectChange records, which failed a check in the updated test suite.

@jeremystretch
Copy link
Member Author

The test failure appears to be unrelated to these changes. Extension of the test functionality has uncovered an unrelated bug; see #19956.

@jeremystretch jeremystretch marked this pull request as ready for review July 25, 2025 19:49
@jeremystretch jeremystretch requested a review from jnovinger July 25, 2025 19:50
@jeremystretch
Copy link
Member Author

The CI failure should be addressed by the fix in PR #19957.


def test_bulk_delete_objects_with_permission(self):
pk_list = self._get_queryset().values_list('pk', flat=True)
pk_list = list(self._get_queryset().values_list('pk', flat=True))[:3]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limit this test to three objects (as above) to avoid failing on cascading deletions for some test data.

Copy link
Member

@jnovinger jnovinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change requested and one question.

Q(user_name__icontains=value) |
Q(object_repr__icontains=value)
Q(object_repr__icontains=value) |
Q(message__icontains=value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to make this a default queried field, should we add an index to the field? (He says not having looked at other similar fields yet ...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not as part of this change. message here is analogous to description on most other models. I'm not sure if it makes sense to add an index for those fields, but it does it IMO it should be undertaken as part of an application-wide audit. For now I think it makes sense to continue the current convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, got ahead of myself here. Just got done looking around and realized the same thing about it being a bigger, separate chunk of work.

@jnovinger jnovinger merged commit 24a0e19 into feature Jul 29, 2025
3 checks passed
@jnovinger jnovinger deleted the 19713-objectchange-message branch July 29, 2025 14:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants