-
-
Notifications
You must be signed in to change notification settings - Fork 280
Improved Snapshot Generation #2533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary by CodeRabbit
WalkthroughIntroduces a specialized Django manager for filtering active releases and updates snapshot processing to use active managers for chapters, projects, and releases with explicit creation-time boundaries instead of generic is_active filtering. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ 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 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/github/models/managers/release.py (1)
6-23: LGTM! Clean implementation of active release filtering.The manager correctly filters out drafts, pre-releases, and empty repositories. The use of
select_related("repository")is a good performance optimization since we're filtering on repository fields.Consider enhancing the docstring to be more explicit about the filtering criteria:
class ActiveReleaseManager(models.Manager): - """Active releases manager.""" + """Manager for active releases. + + Returns releases that are published (not drafts), stable (not pre-releases), + and have a non-empty repository. + """ def get_queryset(self): - """Get queryset of active releases. - - Filters out draft and pre-releases, and ensures repository exists. - """ + """Get queryset of active releases. + + Returns: + QuerySet: Releases filtered by: + - is_draft=False + - is_pre_release=False + - repository__is_empty=False (also excludes null repositories) + """Note: The
repository__is_empty=Falsefilter will also exclude releases where repository is NULL. If releases without repositories should be included, you'll need to adjust the filter logic.backend/apps/owasp/management/commands/owasp_process_snapshots.py (1)
69-85: LGTM! Excellent consistency improvement using active entity managers.The changes successfully achieve the PR objectives by:
- Replacing generic
is_activefiltering with specialized active managers- Using consistent patterns across all three entity types (chapters, projects, releases)
- Maintaining proper time boundaries with
created_at__gteandcreated_at__lteThe approach ensures that:
- Chapter.active_chapters filters active chapters
- Project.active_projects filters active projects
- Release.active_releases filters non-draft, non-prerelease releases from non-empty repositories
All three now apply time boundaries uniformly, eliminating the previous inconsistencies mentioned in issue #1988.
Optional consideration: Releases are filtered by
created_at, which is consistent with chapters and projects. However, releases semantically have apublished_attimestamp that might be more meaningful for snapshot purposes. The current approach (usingcreated_at) is defensible and maintains consistency, but if you want to capture releases by their publication date instead, you'd need to adjust the filter and handle potential nullpublished_atvalues.Given that the goal is consistency and
created_atis guaranteed to be non-null, the current implementation is solid.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/github/models/managers/release.py(1 hunks)backend/apps/github/models/release.py(1 hunks)backend/apps/owasp/management/commands/owasp_process_snapshots.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1967
File: backend/apps/owasp/api/internal/filters/project_health_metrics.py:24-27
Timestamp: 2025-08-04T15:55:08.017Z
Learning: In the OWASP Nest project, the `is_active` filter in `ProjectHealthMetricsFilter` is intentionally designed as a workaround to eliminate inactive projects from the dashboard. It only filters FOR active projects when `value=True`, and returns an empty Q() when `value=False` to avoid showing inactive projects. This is not meant to be a general boolean filter but a specific solution to exclude inactive projects from the project health metrics dashboard.
🧬 Code graph analysis (2)
backend/apps/github/models/release.py (4)
backend/apps/github/models/managers/release.py (1)
ActiveReleaseManager(6-23)backend/apps/github/models/mixins/release.py (1)
ReleaseIndexMixin(8-70)backend/apps/common/models.py (2)
BulkSaveModel(10-34)TimestampedModel(37-46)backend/apps/github/models/common.py (1)
NodeModel(76-90)
backend/apps/owasp/management/commands/owasp_process_snapshots.py (5)
backend/apps/owasp/api/internal/nodes/snapshot.py (4)
new_chapters(35-37)new_issues(40-42)new_projects(45-47)new_releases(50-52)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)backend/apps/owasp/api/internal/queries/snapshot.py (1)
snapshot(14-22)backend/apps/owasp/models/project.py (1)
Project(37-384)backend/apps/github/models/release.py (1)
Release(11-139)
🔇 Additional comments (3)
backend/apps/github/models/release.py (2)
7-7: LGTM! Proper import for the new manager.The import is correctly placed with other model-related imports.
14-16: LGTM! Manager declarations follow Django best practices.Explicitly defining both the default
objectsmanager and the specializedactive_releasesmanager is the correct approach. This matches the pattern used by Chapter and Project models, ensuring consistency across the codebase.The order matters in Django—the first manager declared becomes the default manager, so keeping
objectsfirst maintains expected ORM behavior.backend/apps/owasp/management/commands/owasp_process_snapshots.py (1)
73-90: LGTM! Correct to keep Issues and Users using the generic helper.Issue and User models don't have specialized active managers (and likely don't need them based on their domain semantics), so continuing to use
get_new_itemsfor these entities is appropriate. This selective application of active managers demonstrates good judgment—only entities with meaningful "active" states get the specialized filtering.
|
@arkid15r for new_issue if we use OpenIssueManager that is predefined but it only returns issues from the last 90 days so So instead, can we directly use this ? |



Resolves #1988
Description
This PR improves how we generate snapshots by making the handling of active entities more consistent. Earlier, different parts of the code used different ways to filter chapters by is_active, and some didn’t properly handle cases where repositories were empty
Now, we’ve standardized the process using the manager patterns active_chapters active_projects active_releases
Checklist
make check-testlocally; all checks and tests passed.