AAP-68986 Move activity stream registry to setting#951
Conversation
📝 WalkthroughWalkthroughThe pull request refactors the activity stream system from an inheritance-based model (using Changes
Sequence Diagram(s)sequenceDiagram
participant Settings as Settings<br/>(ACTIVITY_STREAM_MODELS)
participant AppReady as ActivitystreamConfig<br/>.ready()
participant Registry as Registered<br/>Models
participant Signals as Django<br/>Signals
participant Model as Tracked<br/>Model
Note over Settings,Model: New Configuration-Based Flow
Settings->>AppReady: ACTIVITY_STREAM_MODELS list
AppReady->>Registry: apps.get_model() per tuple
Registry->>AppReady: Model class reference
AppReady->>Signals: connect(post_save, post_delete, m2m_changed)
Signals->>Model: Subscribe to create/update/delete/m2m
Model->>Signals: Emit signal on modification
Signals->>Registry: Entry created via signal handler
Note over Settings,Model: Old Inheritance-Based Flow (Deprecated)
Model-->>Model: class MyModel(AuditableModel)
Model-->>AppReady: DeprecationWarning emitted on subclass
AppReady->>Signals: Scan & connect for discovered subclasses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AlanCoding
force-pushed
the
auditable_model_cls
branch
from
8e61229 to
2d4dcf3
Compare
AlanCoding
changed the title
AlanCoding
force-pushed
the
auditable_model_cls
branch
from
dc69f56 to
c365b16
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| def bulk_related_objects(self, objs, using='default'): | ||
| # Return an empty queryset so Django's deletion collector | ||
| # never picks up (and deletes) the related entries. | ||
| return self.remote_field.model._base_manager.db_manager(using).none() |
There was a problem hiding this comment.
NonCascadingGenericRelation override is dead code in Django 5.2
High Severity
NonCascadingGenericRelation overrides bulk_related_objects to prevent cascade deletion of audit entries, but this method was removed from Django's deletion collector in Django 1.9. Since this project uses Django 5.2, the override is never invoked — the GenericRelation will still cascade-delete all related Entry objects when the parent is deleted. This silently destroys audit trail data, contradicting the docstring and the documented guarantee that "activity stream records must survive object deletion." Any model using activity_stream_entries() (e.g. Animal) will lose its entire activity stream history on deletion.
Additional Locations (1)
There was a problem hiding this comment.
Not correct. Wrote a test to confirm, will push in another commit. The case here is that you do animal.delete(), after this the activity stream entries are not lost. The distinction is apparently that it is called on for field in model._meta.private_fields, and this is still necessary.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/apps/activitystream.md (1)
34-51: Consider fixing heading hierarchy for consistent document structure.The heading "Adding an
activity_streamrelation to your model" uses####(h4) but follows a section with##(h2), skipping h3. This creates inconsistent document structure.📝 Suggested fix
-#### Adding an `activity_stream` relation to your model +### Adding an `activity_stream` relation to your model🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/apps/activitystream.md` around lines 34 - 51, The heading "Adding an `activity_stream` relation to your model" is at level #### which skips H3 under the preceding H2; change that heading to ### to restore correct hierarchy and document structure so the section introducing activity_stream_entries, the MyModel example and the activity_stream reverse relation are nested properly under the parent H2.ansible_base/activitystream/apps.py (1)
77-104: Consider usingstacklevel=2for accurate deprecation warning attribution.On line 101,
stacklevel=1points to thewarnings.warncall itself. Since this is called from within theready()method during app initialization (not from user code directly subclassing),stacklevel=1may actually be appropriate here as the warning is informational about the model rather than pointing to a specific call site. However, if you want the warning to point to where the model is defined, you'd need a different approach since this runs at app startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/activitystream/apps.py` around lines 77 - 104, The deprecation warning uses warnings.warn(..., stacklevel=1) inside the app initialization, which attributes the warning to the warn call itself; update the warnings.warn invocation in the ready() logic that handles AuditableModel detection (where msg is built and warnings.warn(msg, DeprecationWarning, stacklevel=1) is called) to use stacklevel=2 so the reported stack frame points one level up (closer to where the user's model is defined) and keep the rest of the call (message and DeprecationWarning) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ansible_base/activitystream/apps.py`:
- Around line 77-104: The deprecation warning uses warnings.warn(...,
stacklevel=1) inside the app initialization, which attributes the warning to the
warn call itself; update the warnings.warn invocation in the ready() logic that
handles AuditableModel detection (where msg is built and warnings.warn(msg,
DeprecationWarning, stacklevel=1) is called) to use stacklevel=2 so the reported
stack frame points one level up (closer to where the user's model is defined)
and keep the rest of the call (message and DeprecationWarning) unchanged.
In `@docs/apps/activitystream.md`:
- Around line 34-51: The heading "Adding an `activity_stream` relation to your
model" is at level #### which skips H3 under the preceding H2; change that
heading to ### to restore correct hierarchy and document structure so the
section introducing activity_stream_entries, the MyModel example and the
activity_stream reverse relation are nested properly under the parent H2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7912aa5f-04b5-4419-8eb9-804440263687
📒 Files selected for processing (17)
ansible_base/activitystream/__init__.pyansible_base/activitystream/apps.pyansible_base/activitystream/models/entry.pyansible_base/activitystream/signals.pyansible_base/lib/abstract_models/common.pyansible_base/oauth2_provider/models/access_token.pyansible_base/oauth2_provider/models/application.pyansible_base/oauth2_provider/models/id_token.pyansible_base/oauth2_provider/models/refresh_token.pydocs/apps/activitystream.mdtest_app/defaults.pytest_app/models.pytest_app/tests/activitystream/models/test_entry.pytest_app/tests/activitystream/test_api.pytest_app/tests/activitystream/test_signals.pytest_app/tests/oauth2_provider/test_authentication.pytest_app/tests/oauth2_provider/views/test_application.py
Add back in AuditableModel class Code cleanup that I should not have had to ask Claude to do but I did Do not leave relationship around on external models Fix generic relation
AlanCoding
force-pushed
the
auditable_model_cls
branch
from
c365b16 to
cae836c
Compare
| """Ensure signal handlers fall back to the correct defaults when a model lacks an attribute.""" | ||
| # SecretColor doesn't define these attributes (except what it inherits), | ||
| # so getattr with defaults should match expectations. | ||
| assert getattr(SecretColor, attribute, expected_default) == expected_default |
There was a problem hiding this comment.
Tautological test always passes, verifies nothing meaningful
Low Severity
test_signal_handler_uses_correct_defaults is tautological — it uses expected_default as both the getattr fallback and the assertion target. Since SecretColor (now inheriting plain models.Model) lacks all four tested attributes, getattr(SecretColor, attribute, expected_default) always returns expected_default, making the assertion expected_default == expected_default — trivially true for every parameterized case. The original test meaningfully verified that AuditableModel defined these attributes with the correct values; the replacement verifies nothing about the signal handler defaults it claims to test.
Reviewed by Cursor Bugbot for commit cae836c. Configure here.
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
AlanCoding
changed the title
|
@jeffh-oss @huffmanca In #956 it was requested to expedite that fix and not couple this refactoring in with it. This still does not undo the key thing (subclasses RBAC app models with activitystream app models), because that requires changing the configuration in aap-gateway (https://github.com/ansible-automation-platform/aap-gateway/pull/1297), and then coming back here and removing it. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Reviewed by Cursor Bugbot for commit 91a2fad. Configure here.
| if 'ansible_base.activitystream' in settings.INSTALLED_APPS: | ||
| from ansible_base.activitystream.apps import get_activity_stream_related_url | ||
|
|
||
| response.update(get_activity_stream_related_url(self)) |
There was a problem hiding this comment.
Duplicate activity_stream URL generation for backward-compat models
Low Severity
For models that still inherit from AuditableModel (backward compat path), related_fields() calls self.extra_related_fields(request) on line 253–254 which invokes get_activity_stream_related_url, and then the new block on lines 256–259 calls get_activity_stream_related_url a second time. Both produce the same {'activity_stream': url} dict for registered models, making the AuditableModel.extra_related_fields method entirely redundant for any model that flows through CommonModel.related_fields().
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 91a2fad. Configure here.
|
1 participant
Description
Technical refactor item:
Making activitystream app follow the philosophy of "outside looking in" regarding handling of the ultimate app that is using it. Everything in activitystream uses GenericForeignKey, and as things stand, it is completely consistent with this philosophy.
A concern was expressed that adding a field to
AditableModelwill trigger migrations in apps. That is a question that I don't think should exist, and it won't with these changes.I also find this to be something that will ease its introduction to AWX and other apps. We do not want to add more parent classes to the models there.
Type of Change
Self-Review Checklist
myClaude's codeNote
Medium Risk
Changes how activity-stream signals are connected (now setting-driven with a runtime registry), so misconfiguration could silently stop auditing or change related links. Also adds a custom non-cascading
GenericRelation, affecting delete behavior around audit records.Overview
Shifts activity-stream tracking from
AuditableModelinheritance to an explicitACTIVITY_STREAM_MODELSsetting:ActivitystreamConfig.ready()now connects create/update/delete/m2m signals only for configured models, while still auto-detectingAuditableModelsubclasses with deprecation warnings.Adds public helpers
get_activity_stream_entries()/get_activity_stream_related_url()and automatically injects anactivity_streamrelated link for registered models viaCommonModel.related_fields(). Introducesactivity_stream_entries()(a non-cascadingGenericRelation) so models can optionally expose a reverse relation without risking audit entry deletion when the parent object is deleted.Updates test app models, OAuth2 models, docs, and tests to remove required
AuditableModelinheritance, rely on the new setting-based registration, and validate the new reverse relation and non-cascading delete behavior.Reviewed by Cursor Bugbot for commit 91a2fad. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
ACTIVITY_STREAM_MODELSsetting instead of class inheritanceDeprecations
AuditableModelinheritance approach is now deprecated; migrate toACTIVITY_STREAM_MODELSsettingChanges