Skip to content

Conversation

@ssmadhavan006
Copy link

@ssmadhavan006 ssmadhavan006 commented Dec 8, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Closes #1070.

Description of Changes

This PR adds missing automated tests to ensure device configuration status updates correctly when organization configuration variables (context) change. The existing implementation already handled this logic correctly (via checksum comparison) but there was no automated test coverage verifying the behavior.

What’s Added

A new test class TestOrganizationConfigUpdates in openwisp_controller/config/tests/test_device.py. It includes three scenarios:

  1. Affected device becomes modified

    • Template uses {{ ssid }}
    • Changing org context updates config status
  2. Unaffected device remains applied

    • Template has no variables
    • Context change does not affect checksum
  3. No status change when checksum does not change

    • Unused variable changes
    • Config remains unchanged

Notes:

  • No production code was modified
  • Tests run synchronously (CELERY_TASK_ALWAYS_EAGER=True)
  • Ensures future regressions are prevented

Screenshot

Creating a Organization
Screenshot 2025-12-08 232106

Creating a Template-1
Screenshot 2025-12-08 230936
Screenshot 2025-12-08 230943

Creating a Template-2
Screenshot 2025-12-08 231012
Screenshot 2025-12-08 231022

Device-1 Configuration
Screenshot 2025-12-08 231631

Device-2 Configuration
Screenshot 2025-12-08 231644

Before Change
Screenshot 2025-12-08 231402

Change Org Variable
Screenshot 2025-12-08 232038

Device Status Change
Screenshot 2025-12-08 231518

@ssmadhavan006 ssmadhavan006 changed the title Fix device config status for organization variable change #1070 Fix device config status for organization variable change Dec 8, 2025
@DragnEmperor DragnEmperor changed the title Fix device config status for organization variable change Tests for device config updates on org variable changes Dec 10, 2025
@ssmadhavan006 ssmadhavan006 force-pushed the fix/org-var-status-#1070 branch from bcc1026 to e7f3bcb Compare December 10, 2025 19:27
@nemesifier
Copy link
Member

@coderabbitai review

Sorry @ssmadhavan006 we've been flooded with PRs lately and it's taking time to respond to everyone.

Instead of sharing screenshots in this case a video recording demonstrating the solution would have been better.

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

@nemesifier: I'll review the changes in this PR. This looks like a focused test-only PR adding coverage for device configuration status updates when organization variables change.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

This pull request adds a new test class TestOrganizationConfigUpdates with three test methods to validate behavior when organization-level configuration variables are modified. The tests verify that: (1) changing organization variables causes related device configs to transition to "modified" status, (2) unrelated devices remain unaffected when organization variables change, and (3) config status remains "applied" when variable changes do not affect the resulting checksum.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding tests for device config updates when organization variables change.
Description check ✅ Passed The description is comprehensive with linked issue, detailed changes explanation, test scenarios, and supporting screenshots. All required sections are well-documented.
Linked Issues check ✅ Passed The PR directly addresses issue #1070 by adding test coverage for the scenario where changing organization variables should update device config status to 'modified'.
Out of Scope Changes check ✅ Passed All changes are scoped to test code only in test_device.py; no production code modifications, staying within the objective of adding test coverage for issue #1070.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ee8c25 and e7f3bcb.

📒 Files selected for processing (1)
  • openwisp_controller/config/tests/test_device.py
🔇 Additional comments (4)
openwisp_controller/config/tests/test_device.py (4)

689-693: LGTM!

The test class structure is appropriate. Using TransactionTestCase is correct for tests involving Celery tasks that execute synchronously with CELERY_TASK_ALWAYS_EAGER=True, and the mixins provide the necessary helper methods.


694-728: LGTM!

This test correctly validates the core scenario from issue #1070. The flow properly:

  1. Sets up a template using {{ ssid }} variable
  2. Establishes "applied" status baseline
  3. Modifies the organization context
  4. Verifies the config transitions to "modified"

The test directly exercises the OrganizationConfigSettings.save() → bulk invalidation → status update pipeline.


730-785: LGTM!

Good test coverage for verifying selective status updates. The test correctly demonstrates that only devices whose rendered configuration is affected by the variable change (device_a with {{ ssid }}) transition to "modified", while devices with static configurations (device_b) remain "applied".


787-813: LGTM!

This is an important edge case test. It validates that the checksum-based optimization works correctly — when organization context variables change but those variables aren't referenced in any template, the rendered configuration checksum remains the same, and the device status correctly stays "applied". This prevents unnecessary configuration re-pushes.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I only see a test here, but no solution, is this on purpose? Waiting for the CI build, I assume you worked on replicating the bug in the tests and I expect this test to fail, let's see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Device config status does not update to "modified" when organization variable is changed

3 participants