Add ActivityEvent model and builder - #5233
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds the ChangesActivityEvent model
Local Compose volume mappings
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Contribution validation failed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/apps/github/models/activity_event.py`:
- Around line 22-31: The ActivityEvent model must allow repeated events for the
same object and activity type. In
backend/src/apps/github/models/activity_event.py lines 22-31, remove or redefine
the unique_activity_event constraint using an event-specific identifier or
occurred_at if uniqueness is still required; in
backend/src/apps/github/migrations/0045_activityevent.py line 32, update the
migration to match the model and remove the existing database constraint.
In `@docker-compose/local/compose.yaml`:
- Around line 124-135: Update cleanup targets in backend/Makefile,
docs/Makefile, and frontend/Makefile to remove the renamed -pulse volume names.
The volume definitions at docker-compose/local/compose.yaml lines 124-135 and
references at lines 26-27, 44-45, 80-81, and 102-103 require no direct changes;
use them as the authoritative names for the Makefile cleanup commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19e3c841-fdbf-4918-9af0-a1eed3d01b2a
📒 Files selected for processing (8)
backend/src/apps/github/admin/__init__.pybackend/src/apps/github/admin/activity_event.pybackend/src/apps/github/migrations/0045_activityevent.pybackend/src/apps/github/models/__init__.pybackend/src/apps/github/models/activity_event.pybackend/src/apps/github/models/enums/__init__.pybackend/src/apps/github/models/enums/activity_event.pydocker-compose/local/compose.yaml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/apps/github/models/activity_event.py (2)
51-70: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPrevent duplicate and redundant database indexes on
ForeignKeyfields.Django automatically creates B-tree indexes for all
ForeignKeyfields.
actorandrepository: TheMeta.indexesblock explicitly defines custom indexes for these fields. This will result in duplicate standalone database indexes being created.content_type: TheMeta.indexesblock defines a composite index on["content_type", "object_id"]. Sincecontent_typeis the leading column in this composite index, it can be used efficiently for queries filtering just bycontent_type. The implicit standalone index oncontent_typeis therefore redundant.To keep your explicit indexes while avoiding database write-penalty and wasted storage space from the implicit ones, explicitly disable
db_indexon these fields in both the model and the migration.
backend/src/apps/github/models/activity_event.py#L51-L70: Adddb_index=Falseto theactor,content_type, andrepositoryForeignKeydeclarations.backend/src/apps/github/migrations/0045_activityevent.py#L50-L74: Update the migration to reflect these constraints so the database tables are created cleanly without redundant indexes.🚀 Proposed fix for the model (backend/src/apps/github/models/activity_event.py)
actor = models.ForeignKey( "github.User", verbose_name="Actor", on_delete=models.SET_NULL, blank=True, null=True, related_name="activity_events", + db_index=False, ) - content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) + content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, db_index=False) object_id = models.PositiveBigIntegerField() occurred_at = models.DateTimeField( verbose_name="Occurred at", help_text="Timestamp when the activity event occurred on GitHub", ) repository = models.ForeignKey( "github.Repository", verbose_name="Repository", on_delete=models.CASCADE, related_name="activity_events", + db_index=False, )🚀 Proposed fix for the migration (backend/src/apps/github/migrations/0045_activityevent.py)
( "actor", models.ForeignKey( blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name="activity_events", to="github.user", verbose_name="Actor", + db_index=False, ), ), ( "content_type", models.ForeignKey( - on_delete=django.db.models.deletion.CASCADE, to="contenttypes.contenttype" + on_delete=django.db.models.deletion.CASCADE, to="contenttypes.contenttype", db_index=False ), ), ( "repository", models.ForeignKey( on_delete=django.db.models.deletion.CASCADE, related_name="activity_events", to="github.repository", verbose_name="Repository", + db_index=False, ), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/apps/github/models/activity_event.py` around lines 51 - 70, Set db_index=False on the actor, content_type, and repository ForeignKey declarations in backend/src/apps/github/models/activity_event.py:51-70, preserving the explicit indexes. Update the corresponding field definitions in backend/src/apps/github/migrations/0045_activityevent.py:50-74 to include the same db_index=False settings; both sites require direct changes.
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle nullable
actorgracefully in string representation.
actoris a nullable foreign key. If an event has no actor,self.actorevaluates toNone, resulting in a literal"None"in the string (e.g.,"issue_closed by None in org/repo").♻️ Proposed refactor
def __str__(self) -> str: """Return human-readable representation.""" - return f"{self.activity_type} by {self.actor} in {self.repository}" + actor_str = f" by {self.actor}" if self.actor else "" + return f"{self.activity_type}{actor_str} in {self.repository}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/apps/github/models/activity_event.py` around lines 73 - 75, Update ActivityEvent.__str__ to handle a null actor without rendering the literal “None”; use an appropriate fallback or omit the actor portion while preserving the existing activity type and repository information.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/src/apps/github/models/activity_event.py`:
- Around line 51-70: Set db_index=False on the actor, content_type, and
repository ForeignKey declarations in
backend/src/apps/github/models/activity_event.py:51-70, preserving the explicit
indexes. Update the corresponding field definitions in
backend/src/apps/github/migrations/0045_activityevent.py:50-74 to include the
same db_index=False settings; both sites require direct changes.
- Around line 73-75: Update ActivityEvent.__str__ to handle a null actor without
rendering the literal “None”; use an appropriate fallback or omit the actor
portion while preserving the existing activity type and repository information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee8269db-5ee8-43d7-b860-628acf72c21b
📒 Files selected for processing (3)
backend/src/apps/github/migrations/0045_activityevent.pybackend/src/apps/github/models/activity_event.pybackend/src/apps/github/models/enums/activity_event.py
💤 Files with no reviewable changes (1)
- backend/src/apps/github/models/enums/activity_event.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/owasp-pulse #5233 +/- ##
=======================================================
- Coverage 98.83% 98.68% -0.16%
=======================================================
Files 538 540 +2
Lines 17123 17209 +86
Branches 2460 2466 +6
=======================================================
+ Hits 16924 16983 +59
- Misses 99 126 +27
Partials 100 100
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
4 similar comments
There was a problem hiding this comment.
This belongs to owasp application, not github.
| from django.db import models | ||
|
|
||
|
|
||
| class ActivityType(models.TextChoices): |
There was a problem hiding this comment.
Any reason for not following existing examples and making it part of ActivityEvent model?
There was a problem hiding this comment.
earlier i was planning to keep ActivityEventBuilder outside the module which is why i introduced a separate enum but as you suggested i will keep ActivityType inside the ActivityEvent model and will move the builder into the module itself
| max_length=32, | ||
| choices=ActivityType.choices, | ||
| ) | ||
| actor = models.ForeignKey( |
There was a problem hiding this comment.
| actor = models.ForeignKey( | |
| github_user = models.ForeignKey( |
| verbose_name="Occurred at", | ||
| help_text="Timestamp when the activity event occurred on GitHub", | ||
| ) | ||
| repository = models.ForeignKey( |
There was a problem hiding this comment.
| repository = models.ForeignKey( | |
| github_repository = models.ForeignKey( |
| volumes: | ||
| - ../../backend:/home/owasp | ||
| - backend-venv:/home/owasp/.venv | ||
| - backend-venv-pulse:/home/owasp/.venv |
There was a problem hiding this comment.
We now use override approach, please update and help testing that.
1 similar comment
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/apps/owasp/admin/activity_event.py`:
- Line 8: Register the ActivityEvent model with Django admin using the existing
ActivityEventAdmin class so Activity Events appear in the admin interface.
Locate the model registration alongside the ActivityEventAdmin definition and
preserve its current configuration.
In `@backend/src/apps/owasp/migrations/0073_activityevent.py`:
- Around line 1-36: Reformat the generated Migration class in
0073_activityevent.py with Ruff’s formatter, preserving the migration
operations, dependencies, indexes, and constraints unchanged; commit the
formatter’s output so pre-commit passes.
- Around line 15-36: Update migration 0073 after creating ActivityEvent to
backfill existing Issue, PullRequest, and Release records into the new table,
using an idempotent RunPython operation and the migration app registry for
historical models. Reuse the same activity types, timestamps, repository/user
relationships, and content-type/object-id mapping expected by ActivityEvent,
while safely avoiding duplicate rows on repeated deployment or migration
execution.
In `@backend/src/apps/owasp/models/activity_event.py`:
- Line 134: Update the exception path in the model conversion method containing
the “Unsupported model type” message: assign the formatted error text to a local
variable before raising TypeError, so the raise statement uses the prebuilt
message and satisfies Ruff EM102 and TRY003.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 524f13bf-1dff-4fe8-94b1-dcaa32f75f13
📒 Files selected for processing (7)
backend/src/apps/github/common.pybackend/src/apps/owasp/admin/__init__.pybackend/src/apps/owasp/admin/activity_event.pybackend/src/apps/owasp/migrations/0073_activityevent.pybackend/src/apps/owasp/models/__init__.pybackend/src/apps/owasp/models/activity_event.pybackend/tests/unit/apps/github/common_test.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose/local/compose.yaml (1)
128-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeclare the renamed volumes consistently.
The service mounts now use
backend-venv-pulse,cache-data-pulse,frontend-next-pulse, andfrontend-node-modules-pulse, but these top-level declarations still use the old names. Docker Compose will treat the new service references as undefined volumes. Rename the corresponding declarations here, and updateCANONICAL_VOLUMESin.github/scripts/docker_compose_check.pyplusTEST_ALLOWED_VOLUMESintools/tests/github/scripts/docker_compose_check_test.py.Proposed fix
- backend-venv: - cache-data: + backend-venv-pulse: + cache-data-pulse: db-data: docs-venv: - frontend-next: - frontend-node-modules: + frontend-next-pulse: + frontend-node-modules-pulse:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose/local/compose.yaml` around lines 128 - 133, Rename the corresponding top-level volume declarations from backend-venv, cache-data, frontend-next, and frontend-node-modules to their -pulse names in the compose configuration, then update CANONICAL_VOLUMES and TEST_ALLOWED_VOLUMES to use the same renamed volume identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docker-compose/local/compose.yaml`:
- Around line 128-133: Rename the corresponding top-level volume declarations
from backend-venv, cache-data, frontend-next, and frontend-node-modules to their
-pulse names in the compose configuration, then update CANONICAL_VOLUMES and
TEST_ALLOWED_VOLUMES to use the same renamed volume identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0400b4f2-d064-49a5-940b-abd11310cba7
📒 Files selected for processing (1)
docker-compose/local/compose.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose/local/compose.override.yaml`:
- Around line 1-13: Remove the committed custom name mappings for backend-venv,
cache-data, db-data, docs-venv, frontend-next, and frontend-node-modules in the
compose override, restoring the file to its documented empty in-tree state. Do
not retain any *-pulse volume names unless the same change also updates the
canonical-volume checker and documented policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35c01dc0-4903-42fb-b0f7-101a4c17a5c0
📒 Files selected for processing (3)
backend/src/apps/owasp/migrations/0073_activityevent.pybackend/src/apps/owasp/models/activity_event.pydocker-compose/local/compose.override.yaml
Signed-off-by: Anurag Yadav <143180737+anurag2787@users.noreply.github.com>
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
cc0f1de to
5244db5
Compare
|
@coderabbitai review it again |
|
✅ Action performedReview finished.
|
|
@cubic-dev-ai review again |
@anurag2787 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/apps/owasp/models/activity_event.py`:
- Around line 120-123: Update ActivityEvent.build_for_release to return no
events when release.published_at is None; only create RELEASE_PUBLISHED using
the actual published_at value. In the release synchronization flow in common.py,
adjust the existing-node early-break logic so previously synced unpublished
releases are revisited after GitHub publishes them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 650884b5-df8e-4214-b720-39d0e099afa7
📒 Files selected for processing (4)
backend/src/apps/owasp/admin/activity_event.pybackend/src/apps/owasp/migrations/0073_activityevent.pybackend/src/apps/owasp/models/activity_event.pydocker-compose/local/compose.override.yaml
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Dismissed because Cubic found issues in a newer review.
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ] | ||
|
|
||
| @staticmethod | ||
| def update_data(obj) -> list["ActivityEvent"]: |
There was a problem hiding this comment.
Could you come up w/ a better naming here instead of obj.
| """Bulk-insert ActivityEvent rows for source objects, skipping duplicates.""" | ||
| events = [event for obj in objects for event in ActivityEvent.update_data(obj)] | ||
| if events: | ||
| ActivityEvent.objects.bulk_create(events, ignore_conflicts=True) |
There was a problem hiding this comment.
Why do you need to ignore conflicts?
There was a problem hiding this comment.
since the sync can run multiple times so some events may already exist in that case we just want to skip them instead of getting an IntegrityError so because of that i added this
| events = [(ActivityEvent.ActivityType.PR_OPENED, pr.created_at, pr.author)] | ||
| if pr.merged_at: | ||
| events.append((ActivityEvent.ActivityType.PR_MERGED, pr.merged_at, pr.author)) | ||
| elif pr.state == "closed" and pr.closed_at: |
There was a problem hiding this comment.
You don't follow the approach consistently -- see IssueState
| """Bulk-insert ActivityEvent rows for source objects, skipping duplicates.""" | ||
| events = [event for obj in objects for event in ActivityEvent.update_data(obj)] | ||
| if events: | ||
| ActivityEvent.objects.bulk_create(events, ignore_conflicts=True) |
There was a problem hiding this comment.
Also why not using ActivityEvent.bulk_save?
There was a problem hiding this comment.
I haven't use ActivityEvent.bulk_save because it doesn't ignore conflict since ActivityEvents are immutable so we only need to insert new ones and skip the ones that already exist
There was a problem hiding this comment.
Why would you have a situation where you need to ignore conflicts, e.g. insert (re-insert) ones that already exist? We wan only new events as a source of data.
There was a problem hiding this comment.
The reason is that the issue and PR sync is based on updated_at and not on whether an ActivityEvent already exists so when an issue or PR gets updated for example when a new comment is added it can get synced again and the same historical event can be generated again and since the ActivityEvents are immutable so because of tha unique constraint bulk_create would fail with an integrity error
Signed-off-by: Anurag Yadav <anuragyadav2787@gmail.com>
023e9b4
|
2 participants
Proposed change
Implements the
ActivityEventmodel and builder for OWASP PulseResolves #5210
Checklist