[monitoring/js] Consistent datetime format in RADIUS sessions tab #662#665
[monitoring/js] Consistent datetime format in RADIUS sessions tab #662#665Eeshu-Yadav wants to merge 2 commits intoopenwisp:masterfrom
Conversation
Eeshu-Yadav
force-pushed
the
issues/662-radius_sessions_issue
branch
2 times, most recently
from
aa60f50 to
830d07e
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
I don't think it is working as expected.
The created/modified timestamp of the Device is in different format from the "Start Time" of the RADIUS session
Check the formatting of "Created" and "Modified" datetime of the Device object:
Check the formatting of "Start time" field in the "RADIUS Sessions" tab:
Check if something like this will solve the problem:
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
|
Hey @Eeshu-Yadav, sorry for any confusion. I didn’t start working on this. Since the CI failure was fixed, I just updated the branch earlier to keep it in sync and just as a check. |
i though first , yaa ci failed previously , i thought , i will fix in next commit |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a monitoring accounting API for RadiusAccounting exposed at GET /api/radius/monitoring/sessions/. Introduces MonitoringRadiusAccountingSerializer that returns localized start_time/stop_time, a MonitoringAccountingView (ListAPIView with filtering and pagination), and conditional exposure of the view in api/views and api/urls only when the integration is available. Updates the admin monitoring template to point to the new monitoring_accounting endpoint and removes client-side datetime reformatting so the frontend uses server-localized start_time/stop_time. Tests add a local MonitoringAccountingView alias. Sequence Diagram(s)sequenceDiagram
participant Browser
participant AdminJS as Admin JS
participant API as MonitoringAccountingView (Server)
participant DB as RadiusAccounting (Database)
Browser->>AdminJS: Open device RADIUS sessions tab
AdminJS->>API: GET /api/radius/monitoring/sessions/?filters
API->>DB: Query RadiusAccounting (apply filters, order by start_time desc)
DB-->>API: Return records
API-->>AdminJS: JSON list (serialized localized start_time/stop_time)
AdminJS->>Browser: Render table (use start_time/stop_time as-is)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code graph analysis (1)openwisp_radius/integrations/monitoring/serializers.py (2)
🪛 Ruff (0.14.14)openwisp_radius/integrations/monitoring/serializers.py[warning] 39-49: Mutable class attributes should be annotated with (RUF012) ⏰ 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)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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
🤖 Fix all issues with AI agents
In
`@openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js`:
- Around line 23-31: The options object in device-change.js contains hour12:
false which forces 24-hour time; remove the hour12 property from the options
constant so toLocaleString() can honor the user's locale preferences (locate and
edit the options declaration in device-change.js where the options object with
year/month/day/hour/minute/second is defined and delete the hour12: false
entry).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js
🔇 Additional comments (1)
openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js (1)
22-22: No guard needed;djangoLocaleis always defined.The template explicitly sets
const djangoLocale = "{{ LANGUAGE_CODE }}"(line 39 ofchange_form.html) before this script loads, sodjangoLocaleis guaranteed to exist. A guard is unnecessary.The concern about
replace("_", "-")replacing only the first underscore is technically valid but low-impact: Django'sLANGUAGE_CODEsetting uses hyphens by convention (e.g.,"en-gb"), andIntl.DateTimeFormatgracefully falls back if an invalid locale string is provided.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request fixes inconsistent datetime formatting in the RADIUS sessions tab on the device page by moving datetime formatting from client-side JavaScript to server-side Django using the locale-aware formats.date_format() function.
Changes:
- Created new
MonitoringRadiusAccountingSerializerthat formats datetime fields server-side using Django's localization settings - Created new
MonitoringAccountingViewto serve the formatted data - Updated admin template to use new API endpoint and removed client-side datetime formatting from JavaScript
- Registered new URL pattern for the monitoring accounting endpoint
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openwisp_radius/integrations/monitoring/serializers.py | New serializer with server-side datetime formatting using Django's DATETIME_FORMAT |
| openwisp_radius/integrations/monitoring/views.py | New API view for monitoring integration with formatted datetime output |
| openwisp_radius/integrations/monitoring/admin.py | Updated to point to new monitoring_accounting_list endpoint |
| openwisp_radius/integrations/monitoring/templates/admin/config/radius-monitoring/device/change_form.html | Removed djangoLocale variable (no longer needed) |
| openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js | Removed client-side datetime formatting function |
| openwisp_radius/api/views.py | Imported and registered MonitoringAccountingView |
| openwisp_radius/api/urls.py | Added new URL pattern for monitoring/sessions endpoint |
| tests/openwisp2/sample_radius/api/views.py | Extended MonitoringAccountingView in test app |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from django_filters.rest_framework import DjangoFilterBackend | ||
| from rest_framework.generics import ListAPIView | ||
| from swapper import load_model | ||
|
|
||
| from openwisp_radius.api.freeradius_views import ( | ||
| AccountingFilter, | ||
| AccountingViewPagination, | ||
| ) | ||
| from openwisp_users.api.mixins import FilterByOrganizationManaged, ProtectedAPIMixin | ||
|
|
||
| from .serializers import MonitoringRadiusAccountingSerializer | ||
|
|
||
| RadiusAccounting = load_model("openwisp_radius", "RadiusAccounting") | ||
|
|
||
|
|
||
| class MonitoringAccountingView( | ||
| ProtectedAPIMixin, FilterByOrganizationManaged, ListAPIView | ||
| ): | ||
| """ | ||
| API view for RADIUS accounting in monitoring integration. | ||
| Uses server-side datetime formatting for consistency with Django admin. | ||
| """ | ||
|
|
||
| throttle_scope = "radius_accounting_list" | ||
| serializer_class = MonitoringRadiusAccountingSerializer | ||
| pagination_class = AccountingViewPagination | ||
| filter_backends = (DjangoFilterBackend,) | ||
| filterset_class = AccountingFilter | ||
| queryset = RadiusAccounting.objects.all().order_by("-start_time") |
There was a problem hiding this comment.
The new MonitoringAccountingView lacks test coverage. The repository has comprehensive API test coverage (see openwisp_radius/tests/test_api/ directory), and similar views like RadiusAccountingView have dedicated tests (test_radius_accounting in test_api.py). Consider adding tests that verify:
- The datetime formatting is working correctly with Django's localization
- The view properly filters by organization
- The view handles authentication/authorization correctly
- The formatted datetime strings match the expected Django admin format
| from django.utils import formats, timezone | ||
| from rest_framework import serializers | ||
| from swapper import load_model | ||
|
|
||
| RadiusAccounting = load_model("openwisp_radius", "RadiusAccounting") | ||
|
|
||
|
|
||
| class MonitoringRadiusAccountingSerializer(serializers.ModelSerializer): | ||
| """ | ||
| Read-only serializer for RADIUS accounting in monitoring integration | ||
| that formats datetime fields server-side using Django's localization | ||
| for consistency with Django admin datetime formatting. | ||
| """ | ||
|
|
||
| start_time = serializers.SerializerMethodField() | ||
| stop_time = serializers.SerializerMethodField() | ||
|
|
||
| def get_start_time(self, obj): | ||
| """Format start_time using Django's localization settings""" | ||
| if not obj.start_time: | ||
| return None | ||
| local_time = timezone.localtime(obj.start_time) | ||
| return formats.date_format(local_time, "DATETIME_FORMAT") | ||
|
|
||
| def get_stop_time(self, obj): | ||
| """Format stop_time using Django's localization settings""" | ||
| if not obj.stop_time: | ||
| return None | ||
| local_time = timezone.localtime(obj.stop_time) | ||
| return formats.date_format(local_time, "DATETIME_FORMAT") | ||
|
|
||
| class Meta: | ||
| model = RadiusAccounting | ||
| fields = [ | ||
| "session_id", | ||
| "unique_id", | ||
| "username", | ||
| "input_octets", | ||
| "output_octets", | ||
| "calling_station_id", | ||
| "called_station_id", | ||
| "start_time", | ||
| "stop_time", | ||
| ] | ||
| read_only_fields = fields |
There was a problem hiding this comment.
The new MonitoringRadiusAccountingSerializer lacks test coverage. The repository has comprehensive API test coverage, and serializers are typically tested alongside their views. Consider adding tests that verify:
- The start_time and stop_time fields are correctly formatted using Django's DATETIME_FORMAT
- The formatting works correctly with different timezone settings
- None values are handled correctly for start_time and stop_time
- The serialized output contains all expected fields
openwisp_radius/api/views.py
Outdated
| SmsAttemptCooldownException, | ||
| UserAlreadyVerified, | ||
| ) | ||
| from ..integrations.monitoring.views import MonitoringAccountingView |
There was a problem hiding this comment.
This import will cause an ImportError when the monitoring integration is not installed, since the integration is optional (see docs/user/radius_monitoring.rst). The import should be conditional or the view should be defined within the monitoring integration's own URL configuration. Consider either:
- Moving the monitoring_accounting view definition to openwisp_radius/integrations/monitoring/api/views.py and registering it there
- Using a conditional import like:
try:
from ..integrations.monitoring.views import MonitoringAccountingView
monitoring_accounting = MonitoringAccountingView.as_view()
except ImportError:
monitoring_accounting = NoneAnd conditionally adding the URL only when monitoring_accounting is not None.
| from ..integrations.monitoring.views import MonitoringAccountingView | |
| try: | |
| from ..integrations.monitoring.views import MonitoringAccountingView | |
| monitoring_accounting = MonitoringAccountingView.as_view() | |
| except ImportError: | |
| MonitoringAccountingView = None | |
| monitoring_accounting = None |
openwisp_radius/api/urls.py
Outdated
| path( | ||
| "radius/monitoring/sessions/", | ||
| api_views.monitoring_accounting, | ||
| name="monitoring_accounting_list", | ||
| ), |
There was a problem hiding this comment.
This URL registration should be conditional on whether the monitoring integration is installed. When api_views.monitoring_accounting is None or doesn't exist (in installations without the monitoring integration), this will cause an error. Consider checking if the view exists before adding it to the URL patterns, or handle this in the monitoring integration's own URL configuration.
…nwisp#662 Use Django locale for date formatting in device page RADIUS sessions tab JS, matching Django admin and localization settings. Fixes openwisp#662
Eeshu-Yadav
force-pushed
the
issues/662-radius_sessions_issue
branch
from
18f2767 to
b189d0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_radius/integrations/monitoring/serializers.py`:
- Around line 18-30: The serializer currently passes datetimes directly to
timezone.localtime() in get_start_time and get_stop_time, which will raise
ValueError for naive datetimes; update both methods to first check
timezone.is_aware(obj.start_time/obj.stop_time) and, if naive, call
timezone.make_aware(obj.start_time/obj.stop_time,
timezone.get_default_timezone()) before calling timezone.localtime(), then
format with formats.date_format as before so external/legacy naive RADIUS
timestamps are handled safely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
openwisp_radius/api/urls.pyopenwisp_radius/api/views.pyopenwisp_radius/integrations/monitoring/admin.pyopenwisp_radius/integrations/monitoring/serializers.pyopenwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.jsopenwisp_radius/integrations/monitoring/templates/admin/config/radius-monitoring/device/change_form.htmlopenwisp_radius/integrations/monitoring/views.pytests/openwisp2/sample_radius/api/views.py
💤 Files with no reviewable changes (2)
- openwisp_radius/integrations/monitoring/static/radius-monitoring/js/device-change.js
- openwisp_radius/integrations/monitoring/templates/admin/config/radius-monitoring/device/change_form.html
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/openwisp2/sample_radius/api/views.py
- openwisp_radius/integrations/monitoring/admin.py
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_radius/api/views.py (2)
openwisp_radius/integrations/monitoring/views.py (1)
MonitoringAccountingView(16-29)tests/openwisp2/sample_radius/api/views.py (1)
MonitoringAccountingView(104-105)
openwisp_radius/integrations/monitoring/views.py (4)
openwisp_radius/utils.py (1)
load_model(33-34)openwisp_radius/api/freeradius_views.py (2)
AccountingFilter(67-83)AccountingViewPagination(466-469)openwisp_radius/integrations/monitoring/serializers.py (1)
MonitoringRadiusAccountingSerializer(8-45)tests/openwisp2/sample_radius/api/views.py (1)
MonitoringAccountingView(104-105)
openwisp_radius/integrations/monitoring/serializers.py (2)
openwisp_radius/utils.py (1)
load_model(33-34)openwisp_radius/api/views.py (1)
Meta(412-415)
🪛 Ruff (0.14.14)
openwisp_radius/integrations/monitoring/serializers.py
[warning] 34-44: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (8)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.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~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🔇 Additional comments (5)
openwisp_radius/integrations/monitoring/serializers.py (2)
8-17: Good move to localize datetime formatting server-side.This aligns well with the goal of consistent locale-aware formatting.
32-45: Meta fields/read-only configuration looks good.Clear and explicit field exposure for the monitoring endpoint.
openwisp_radius/api/urls.py (1)
91-95: New monitoring sessions route is correctly wired.Naming and URL pattern are consistent with the existing API routes.
openwisp_radius/integrations/monitoring/views.py (1)
16-29: Monitoring view configuration is solid.Permissions, filtering, and pagination are consistent with existing accounting views.
openwisp_radius/api/views.py (1)
56-56: Monitoring endpoint exposure looks correct.Import and view alias match the new API route.
Also applies to: 851-852
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def get_start_time(self, obj): | ||
| """Format start_time using Django's localization settings""" | ||
| if not obj.start_time: | ||
| return None | ||
| local_time = timezone.localtime(obj.start_time) | ||
| return formats.date_format(local_time, "DATETIME_FORMAT") | ||
|
|
||
| def get_stop_time(self, obj): | ||
| """Format stop_time using Django's localization settings""" | ||
| if not obj.stop_time: | ||
| return None | ||
| local_time = timezone.localtime(obj.stop_time) | ||
| return formats.date_format(local_time, "DATETIME_FORMAT") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Django timezone.localtime naive datetime behavior USE_TZ False
💡 Result:
With USE_TZ = False, Django operates in local time using naive datetime objects (i.e., no tzinfo). That means there’s nothing to “convert to local time” — values are already assumed local. (docs.djangoproject.com)
django.utils.timezone.localtime() with naive datetimes (including when USE_TZ=False)
timezone.localtime(value) requires an aware datetime. If value is naive, it raises:
ValueError: localtime() cannot be applied to a naive datetime(docs.djangoproject.com)
This is true regardless of USE_TZ.
timezone.localtime() with no argument when USE_TZ=False
If you call timezone.localtime() with no value, it defaults to timezone.now(). But timezone.now() returns a naive datetime when USE_TZ=False, so localtime() will then raise the same ValueError. (docs.djangoproject.com)
Template behavior difference
Django’s template helper (template_localtime) only converts when time zone support is enabled and the datetime is aware; otherwise it returns the value unchanged. (docs.djangoproject.com)
Practical takeaway
- If
USE_TZ=False: don’t usetimezone.localtime(); treat your naive datetimes as already-local. (docs.djangoproject.com) - If you need conversions between time zones: set
USE_TZ=Trueand use aware datetimes. (docs.djangoproject.com)
Citations:
- 1: https://docs.djangoproject.com/en/4.0/topics/i18n/timezones/?utm_source=openai
- 2: https://docs.djangoproject.com/en/3.2/_modules/django/utils/timezone/?utm_source=openai
- 3: https://docs.djangoproject.com/en/3.2/_modules/django/utils/timezone/?utm_source=openai
- 4: https://docs.djangoproject.com/en/3.2/_modules/django/utils/timezone/?utm_source=openai
- 5: https://docs.djangoproject.com/en/4.0/topics/i18n/timezones/?utm_source=openai
- 6: https://docs.djangoproject.com/en/4.0/topics/i18n/timezones/?utm_source=openai
🏁 Script executed:
# Find and examine the serializer file
fd -t f "serializers.py" | grep monitoringRepository: openwisp/openwisp-radius
Length of output: 122
🏁 Script executed:
# Check the model definition and imports in the serializer
head -40 openwisp_radius/integrations/monitoring/serializers.pyRepository: openwisp/openwisp-radius
Length of output: 1447
🏁 Script executed:
# Check Django settings for USE_TZ configuration
fd -t f "settings.py" | head -5 | xargs grep -l "USE_TZ"Repository: openwisp/openwisp-radius
Length of output: 52
🏁 Script executed:
# Find the RadiusAccounting model
fd -t f "models.py" | grep radius | head -5Repository: openwisp/openwisp-radius
Length of output: 201
🏁 Script executed:
# Search for timezone.localtime usage in the codebase
rg "timezone\.localtime" -A 2 -B 2Repository: openwisp/openwisp-radius
Length of output: 974
🏁 Script executed:
# Search for RadiusAccounting model definition
rg "class RadiusAccounting" -A 10Repository: openwisp/openwisp-radius
Length of output: 5857
🏁 Script executed:
# Check for USE_TZ in settings
rg "USE_TZ\s*=" openwisp_radius/Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Find AbstractRadiusAccounting definition
rg "class AbstractRadiusAccounting" -A 20Repository: openwisp/openwisp-radius
Length of output: 1398
🏁 Script executed:
# Look for start_time and stop_time field definitions
rg "start_time|stop_time" -B 2 -A 2 openwisp_radius/base/models.py | head -50Repository: openwisp/openwisp-radius
Length of output: 1751
🏁 Script executed:
# Check if there are tests for the monitoring serializer
fd -t f "test*monitoring*" | grep -i serializerRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Search for any datetime handling or timezone awareness in tests
rg "start_time|stop_time" openwisp_radius/integrations/monitoring/ -t pyRepository: openwisp/openwisp-radius
Length of output: 5562
🏁 Script executed:
# Check the DateTimeField definitions more thoroughly
rg "start_time|stop_time" openwisp_radius/base/models.py -B 3 -A 3Repository: openwisp/openwisp-radius
Length of output: 1937
🏁 Script executed:
# Check if there are any tests that exercise the serializer with naive datetimes
fd -t f "test_metrics.py" -exec head -100 {} \;Repository: openwisp/openwisp-radius
Length of output: 256
🏁 Script executed:
# Look for any timezone awareness checks in similar code
rg "is_aware|is_naive" openwisp_radius/Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check test files for the serializer usage
fd -t f "test" openwisp_radius/integrations/monitoring/ | xargs grep -l "Serializer\|serializer"Repository: openwisp/openwisp-radius
Length of output: 52
🏁 Script executed:
# Look at the test file that likely uses the serializer
cat openwisp_radius/integrations/monitoring/tests/test_metrics.py | head -100Repository: openwisp/openwisp-radius
Length of output: 4130
🏁 Script executed:
# Check if there's a specific test for the monitoring serializer
rg "MonitoringRadiusAccountingSerializer" openwisp_radius/integrations/monitoring/tests/Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check what now() function is being used and how it behaves with USE_TZ
rg "from.*import.*now|def now" openwisp_radius/base/models.py -B 2 -A 2Repository: openwisp/openwisp-radius
Length of output: 276
🏁 Script executed:
# Check the actual project settings for USE_TZ
find . -name "settings*.py" -type f | head -10Repository: openwisp/openwisp-radius
Length of output: 181
🏁 Script executed:
# Look at test settings to see if USE_TZ is set
cat tests/openwisp2/settings.py | grep -i "use_tz" -A 2 -B 2Repository: openwisp/openwisp-radius
Length of output: 290
🏁 Script executed:
# Check if there are any integration tests for the monitoring serializer that use actual data
rg "MonitoringRadiusAccountingSerializer\|get_start_time\|get_stop_time" openwisp_radius/ -A 5Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check how data flows from RADIUS protocol (which often produces naive datetimes) into the serializer
rg "_RADACCT|start_time.*=" openwisp_radius/tests/ -B 2 -A 2 | head -50Repository: openwisp/openwisp-radius
Length of output: 3134
🏁 Script executed:
# Check _RADACCT definition to see what datetime format is used for test data
rg "_RADACCT\s*=" openwisp_radius/tests/ -A 20Repository: openwisp/openwisp-radius
Length of output: 1318
🏁 Script executed:
# Look for any data migrations or imports from external RADIUS sources
rg "naive|aware|timezone|import.*datetime" openwisp_radius/tests/__init__.pyRepository: openwisp/openwisp-radius
Length of output: 130
🏁 Script executed:
# Check if there are any settings variations where USE_TZ might be False
rg "USE_TZ" openwisp_radius/ --include="*.py"Repository: openwisp/openwisp-radius
Length of output: 509
🏁 Script executed:
# Check for USE_TZ in the entire codebase more carefully
rg "USE_TZ" openwisp_radius/ -t pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check what happens when datetime strings are parsed - check the model creation
rg "start_time.*=" openwisp_radius/tests/__init__.py -B 5 -A 5Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Let's understand how the test data (string) gets converted to a datetime object
cat openwisp_radius/tests/__init__.py | head -50Repository: openwisp/openwisp-radius
Length of output: 1647
🏁 Script executed:
# Check if there are any database interactions that could affect timezone awareness
rg "strptime|parse_datetime|DateTimeField" openwisp_radius/base/models.py -B 2 -A 2Repository: openwisp/openwisp-radius
Length of output: 882
🏁 Script executed:
# Check if there are API tests that actually call the monitoring serializer
fd -t f "test" openwisp_radius/integrations/monitoring/ -exec grep -l "serializer\|api\|json" {} \;Repository: openwisp/openwisp-radius
Length of output: 283
🏁 Script executed:
# Look for views that use the monitoring serializer
rg "MonitoringRadiusAccountingSerializer" openwisp_radius/integrations/monitoring/views.py -B 5 -A 5Repository: openwisp/openwisp-radius
Length of output: 831
🏁 Script executed:
# Check if there are API endpoint tests that would exercise the serializer
fd -t f "test*.py" openwisp_radius/integrations/monitoring/tests/Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# List all test files in monitoring integration
fd -t f "test" openwisp_radius/integrations/monitoring/tests/Repository: openwisp/openwisp-radius
Length of output: 189
🏁 Script executed:
# Check if there are any API tests for the monitoring endpoint
rg "MonitoringAccountingView\|api.*accounting\|test.*monitoring" openwisp_radius/integrations/monitoring/tests/ -lRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Look at the test directory structure
ls -la openwisp_radius/integrations/monitoring/tests/Repository: openwisp/openwisp-radius
Length of output: 433
🏁 Script executed:
# Check test_admin.py for serializer usage
rg "serializer\|Serializer" openwisp_radius/integrations/monitoring/tests/test_admin.pyRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there's any test that actually exercises the API with the serializer
rg "get\(.*api\|get_json\|api.*accounting" openwisp_radius/integrations/monitoring/tests/test_admin.py -A 5Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Look for any comment or issue reference about this serializer being added
rg "localtime\|timezone.*aware\|naive" openwisp_radius/integrations/monitoring/ -B 3 -A 3Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there's a way the datetime could actually be naive - look at how RADIUS accounting data is created
rg "RadiusAccounting.objects.create\|_create_radius_accounting" openwisp_radius/tests/mixins.py -A 10Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Check if there's any documentation about how RADIUS accounting data is imported
rg "import\|migrate\|load" openwisp_radius/integrations/monitoring/ | grep -i accountRepository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Let's search for any other similar patterns where timezone handling might be incomplete
rg "formats.date_format" openwisp_radius/ -B 3 -A 3Repository: openwisp/openwisp-radius
Length of output: 1342
🏁 Script executed:
# Check what Django version this project targets
cat setup.py | grep -i djangoRepository: openwisp/openwisp-radius
Length of output: 374
🏁 Script executed:
# Check if there's any documentation or comments about timezone handling in RADIUS
rg "timezone\|aware\|naive\|USE_TZ" openwisp_radius/base/models.py | head -20Repository: openwisp/openwisp-radius
Length of output: 50
🏁 Script executed:
# Verify the exact Django version requirement
cat setup.py | head -50Repository: openwisp/openwisp-radius
Length of output: 1723
🏁 Script executed:
# Check if there are any RADIUS data import scripts that might introduce naive datetimes
rg "import.*csv\|loaddata\|migrate" openwisp_radius/ -l | head -5Repository: openwisp/openwisp-radius
Length of output: 50
Guard against naive datetimes before calling timezone.localtime().
timezone.localtime() raises ValueError when passed a naive datetime, regardless of the USE_TZ setting. Since RADIUS accounting data originates from external sources and may include legacy naive datetimes, the serializer should check timezone awareness before conversion.
Suggested fix
def get_start_time(self, obj):
"""Format start_time using Django's localization settings"""
if not obj.start_time:
return None
- local_time = timezone.localtime(obj.start_time)
- return formats.date_format(local_time, "DATETIME_FORMAT")
+ value = obj.start_time
+ if timezone.is_aware(value):
+ value = timezone.localtime(value)
+ return formats.date_format(value, "DATETIME_FORMAT")
def get_stop_time(self, obj):
"""Format stop_time using Django's localization settings"""
if not obj.stop_time:
return None
- local_time = timezone.localtime(obj.stop_time)
- return formats.date_format(local_time, "DATETIME_FORMAT")
+ value = obj.stop_time
+ if timezone.is_aware(value):
+ value = timezone.localtime(value)
+ return formats.date_format(value, "DATETIME_FORMAT")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_start_time(self, obj): | |
| """Format start_time using Django's localization settings""" | |
| if not obj.start_time: | |
| return None | |
| local_time = timezone.localtime(obj.start_time) | |
| return formats.date_format(local_time, "DATETIME_FORMAT") | |
| def get_stop_time(self, obj): | |
| """Format stop_time using Django's localization settings""" | |
| if not obj.stop_time: | |
| return None | |
| local_time = timezone.localtime(obj.stop_time) | |
| return formats.date_format(local_time, "DATETIME_FORMAT") | |
| def get_start_time(self, obj): | |
| """Format start_time using Django's localization settings""" | |
| if not obj.start_time: | |
| return None | |
| value = obj.start_time | |
| if timezone.is_aware(value): | |
| value = timezone.localtime(value) | |
| return formats.date_format(value, "DATETIME_FORMAT") | |
| def get_stop_time(self, obj): | |
| """Format stop_time using Django's localization settings""" | |
| if not obj.stop_time: | |
| return None | |
| value = obj.stop_time | |
| if timezone.is_aware(value): | |
| value = timezone.localtime(value) | |
| return formats.date_format(value, "DATETIME_FORMAT") |
🤖 Prompt for AI Agents
In `@openwisp_radius/integrations/monitoring/serializers.py` around lines 18 - 30,
The serializer currently passes datetimes directly to timezone.localtime() in
get_start_time and get_stop_time, which will raise ValueError for naive
datetimes; update both methods to first check
timezone.is_aware(obj.start_time/obj.stop_time) and, if naive, call
timezone.make_aware(obj.start_time/obj.stop_time,
timezone.get_default_timezone()) before calling timezone.localtime(), then
format with formats.date_format as before so external/legacy naive RADIUS
timestamps are handled safely.
|
please resolve code rabbit's comments by either: -> replying to the suggestions made (reply to it stating why u don't agree with it's approach / reply to it with a question related to it's approach to get more clarity on it) Or, -> applying the changes suggested by it if you're sure that they are worth addressing. |
…nwisp#662 Use Django locale for date formatting in device page RADIUS sessions tab JS, matching Django admin and localization settings. Fixes openwisp#662
Eeshu-Yadav
force-pushed
the
issues/662-radius_sessions_issue
branch
from
b189d0d to
dbd1b63
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav once you're done, please post a screenshot of the UI result in high res so we can verify the visual result matches our expectations (as @pandafy explained). Thanks 👍
Checklist
Use Django locale for date formatting in device page RADIUS sessions tab JS, matching Django admin and localization settings.
Fixes #662