[change] Make Notification.type non-nullable #481#484
Conversation
📝 WalkthroughWalkthroughThis PR makes Notification.type required: the model field is made non-nullable and migrations add Django-version-aware choices. Model methods always use notification configuration for link resolution, message rendering, and email subject generation. The notify_handler now requires a type and preference lookups were refactored to always filter by type. Utility logic and tests/documentation were updated to pass explicit Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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 |
BHARATH0153
force-pushed
the
fix/make-type-non-nullable
branch
from
8019d68 to
e6e55f5
Compare
![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
`@openwisp_notifications/migrations/0013_make_notification_type_nonnullable.py`:
- Around line 14-26: The migration currently alters the Notification.type field
to non-nullable without backfilling existing NULLs; add a preceding data
migration (migrations.RunPython) in the same migration file that updates
existing Notification rows where type IS NULL to a valid value (e.g., the first
value from NOTIFICATION_CHOICES or result of get_notification_choices) before
the migrations.AlterField runs; implement a forward function that queries the
Notification model and sets a safe default for NULL types and a noop reverse
function (or one that sets them back to NULL if desired), then ensure the
migrations.AlterField targeting model_name="notification" name="type" runs after
this RunPython step.
In
`@tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.py`:
- Around line 14-26: The migration applies migrations.AlterField on
model_name="notification" name="type" to make it non-nullable but lacks the
pre-AlterField backfill used in the main migration; add a RunPython data
migration before this migrations.AlterField that updates existing rows with NULL
in the "type" column to a safe default (matching the logic used where
NOTIFICATION_CHOICES/get_notification_choices are defined) so the AlterField
won't fail, then keep the existing AlterField using NOTIFICATION_CHOICES (or
get_notification_choices for Django>=5) as-is.
🪄 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: d3588129-ddcf-4c92-b16d-7068f3ee3409
📒 Files selected for processing (8)
docs/developer/sending-notifications.rstopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.pytests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt,adoc}: Changes: If the change affects documented behavior, update the documentation to reflect the new behavior
Features: Documentation must mention the new feature; if the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/developer/sending-notifications.rst
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
General: Flag outdated or incorrect documentation, comments, or docstrings
Files:
docs/developer/sending-notifications.rsttests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/utils.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/base/models.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}: General: Flag potential security vulnerabilities in code
General: Flag obvious performance regressions such as heavy loops, repeated I/O, or unoptimized queries
General: Avoid unnecessary comments or docstrings for code that is already clear
General: Code formatting must be compact and readable; do not add excessive blank lines, especially inside function or method bodies
General: Flag unused or redundant code
General: Ensure variables, functions, classes, and files have descriptive and consistent names
General: New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/utils.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/base/models.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,sh,bash}
📄 CodeRabbit inference engine (Custom checks)
General: 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:
tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/utils.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/base/models.py
**/*.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
**/*.py: Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/utils.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/base/models.py
openwisp_notifications/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings as translatable with Django i18n helpers, typically
gettext_lazyimported as_
Files:
openwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/utils.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/base/models.py
openwisp_notifications/{serializers,views,handlers,tasks}.py
📄 CodeRabbit inference engine (AGENTS.md)
Preserve validation and permission checks when changing serializers, views, handlers, or tasks that include notification payloads with related objects and URLs
Files:
openwisp_notifications/handlers.py
**/models.py
📄 CodeRabbit inference engine (AGENTS.md)
Preserve swappable model support and integration with
openwisp-usersorganizations and memberships in Django models
Files:
openwisp_notifications/base/models.py
🧠 Learnings (1)
📚 Learning: 2026-05-07T16:17:11.806Z
Learnt from: pandafy
Repo: openwisp/openwisp-notifications PR: 450
File: openwisp_notifications/tests/test_selenium.py:276-285
Timestamp: 2026-05-07T16:17:11.806Z
Learning: In OpenWISP projects’ test code (e.g., openwisp-notifications), Celery is configured project-wide to run tasks eagerly/synchronously during tests (e.g., `CELERY_TASK_ALWAYS_EAGER=True` or an equivalent setting). Therefore, when reviewing these tests, don’t flag “race condition” or “async Celery” concerns for code that assumes task completion—tasks are guaranteed to finish before execution proceeds to the next line of test code.
Applied to files:
openwisp_notifications/tests/test_notifications.pyopenwisp_notifications/tests/test_admin.py
🔇 Additional comments (10)
openwisp_notifications/tests/test_notifications.py (4)
80-80: LGTM!
97-97: LGTM!
131-131: LGTM!
1989-1989: LGTM!openwisp_notifications/tests/test_admin.py (1)
66-66: LGTM!docs/developer/sending-notifications.rst (2)
55-56: LGTM!
91-91: LGTM!openwisp_notifications/base/models.py (1)
85-98: LGTM!Also applies to: 218-227, 282-294
openwisp_notifications/handlers.py (1)
57-57: LGTM!Also applies to: 91-117
openwisp_notifications/utils.py (1)
164-167: LGTM!
BHARATH0153
force-pushed
the
fix/make-type-non-nullable
branch
3 times, most recently
from
ac53792 to
f2947e0
Compare
|
@nemesifier please review when you have time Thanks! |
BHARATH0153
force-pushed
the
fix/make-type-non-nullable
branch
from
f2947e0 to
e40338c
Compare
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 `@openwisp_notifications/tests/__init__.py`:
- Around line 10-22: Tests render notifications as "by None" because
notifications are being created with actor=None; update the test fixtures or the
notification rendering to avoid showing "None". Either change the test setup
that calls notify.send or Notification.objects.create to supply a real actor
(search for notify.send with type="default" and Notification.objects.create in
tests) or make the message formatter/template (e.g., the function or template
that formats the message like notification_message/format_message) skip the "by
{actor}" suffix when actor is None/empty (use a conditional so actor is only
appended if truthy). Ensure you update the fixtures in
openwisp_notifications/tests/__init__.py accordingly so user-facing text never
shows "None".
🪄 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: 06113583-07ef-4d02-bda3-6be5061fd901
📒 Files selected for processing (9)
docs/developer/sending-notifications.rstopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.pytests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.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). (3)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
**/*.{md,rst,txt,adoc}: Changes: If the change affects documented behavior, update the documentation to reflect the new behavior
Features: Documentation must mention the new feature; if the feature is heavily UI-related, a new section or page is appropriate
Files:
docs/developer/sending-notifications.rst
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,md,rst,txt,adoc}
📄 CodeRabbit inference engine (Custom checks)
General: Flag outdated or incorrect documentation, comments, or docstrings
Files:
docs/developer/sending-notifications.rsttests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala}: General: Flag potential security vulnerabilities in code
General: Flag obvious performance regressions such as heavy loops, repeated I/O, or unoptimized queries
General: Avoid unnecessary comments or docstrings for code that is already clear
General: Code formatting must be compact and readable; do not add excessive blank lines, especially inside function or method bodies
General: Flag unused or redundant code
General: Ensure variables, functions, classes, and files have descriptive and consistent names
General: New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.py
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,c,cpp,cs,swift,kt,scala,sh,bash}
📄 CodeRabbit inference engine (Custom checks)
General: 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:
tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.py
**/*.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
**/*.py: Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.pyopenwisp_notifications/tests/test_admin.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.py
openwisp_notifications/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings as translatable with Django i18n helpers, typically
gettext_lazyimported as_
Files:
openwisp_notifications/tests/test_admin.pyopenwisp_notifications/migrations/0013_make_notification_type_nonnullable.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/base/models.pyopenwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/utils.py
**/models.py
📄 CodeRabbit inference engine (AGENTS.md)
Preserve swappable model support and integration with
openwisp-usersorganizations and memberships in Django models
Files:
openwisp_notifications/base/models.py
openwisp_notifications/{serializers,views,handlers,tasks}.py
📄 CodeRabbit inference engine (AGENTS.md)
Preserve validation and permission checks when changing serializers, views, handlers, or tasks that include notification payloads with related objects and URLs
Files:
openwisp_notifications/handlers.py
🧠 Learnings (1)
📚 Learning: 2026-05-07T16:17:11.806Z
Learnt from: pandafy
Repo: openwisp/openwisp-notifications PR: 450
File: openwisp_notifications/tests/test_selenium.py:276-285
Timestamp: 2026-05-07T16:17:11.806Z
Learning: In OpenWISP projects’ test code (e.g., openwisp-notifications), Celery is configured project-wide to run tasks eagerly/synchronously during tests (e.g., `CELERY_TASK_ALWAYS_EAGER=True` or an equivalent setting). Therefore, when reviewing these tests, don’t flag “race condition” or “async Celery” concerns for code that assumes task completion—tasks are guaranteed to finish before execution proceeds to the next line of test code.
Applied to files:
openwisp_notifications/tests/test_admin.pyopenwisp_notifications/tests/__init__.pyopenwisp_notifications/tests/test_notifications.py
🔇 Additional comments (18)
openwisp_notifications/migrations/0013_make_notification_type_nonnullable.py (1)
14-26: Backfill legacyNULLvalues before enforcingNOT NULL.This concern was already raised in a previous review. The migration enforces non-nullability directly without backfilling existing NULL values, which will cause the migration to fail on databases with
notification.type IS NULLrows.tests/openwisp2/sample_notifications/migrations/0005_make_notification_type_nonnullable.py (1)
14-26: Mirror the null-backfill step in sample migration.This concern was already raised in a previous review. The sample app migration has the same issue as the main migration - it enforces non-nullability without backfilling existing NULL values.
openwisp_notifications/tests/__init__.py (1)
79-131: HTML template also renders actor as "None".Same issue as the plain-text fixture - the HTML email template shows "by None" in multiple notification titles. This affects user experience even in test scenarios.
docs/developer/sending-notifications.rst (1)
91-91: LGTM!openwisp_notifications/base/models.py (4)
85-98: LGTM!
218-227: LGTM!
260-278: LGTM!
281-293: LGTM!openwisp_notifications/handlers.py (2)
57-57: LGTM!
91-112: LGTM!openwisp_notifications/utils.py (1)
17-17: LGTM!Also applies to: 162-168
openwisp_notifications/tests/test_admin.py (1)
66-66: LGTM!openwisp_notifications/tests/test_notifications.py (6)
80-81: LGTM!
97-97: LGTM!Also applies to: 116-116
131-131: LGTM!
259-259: LGTM!Also applies to: 367-370
1869-1877: LGTM!
1999-1999: LGTM!
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
At first glance it looks good.
2 participants
Checklist
changes
Made
typerequired for notifications — removed the nullable fallback and all theif type is Noneguard code across models, handlers, and utils.NotificationSetting.typestays nullable since that is intentional for global/all-types settings.notify.send()now requirestypeNotification.typeis non-nullable (with migration)type="default"Closes #481