[fix] Fix duplicate check creation in auto_create_check_receiver#769
[fix] Fix duplicate check creation in auto_create_check_receiver#769855princekumar wants to merge 11 commits intoopenwisp:masterfrom
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:
📝 WalkthroughWalkthroughThe signal handler signature for AbstractCheck.auto_create_check_receiver was changed to accept Django signal arguments (sender, instance, created, **kwargs). The transaction_on_commit callback now invokes _auto_check_receiver with sender and instance. _auto_check_receiver resolves the Check model via swapper.load_model, computes the ContentType for the instance, prefetches existing check_type values for the same (content_type, object_id), and during iteration over app_settings.CHECK_CLASSES it skips scheduling auto_create_check.delay(...) when a Check with the matching check_type already exists. Non-functional docstring/comment lines were removed. Sequence DiagramsequenceDiagram
participant Signal as Django Signal
participant Handler as auto_create_check_receiver
participant Tx as transaction_on_commit
participant Receiver as _auto_check_receiver
participant Swapper as swapper.load_model
participant CT as ContentType
participant DB as Database (Check)
participant Celery as Celery Task Queue
Signal->>Handler: Emit signal (sender, instance, created, **kwargs)
Handler->>Tx: Register callback with sender and instance
Tx->>Receiver: Invoke callback (sender, instance) after commit
Receiver->>Swapper: Resolve Check model
Swapper-->>Receiver: Return Check model
Receiver->>CT: Get ContentType for instance
CT-->>Receiver: Return ContentType
Receiver->>DB: Query existing Checks for (content_type, object_id) and prefetch check_type
DB-->>Receiver: Return existing check_type list
loop for each check_class in CHECK_CLASSES
alt matching check_type found
Receiver-->>DB: Skip scheduling (exists)
else no matching check_type
Receiver->>Celery: Schedule auto_create_check.delay(check_class, content_type, object_id)
Celery-->>Receiver: Task queued
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/check/base/models.py`:
- Around line 125-134: The import of Check inside the loop is inefficient and
should be moved outside the loop (e.g., import
openwisp_monitoring.check.models.Check before iterating) and then use that local
reference inside the loop; also change the filter to use check_type (i.e.,
filter(content_type=ct, object_id=object_id, check_type=check_type)) instead of
name so the existence check aligns with CHECK_CLASSES mapping and the
receiver/task consistency.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a522819d-7193-4e4a-b6eb-be8e9a520da8
📒 Files selected for processing (1)
openwisp_monitoring/check/base/models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/base/models.py
🔇 Additional comments (1)
openwisp_monitoring/check/base/models.py (1)
106-112: LGTM!The updated signature correctly matches Django's
post_savesignal convention, and thetransaction_on_commitcallback properly defers the check creation logic until after the transaction commits.
CI Failures: Code Style, Commit Message, and Test ErrorsHello @855princekumar,
For example:
|
855princekumar
changed the title
Moved the import of Check to the top-level for better performance and safety.
Moved import statement for Check to top-level for safety.
QA and Commit Message FailuresHello @855princekumar,
Example:
|
…wisp#649 Moved Check import inside function to avoid circular import issues while keeping it outside the loop for efficiency. Also fixed code style issues to comply with QA checks. Fixes openwisp#649 Moved Check import inside function to avoid circular import issues while keeping it outside the loop for efficiency. Also ensured code style compliance. Fixes openwisp#649
Multiple Test Failures DetectedHello @855princekumar,
|
…wisp#649 Ensure idempotency by validating existing checks using check_type instead of name. Fixes openwisp#649
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/check/base/models.py`:
- Around line 130-135: The loop currently issues a database exists() call per
check class using Check.objects.filter(..., check_type=class_string).exists();
instead, query once for all existing check_type values for the given content
type and object id (use Check.objects.filter(content_type=ct,
object_id=object_id).values_list('check_type', flat=True)), build an in-memory
set, and replace the per-class exists() check with a membership test like "if
class_string in existing_check_types: continue" to avoid N queries.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04393f18-3dbd-4f1c-8f74-3cd5f84527b6
📒 Files selected for processing (1)
openwisp_monitoring/check/base/models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/base/models.py
🔇 Additional comments (2)
openwisp_monitoring/check/base/models.py (2)
107-113: Signal receiver signature and deferred dispatch look correct.This aligns with Django
post_savearguments and keeps scheduling deferred to transaction commit, which is the right pattern here.
123-124: Good handling of circular dependency.Keeping the import local with an explicit rationale makes this non-obvious dependency edge clear and maintainable.
As per coding guidelines, "Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable."
Model class
|
…wisp#649 Ensure idempotent check creation by validating existing checks using check_type. Use swapper.load_model to avoid app registry issues and circular imports. Fixes openwisp#649
|
Updated the implementation to use swapper.load_model instead of direct model import The existing test This fix ensures idempotency by checking existing checks using check_type, |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openwisp_monitoring/check/base/models.py (1)
116-144: 🧹 Nitpick | 🔵 TrivialAvoid per-check-type existence queries in the loop.
The current implementation issues one
exists()query per check class. With a small number of check classes this is acceptable, but you can optimize by fetching existingcheck_types once and performing in-memory set membership checks.def _auto_check_receiver(sender, instance, **kwargs): from swapper import load_model Check = load_model("check", "Check") model = sender.__name__.lower() app_label = sender._meta.app_label object_id = str(instance.pk) ct = ContentType.objects.get_for_model(instance) + # Fetch all existing check_types for this object in a single query + existing_check_types = set( + Check.objects.filter(content_type=ct, object_id=object_id).values_list( + "check_type", flat=True + ) + ) + for class_string, name, auto_create_setting in app_settings.CHECK_CLASSES: if not getattr(app_settings, auto_create_setting): continue - if Check.objects.filter( - content_type=ct, - object_id=object_id, - check_type=class_string, - ).exists(): + if class_string in existing_check_types: continue auto_create_check.delay( model=model, app_label=app_label, object_id=object_id, check_type=class_string, check_name=name, ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/check/base/models.py` around lines 116 - 144, The _auto_check_receiver currently calls Check.objects.filter(...).exists() for every entry in app_settings.CHECK_CLASSES; instead, query existing checks once and use an in-memory set to avoid N queries: load the Check model (Check), compute ct and object_id as now, then fetch existing_types = set(Check.objects.filter(content_type=ct, object_id=object_id).values_list("check_type", flat=True)); then iterate app_settings.CHECK_CLASSES and for each (class_string, name, auto_create_setting) skip if not getattr(app_settings, auto_create_setting) or if class_string in existing_types, otherwise call auto_create_check.delay(...) as before. Ensure you keep the same arguments to auto_create_check.delay and preserve model/app_label determination logic in _auto_check_receiver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openwisp_monitoring/check/base/models.py`:
- Around line 116-144: The _auto_check_receiver currently calls
Check.objects.filter(...).exists() for every entry in
app_settings.CHECK_CLASSES; instead, query existing checks once and use an
in-memory set to avoid N queries: load the Check model (Check), compute ct and
object_id as now, then fetch existing_types =
set(Check.objects.filter(content_type=ct,
object_id=object_id).values_list("check_type", flat=True)); then iterate
app_settings.CHECK_CLASSES and for each (class_string, name,
auto_create_setting) skip if not getattr(app_settings, auto_create_setting) or
if class_string in existing_types, otherwise call auto_create_check.delay(...)
as before. Ensure you keep the same arguments to auto_create_check.delay and
preserve model/app_label determination logic in _auto_check_receiver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb21b8c1-76bf-4c2f-9951-34618b4cc93b
📒 Files selected for processing (1)
openwisp_monitoring/check/base/models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/base/models.py
🔇 Additional comments (1)
openwisp_monitoring/check/base/models.py (1)
106-113: LGTM!The method signature now correctly accepts Django's
post_savesignal arguments (sender,instance,created). Thetransaction_on_commitcallback properly passes these to_auto_check_receiver, ensuring the existence check runs after the transaction commits. This is the correct pattern for signal handlers that need to query the database for the just-saved instance.
Commit Message Format FailureHello @855princekumar, The CI failed because the commit message does not follow the required format. Specifically, the issue number To fix this, please ensure your commit message follows this structure: For example: |
…wisp#649 Ensure idempotent check creation by validating existing checks using check_type. Use swapper.load_model to avoid app registry issues and circular imports. Optimize query by fetching existing check types once.
|
Updated implementation to use swapper.load_model to avoid app registry Optimized duplicate detection by fetching existing check_types once instead The existing test This ensures idempotent behavior aligned with current test coverage. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/check/base/models.py`:
- Around line 117-132: Add concise inline comments explaining why
swapper.load_model("check", "Check") is used (dynamic model loading to avoid
import-time circular dependencies and to support swappable Check models) and why
the code builds existing_check_types via
ContentType.objects.get_for_model(instance) followed by
Check.objects.filter(...).values_list(...) (prefetching existing check_type
values into a set for fast membership checks to ensure idempotent
creation/deduplication of checks). Place the comments near the load_model call
and the existing_check_types assignment and reference the symbols load_model,
Check, ContentType.objects.get_for_model, existing_check_types, sender, and
instance so future readers understand the intent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f763f147-7183-4aa2-9f87-741afea87af1
📒 Files selected for processing (1)
openwisp_monitoring/check/base/models.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/check/base/models.py
🧠 Learnings (3)
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/check/base/models.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/check/base/models.py
🔇 Additional comments (1)
openwisp_monitoring/check/base/models.py (1)
107-113: Good idempotency guard flow in receiver/task dispatch path.The
createdgate plus pre-fetchedexisting_check_typesmembership check before enqueueing looks correct and keeps the signal handler lightweight.Also applies to: 127-139
Commit Message Format FailureHello @855princekumar, The CI failed because the commit message does not follow the required format. Specifically, the issue number To fix this, please update your commit message to include the issue in the body. Correct Format Example: |
Ensure idempotent check creation by validating existing checks using check_type. Use swapper.load_model to support swappable models and avoid app registry issues. Optimize duplicate detection by prefetching existing check types. Fixes openwisp#649
Commit Message Format FailureHello @855princekumar, The commit message failed validation because the issue number ( Fix: For example: |
…wisp#649 Ensure idempotent check creation by validating existing checks using check_type. Use swapper.load_model to support swappable models and avoid app registry issues. Optimize duplicate detection by prefetching existing check types. Fixes openwisp#649
|
please address the code rabbit comments either by: Applying the changes in your next push if you think they're worth addressing |
|
Thanks for the clarification.
I understand now that the review is still pending due to unresolved CodeRabbit comments.
Regarding the regression test: the existing test `test_no_duplicate_check_created` already validates that repeated execution of the auto-create logic does not create duplicate checks. The current fix makes this behavior explicitly idempotent by checking existing `check_type` values before creation.
I also tried adding a separate regression test specifically for this scenario, but it conflicted with the existing test setup and caused unstable test behavior due to overlapping signal execution.
Given that the current test already covers duplicate prevention and all tests are passing, I considered additional coverage redundant. However, I’m happy to add a more explicit regression test if that is preferred.
I will also add concise comments explaining the use of `swapper.load_model` and the deduplication logic as suggested.
|
Explain use of swapper.load_model and prefetch-based deduplication logic. Fixes openwisp#649
|
Added inline comments explaining the dynamic model loading and idempotent deduplication logic as suggested. |
Test Failure in Monitoring NotificationsHello @855princekumar, There is one test failure:
Fix: Review the logic in the |
2 participants
Checklist
Reference to Existing Issue
Closes #649
Description of Changes
Fixed duplicate monitoring check creation by ensuring idempotent behavior.
Previously, checks were created multiple times because validation was missing for each check type.
This resulted in duplicate entries and failing tests.
This fix:
As a result, duplicate-check creation is prevented, and related test failures are resolved.
Screenshot
Test results after applying the fix: