[change:api] Include reset period in UserRadiusUsageView response #669#670
[change:api] Include reset period in UserRadiusUsageView response #669#670PRoIISHAAN wants to merge 5 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:
📝 WalkthroughWalkthroughAdds a Sequence Diagram(s)sequenceDiagram
participant Client
participant View
participant Serializer
participant AppSettings as app_settings
participant CounterModule as Counter
Client->>View: GET UserRadiusUsage
View->>Serializer: serialize(checks)
Serializer->>AppSettings: lookup CHECK_ATTRIBUTE_COUNTERS_MAP[type]
AppSettings-->>Serializer: counter_class
Serializer->>CounterModule: counter_class.get_reset_timestamps(obj)
CounterModule-->>Serializer: {start_time, end_time} or raise/None
Serializer-->>View: serialized data (includes reset)
View-->>Client: HTTP 200 with JSON (includes reset)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @PRoIISHAAN 👍
We need to add a test for this, see also my comment below.
PRoIISHAAN
force-pushed
the
api-change
branch
2 times, most recently
from
a4c8476 to
8e1cf73
Compare
PRoIISHAAN
force-pushed
the
api-change
branch
from
8e1cf73 to
5d5e786
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_radius/tests/test_api/test_api.py (1)
1094-1115: Test assertions forresetfield only verify presence, not correctness.The assertions use
checks[0]["reset"]as the expected value, which is self-referential and only confirms the field exists. Unlikeresultwhich is tested with concrete expected values (e.g.,0,261), theresetfield's actual value is never validated.Consider at minimum:
- Asserting that
resetis notNonewhen a counter exists- Verifying the type is a valid datetime (e.g.,
assertIsInstanceor regex match for ISO format)- Or mocking time to get deterministic expected values
💡 Example improvement
from datetime import datetime # Add type assertion for reset field self.assertIsNotNone(checks[0]["reset"]) # Optionally verify it's a valid ISO datetime string datetime.fromisoformat(checks[0]["reset"].replace("Z", "+00:00"))
🧹 Nitpick comments (1)
openwisp_radius/api/serializers.py (1)
330-341: LGTM! Consider extracting shared counter instantiation logic.The implementation correctly mirrors the existing
get_resultpattern and handles exceptions appropriately. The static analysis hint (TRY300) about moving the return to anelseblock is a style preference—sinceget_resultuses the same pattern, consistency is more valuable here.For a future improvement, you could extract the shared counter instantiation logic (lines 307-313 and 332-337) into a helper method to reduce duplication, but this is optional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_radius/api/serializers.pyopenwisp_radius/tests/test_api/test_api.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_radius/api/serializers.py (3)
openwisp_radius/counters/base.py (2)
reset(39-40)get_reset_timestamps(81-89)openwisp_radius/models.py (1)
RadiusGroupCheck(49-52)openwisp_radius/counters/exceptions.py (1)
SkipCheck(13-18)
🪛 Ruff (0.14.13)
openwisp_radius/api/serializers.py
339-339: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (2)
openwisp_radius/tests/test_api/test_api.py (2)
1064-1075: LGTM!The test correctly verifies that
resetisNonewhen the counter attribute doesn't exist inCHECK_ATTRIBUTE_COUNTERS_MAP.
1245-1265: LGTM!This test appropriately mirrors the
test_user_group_check_serializer_counter_does_not_existtest inTestApifor the transaction test case.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_radius/tests/test_api/test_api.py (1)
1307-1457:⚠️ Potential issue | 🟠 MajorTautological
resetassertions don't validate the feature — tests will always pass regardless of the actual value.
"reset": checks[0]["reset"]asserts that a key equals itself, which is trivially true. ForMax-Daily-SessionandMax-Daily-Session-Traffic(both daily counters),resetshould be a non-NoneISO 8601 datetime string representing the next midnight. The test suite would pass even ifget_resetreturned a completely wrong value, or even if it always returnedNone.✅ Proposed fix — assert non-None and validate format
Replace the tautological assertions with explicit value checks. For example, for each subtest:
- "reset": checks[0]["reset"], + "reset": checks[0]["reset"], # keep for dict equality, but add:Then add explicit assertions after each
assertDictEqual:+ self.assertIsNotNone(checks[0]["reset"]) + self.assertIsNotNone(checks[1]["reset"])Or, more thoroughly, validate the format using a
datetime.fromisoformat/dateutil.parser.parsecheck:from django.utils.dateparse import parse_datetime self.assertIsNotNone(parse_datetime(checks[0]["reset"])) self.assertIsNotNone(parse_datetime(checks[1]["reset"]))Alternatively, compute the expected next-midnight value and assert it directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/tests/test_api/test_api.py` around lines 1307 - 1457, The tests currently assert `"reset": checks[0]["reset"]` which is tautological and doesn't validate reset values; update the subtests that check `checks[0]` and `checks[1]` (the entries for "Max-Daily-Session" and "Max-Daily-Session-Traffic") to instead assert that `checks[i]["reset"]` is not None and is a valid ISO8601 datetime (e.g., use Django's `parse_datetime` / `datetime.fromisoformat` to parse and assert parsing succeeded), or compute the expected next-midnight datetime and assert equality—replace the self-referential `"reset": checks[i]["reset"]` entries with these explicit assertions.openwisp_radius/api/serializers.py (1)
309-344: 🧹 Nitpick | 🔵 TrivialExtract a shared
_get_counter(obj)helper to avoid double counter instantiation.Both
get_result(Line 312–316) andget_reset(Line 336–340) independently instantiate the sameCounterobject for the sameobj. Each instantiation may trigger database access (e.g., inconsumed()andget_reset_timestamps()), so serializing a single check item fires the counter constructor twice.♻️ Proposed refactor
+ def _get_counter(self, obj): + Counter = app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute] + return Counter( + user=self.context["user"], + group=self.context["group"], + group_check=obj, + ) + def get_result(self, obj): try: - Counter = app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute] - counter = Counter( - user=self.context["user"], - group=self.context["group"], - group_check=obj, - ) + counter = self._get_counter(obj) consumed = counter.consumed() value = int(obj.value) if consumed > value: consumed = value return consumed except (SkipCheck, ValueError, KeyError): return None def get_reset(self, obj): try: - Counter = app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute] - counter = Counter( - user=self.context["user"], - group=self.context["group"], - group_check=obj, - ) + counter = self._get_counter(obj) _, end_time = counter.get_reset_timestamps() return end_time except (SkipCheck, ValueError, KeyError): return NoneNote:
_get_counteritself should remain free of a try/except, since callers already handleKeyErrorandSkipCheck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/api/serializers.py` around lines 309 - 344, Add a private helper method _get_counter(self, obj) that looks up the Counter class from app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute] and returns Counter(user=self.context["user"], group=self.context["group"], group_check=obj) (do not wrap it in try/except). Replace the duplicated Counter instantiation in get_result(self, obj) and get_reset(self, obj) to call self._get_counter(obj) and keep the existing exception handling in those methods intact; leave get_type as-is (it only needs the Counter class lookup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_radius/api/serializers.py`:
- Around line 309-344: Add a private helper method _get_counter(self, obj) that
looks up the Counter class from
app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute] and returns
Counter(user=self.context["user"], group=self.context["group"], group_check=obj)
(do not wrap it in try/except). Replace the duplicated Counter instantiation in
get_result(self, obj) and get_reset(self, obj) to call self._get_counter(obj)
and keep the existing exception handling in those methods intact; leave get_type
as-is (it only needs the Counter class lookup).
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1307-1457: The tests currently assert `"reset":
checks[0]["reset"]` which is tautological and doesn't validate reset values;
update the subtests that check `checks[0]` and `checks[1]` (the entries for
"Max-Daily-Session" and "Max-Daily-Session-Traffic") to instead assert that
`checks[i]["reset"]` is not None and is a valid ISO8601 datetime (e.g., use
Django's `parse_datetime` / `datetime.fromisoformat` to parse and assert parsing
succeeded), or compute the expected next-midnight datetime and assert
equality—replace the self-referential `"reset": checks[i]["reset"]` entries with
these explicit assertions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
openwisp_radius/api/serializers.pyopenwisp_radius/tests/test_api/test_api.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.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | 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.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_radius/api/serializers.py (3)
openwisp_radius/counters/base.py (2)
reset(39-40)get_reset_timestamps(81-89)openwisp_radius/models.py (1)
RadiusGroupCheck(49-52)openwisp_radius/counters/exceptions.py (1)
SkipCheck(13-18)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_radius/tests/test_api/test_api.py (1)
1577-1598:⚠️ Potential issue | 🟡 MinorTests should validate actual reset timestamp values, not compare to themselves.
The assertions use
"reset": checks[0]["reset"]and"reset": checks[1]["reset"], which compares the value to itself. This verifies the field exists but doesn't validate the correctness of the computed reset timestamp.For meaningful test coverage, consider asserting against expected values. For daily counters, the reset should be the Unix timestamp of the next day's start.
💚 Suggested test improvement
+from datetime import datetime, timedelta +from django.utils import timezone + +def _get_expected_daily_reset(): + """Returns expected Unix timestamp for next daily reset.""" + now = timezone.now() + tomorrow = datetime(now.year, now.month, now.day) + timedelta(days=1) + return int(tomorrow.timestamp()) + # In test assertions: self.assertDictEqual( dict(checks[0]), { "attribute": "Max-Daily-Session", "op": ":=", "value": "10800", "result": 0, "type": "seconds", - "reset": checks[0]["reset"], + "reset": self._get_expected_daily_reset(), }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_radius/tests/test_api/test_api.py` around lines 1577 - 1598, The test currently validates "reset" by comparing it to itself (using checks[0]["reset"] / checks[1]["reset"]) which only verifies presence; replace those self-referential assertions with computed expected reset timestamps for the next day's start and assert equality (or a small allowable delta) against checks[i]["reset"] for the entries with attribute names "Max-Daily-Session" and "Max-Daily-Session-Traffic"; compute the expected reset as the Unix timestamp for midnight of the next day in the same timezone/UTC logic used by the production code and use that value in the dict passed to assertDictEqual (or assertAlmostEqual/assertInRange if timezone/precision requires tolerance) so the test verifies the actual reset calculation.
🤖 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_radius/api/serializers.py`:
- Around line 333-344: The get_reset method currently returns a raw Unix
timestamp from counter.get_reset_timestamps(); update get_reset (in the
serializer method get_reset) to convert the integer end_time into an ISO 8601
datetime string before returning (e.g., use datetime.fromtimestamp/end_time with
UTC and .isoformat() or append 'Z' for UTC) so API consumers receive a
human-readable ISO datetime; preserve the existing exception handling
(SkipCheck, ValueError, KeyError) and still return None on failure.
---
Outside diff comments:
In `@openwisp_radius/tests/test_api/test_api.py`:
- Around line 1577-1598: The test currently validates "reset" by comparing it to
itself (using checks[0]["reset"] / checks[1]["reset"]) which only verifies
presence; replace those self-referential assertions with computed expected reset
timestamps for the next day's start and assert equality (or a small allowable
delta) against checks[i]["reset"] for the entries with attribute names
"Max-Daily-Session" and "Max-Daily-Session-Traffic"; compute the expected reset
as the Unix timestamp for midnight of the next day in the same timezone/UTC
logic used by the production code and use that value in the dict passed to
assertDictEqual (or assertAlmostEqual/assertInRange if timezone/precision
requires tolerance) so the test verifies the actual reset calculation.
🪄 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: 909fdb63-6c61-48ff-ba0e-dc068ad1695e
📒 Files selected for processing (2)
openwisp_radius/api/serializers.pyopenwisp_radius/tests/test_api/test_api.py
📜 Review details
🔇 Additional comments (3)
openwisp_radius/api/serializers.py (1)
300-307: LGTM!The addition of the
resetfield toUserGroupCheckSerializerfollows the existing patterns established byresultandtypefields. The implementation correctly:
- Uses
SerializerMethodFieldconsistent with other computed fields- Includes the field in
Meta.fields- Handles exceptions appropriately
openwisp_radius/tests/test_api/test_api.py (2)
1068-1078: LGTM!The test correctly verifies that when a counter does not exist for the attribute (
ChilliSpot-Max-Input-Octets), theresetfield isNone. This matches the expected behavior fromget_resetreturningNoneonKeyError.
1738-1748: LGTM!The test correctly verifies the
reset: Nonebehavior in the transaction test context.
| def get_reset(self, obj): | ||
| try: | ||
| Counter = app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute] | ||
| counter = Counter( | ||
| user=self.context["user"], | ||
| group=self.context["group"], | ||
| group_check=obj, | ||
| ) | ||
| _, end_time = counter.get_reset_timestamps() | ||
| return end_time | ||
| except (SkipCheck, ValueError, KeyError): | ||
| return None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider converting Unix timestamp to ISO datetime string for API consistency.
The get_reset method returns a raw integer Unix timestamp from get_reset_timestamps(), while typical REST API datetime fields return ISO 8601 formatted strings. API consumers might expect a human-readable datetime format rather than a Unix timestamp.
If this is intentional for performance or client-side handling reasons, consider documenting the format in the API specification.
♻️ Optional: Convert to ISO datetime string
+from datetime import datetime
+
def get_reset(self, obj):
try:
Counter = app_settings.CHECK_ATTRIBUTE_COUNTERS_MAP[obj.attribute]
counter = Counter(
user=self.context["user"],
group=self.context["group"],
group_check=obj,
)
_, end_time = counter.get_reset_timestamps()
+ if end_time is not None:
+ return datetime.fromtimestamp(end_time, tz=timezone.utc).isoformat()
return end_time
except (SkipCheck, ValueError, KeyError):
return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_radius/api/serializers.py` around lines 333 - 344, The get_reset
method currently returns a raw Unix timestamp from
counter.get_reset_timestamps(); update get_reset (in the serializer method
get_reset) to convert the integer end_time into an ISO 8601 datetime string
before returning (e.g., use datetime.fromtimestamp/end_time with UTC and
.isoformat() or append 'Z' for UTC) so API consumers receive a human-readable
ISO datetime; preserve the existing exception handling (SkipCheck, ValueError,
KeyError) and still return None on failure.
3 participants
Checklist
Reference to Existing Issue
Closes #669 >.
Description of Changes
Modified API response to return reset attribute in API