[chores] Added support for user expiration reminders #608#615
Conversation
- Added celery beat schedule for expiration reminder emails - Added support for openwisp_users expiration tasks - Added configurable reminder email schedule variable Closes #608
| # TODO: Remove before merging | ||
| - "openwisp-radius[openvpn_status] @ https://github.com/openwisp/openwisp-radius/tarball/issues/706-user-expiration" | ||
| - "openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/issues/499-user-expiration" |
There was a problem hiding this comment.
TODO: Remove before merging!
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| - bpython | ||
| # TODO: Remove before merging | ||
| - "openwisp-radius[openvpn_status] @ https://github.com/openwisp/openwisp-radius/tarball/issues/706-user-expiration" | ||
| - "openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/issues/499-user-expiration" |
There was a problem hiding this comment.
CRITICAL: TODO items must be resolved before merging. Temporary tarball dependencies should not be merged to master.
| "max_retries": {{ openwisp2_celery_broker_max_tries }}, | ||
| } | ||
|
|
||
| CELERY_TIMEZONE = TIME_ZONE |
There was a problem hiding this comment.
WARNING: The CELERY_TIMEZONE = TIME_ZONE setting is added here, but TIME_ZONE is defined later in the file (around line 420). While Python handles this at runtime, for clarity consider placing this after TIME_ZONE is defined, or add a comment noting it's a Django setting that will be resolved at runtime.
| "schedule": crontab(**{ {{ cron_password_expiration_email }} }), | ||
| }, | ||
| {% endif %} | ||
| "deactivate_expired_users": { |
There was a problem hiding this comment.
CRITICAL: The deactivate_expired_users task has been moved from openwisp_radius (conditional on openwisp2_radius and openwisp2_radius_periodic_tasks) to openwisp_users (always enabled). This changes behavior for non-RADIUS installations. Ensure this is intentional and that openwisp_users.tasks.deactivate_expired_users exists in all supported openwisp-users versions.
| "schedule": crontab(**{ {{ cron_deactivate_expired_users }} }), | ||
| "args": None, | ||
| }, | ||
| "expiration_reminder_email": { |
There was a problem hiding this comment.
CRITICAL: The expiration_reminder_email task runs unconditionally. Ensure openwisp_users.tasks.expiration_reminder_email exists in all supported openwisp-users versions, or wrap this in a conditional check similar to password_expiration_email above.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address CRITICAL issues before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)
Files Reviewed (3 files)
Issue Alignment Check: This PR implements the changes described in #608 - adding celery beat schedules for expiration reminders and user deactivation tasks. Testing Recommendation: Since this PR modifies celery beat schedules and moves tasks between modules, ensure integration tests verify:
Reviewed by kimi-k2.5 · 147,463 tokens |
📝 WalkthroughWalkthroughThis PR updates the Ansible OpenWISP2 role to support user expiration feature improvements. It adds GitHub tarball dependencies for two feature branches, adjusts Celery cron schedules by adding a new Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
templates/openwisp2/settings.py (1)
248-248:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix import-time name resolution bug for
CELERY_TIMEZONE.Line 248 references
TIME_ZONEbefore it is assigned (Line 420), which can fail at settings import time.Proposed fix
-CELERY_TIMEZONE = TIME_ZONE +CELERY_TIMEZONE = "{{ openwisp2_time_zone }}"#!/bin/bash python - <<'PY' from pathlib import Path p = Path("templates/openwisp2/settings.py") lines = p.read_text().splitlines() celery_tz_line = next(i+1 for i, l in enumerate(lines) if l.strip().startswith("CELERY_TIMEZONE")) time_zone_line = next(i+1 for i, l in enumerate(lines) if l.strip().startswith("TIME_ZONE =")) print(f"CELERY_TIMEZONE line: {celery_tz_line}") print(f"TIME_ZONE line: {time_zone_line}") print("time_zone_defined_before_reference:", time_zone_line < celery_tz_line) PY🤖 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 `@templates/openwisp2/settings.py` at line 248, CELERY_TIMEZONE is referencing TIME_ZONE before it's assigned; update settings so TIME_ZONE is defined prior to using it (move the CELERY_TIMEZONE assignment to after the TIME_ZONE = ... line) or change CELERY_TIMEZONE to read TIME_ZONE safely (e.g. via globals().get('TIME_ZONE', '<default>')) so CELERY_TIMEZONE and TIME_ZONE in templates/openwisp2/settings.py are resolved at import time; edit the CELERY_TIMEZONE assignment or its position and ensure the symbols CELERY_TIMEZONE and TIME_ZONE are consistently ordered or accessed safely.defaults/main.yml (1)
19-21:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove temporary branch tarball dependencies from defaults before merge.
Line 19 explicitly marks this as temporary, and Line 20-Line 21 pin default installs to mutable GitHub issue branches, which is not safe/reproducible for
master.Proposed fix
openwisp2_extra_python_packages: - bpython - # TODO: Remove before merging - - "openwisp-radius[openvpn_status] @ https://github.com/openwisp/openwisp-radius/tarball/issues/706-user-expiration" - - "openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/issues/499-user-expiration"🤖 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 `@defaults/main.yml` around lines 19 - 21, Remove the temporary GitHub tarball pins from defaults/main.yml: delete the entries containing "openwisp-radius[openvpn_status] @ https://github.com/openwisp/openwisp-radius/tarball/issues/706-user-expiration" and "openwisp-users @ https://github.com/openwisp/openwisp-users/tarball/issues/499-user-expiration" and replace them with stable, reproducible pins (either the package name alone or a released version spec, e.g., openwisp-radius==<released_version> and openwisp-users==<released_version>) so defaults no longer reference mutable issue-branch tarballs before merging.
🤖 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.
Duplicate comments:
In `@defaults/main.yml`:
- Around line 19-21: Remove the temporary GitHub tarball pins from
defaults/main.yml: delete the entries containing
"openwisp-radius[openvpn_status] @
https://github.com/openwisp/openwisp-radius/tarball/issues/706-user-expiration"
and "openwisp-users @
https://github.com/openwisp/openwisp-users/tarball/issues/499-user-expiration"
and replace them with stable, reproducible pins (either the package name alone
or a released version spec, e.g., openwisp-radius==<released_version> and
openwisp-users==<released_version>) so defaults no longer reference mutable
issue-branch tarballs before merging.
In `@templates/openwisp2/settings.py`:
- Line 248: CELERY_TIMEZONE is referencing TIME_ZONE before it's assigned;
update settings so TIME_ZONE is defined prior to using it (move the
CELERY_TIMEZONE assignment to after the TIME_ZONE = ... line) or change
CELERY_TIMEZONE to read TIME_ZONE safely (e.g. via globals().get('TIME_ZONE',
'<default>')) so CELERY_TIMEZONE and TIME_ZONE in
templates/openwisp2/settings.py are resolved at import time; edit the
CELERY_TIMEZONE assignment or its position and ensure the symbols
CELERY_TIMEZONE and TIME_ZONE are consistently ordered or accessed safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fb0316e-f344-4d78-bbfd-81729ba4734b
📒 Files selected for processing (3)
defaults/main.ymldocs/user/role-variables.rsttemplates/openwisp2/settings.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). (4)
- GitHub Check: Build debian12
- GitHub Check: Build debian13
- GitHub Check: Build ubuntu2404
- GitHub Check: Build ubuntu2204
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{md,rst,txt}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt}: Changes: If the change affects behavior that is already documented, the documentation must be updated to reflect the new behavior
Features: Documentation must mention the new feature; for heavily UI-related features, a new section or page is appropriate
Files:
docs/user/role-variables.rst
**/*.py
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
templates/openwisp2/settings.py
🔇 Additional comments (3)
defaults/main.yml (1)
217-218: LGTM!docs/user/role-variables.rst (1)
450-451: LGTM!templates/openwisp2/settings.py (1)
256-264: LGTM!
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
1 participant
Checklist
Reference to Existing Issue
Closes #608
Description of Changes
Blockers