feat(admin): enhance analytics endpoints and session insights#22
Open
VarshVishwakarma wants to merge 1 commit into
Open
feat(admin): enhance analytics endpoints and session insights#22VarshVishwakarma wants to merge 1 commit into
VarshVishwakarma wants to merge 1 commit into
Conversation
|
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR extends the admin analytics API with a new top-triggers aggregation endpoint, expands session risk and stats responses with peak risk metrics and category percentages, and includes comprehensive test coverage using isolated database fixtures for robust edge-case validation. ChangesAdmin Analytics Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_admin_api.py (3)
170-181: ⚡ Quick winClose the SQLite connection in
_blank_dbfixture.Line 170 opens a connection that is never explicitly closed in this fixture. This can cause intermittent DB file-lock behavior in test runs.
Suggested fix
def _blank_db(tmp_path, monkeypatch): """Creates a fresh, empty temp DB isolated from original seeded data.""" db_path = tmp_path / "test_admin_enhanced.db" import sqlite3 - conn = sqlite3.connect(str(db_path)) - with conn: + with sqlite3.connect(str(db_path)) as conn: conn.execute( """CREATE TABLE escalations ( id INTEGER PRIMARY KEY AUTOINCREMENT, session_id TEXT, category TEXT, risk_score REAL, triggers TEXT, timestamp REAL, message_hash TEXT, stage_reached INTEGER, reasoning TEXT )""" ) monkeypatch.setattr("humane_proxy.api.admin._get_db_path", lambda: str(db_path)) return str(db_path)🤖 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 `@tests/test_admin_api.py` around lines 170 - 181, The fixture `_blank_db` opens a SQLite connection via `sqlite3.connect` and uses `with conn:` to create the table but never closes the connection (`conn`), which can leave the DB file locked; close the connection before returning the path (either call `conn.close()` after the `with conn:` block or use `with sqlite3.connect(...) as conn:` to ensure the connection is closed) so that the test fixture releases the DB file before `monkeypatch.setattr("humane_proxy.api.admin._get_db_path", ...)` and return.
229-254: ⚡ Quick winMake the week-boundary test deterministic by freezing time.
Lines 231–248 use runtime
nowin the test while the API call computes its ownnow. Around boundary transitions, this can become flaky.Suggested direction
def test_period_comparison_strict_bounds(self, _blank_db): - now_dt = datetime.now(timezone.utc) + now_dt = datetime(2026, 5, 18, 12, 0, 0, tzinfo=timezone.utc) current_week_start_dt = now_dt.replace( hour=0, minute=0, second=0, microsecond=0 ) - timedelta(days=now_dt.weekday()) @@ - resp = client.get("/admin/stats", headers=self.HEADERS) + with patch("humane_proxy.api.admin.datetime") as mock_datetime: + mock_datetime.now.return_value = now_dt + mock_datetime.side_effect = lambda *args, **kwargs: datetime(*args, **kwargs) + resp = client.get("/admin/stats", headers=self.HEADERS)🤖 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 `@tests/test_admin_api.py` around lines 229 - 254, The test test_period_comparison_strict_bounds is flaky because it computes now locally while the API computes its own now; make the test deterministic by freezing time to a fixed UTC datetime so both the test setup (current_week_start_dt, pw_start and _insert_row timestamps) and the API use the same "now". Do this by applying a time freeze (e.g., freezegun.freeze_time or monkeypatching datetime.now in the module that implements /admin/stats) for the duration of the test, ensure the frozen time uses timezone.utc, then run client.get("/admin/stats", headers=self.HEADERS) and assert period_comparison as before.
205-205: ⚡ Quick winAvoid mutable class attributes for
HEADERSconstants.Lines 205, 259, and 331 define mutable dicts at class scope (
RUF012). Prefer immutable/explicit class constants to avoid accidental cross-test mutation.Suggested fix
+from typing import ClassVar @@ class TestStatsEnhanced: @@ - HEADERS = {"Authorization": "Bearer test-admin-secret"} + HEADERS: ClassVar[dict[str, str]] = {"Authorization": "Bearer test-admin-secret"} @@ class TestAnalyticsTopTriggers: @@ - HEADERS = {"Authorization": "Bearer test-admin-secret"} + HEADERS: ClassVar[dict[str, str]] = {"Authorization": "Bearer test-admin-secret"} @@ class TestSessionRiskEnhanced: @@ - HEADERS = {"Authorization": "Bearer test-admin-secret"} + HEADERS: ClassVar[dict[str, str]] = {"Authorization": "Bearer test-admin-secret"}Also applies to: 259-259, 331-331
🤖 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 `@tests/test_admin_api.py` at line 205, The class-scoped mutable dicts named HEADERS (present in the test classes) can be accidentally mutated across tests; replace each class-level HEADERS with an immutable representation or a factory: either wrap the dict in types.MappingProxyType to make it read-only or replace the class attribute with a tuple-of-tuples constant (e.g. HEADERS_TUPLE) and convert to a dict inside each test, or add a small helper function get_headers() that returns a fresh dict for use in tests; update every occurrence of the HEADERS symbol so tests use the immutable/factory form instead of a shared mutable dict.
🤖 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.
Nitpick comments:
In `@tests/test_admin_api.py`:
- Around line 170-181: The fixture `_blank_db` opens a SQLite connection via
`sqlite3.connect` and uses `with conn:` to create the table but never closes the
connection (`conn`), which can leave the DB file locked; close the connection
before returning the path (either call `conn.close()` after the `with conn:`
block or use `with sqlite3.connect(...) as conn:` to ensure the connection is
closed) so that the test fixture releases the DB file before
`monkeypatch.setattr("humane_proxy.api.admin._get_db_path", ...)` and return.
- Around line 229-254: The test test_period_comparison_strict_bounds is flaky
because it computes now locally while the API computes its own now; make the
test deterministic by freezing time to a fixed UTC datetime so both the test
setup (current_week_start_dt, pw_start and _insert_row timestamps) and the API
use the same "now". Do this by applying a time freeze (e.g.,
freezegun.freeze_time or monkeypatching datetime.now in the module that
implements /admin/stats) for the duration of the test, ensure the frozen time
uses timezone.utc, then run client.get("/admin/stats", headers=self.HEADERS) and
assert period_comparison as before.
- Line 205: The class-scoped mutable dicts named HEADERS (present in the test
classes) can be accidentally mutated across tests; replace each class-level
HEADERS with an immutable representation or a factory: either wrap the dict in
types.MappingProxyType to make it read-only or replace the class attribute with
a tuple-of-tuples constant (e.g. HEADERS_TUPLE) and convert to a dict inside
each test, or add a small helper function get_headers() that returns a fresh
dict for use in tests; update every occurrence of the HEADERS symbol so tests
use the immutable/factory form instead of a shared mutable dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56bb49d2-3731-4948-8b3b-a5d3fe5b152c
📒 Files selected for processing (2)
humane_proxy/api/admin.pytests/test_admin_api.py
Owner
|
@VarshVishwakarma please sign the CLA. Also, were you able to talk to the contributor responsible for admin dashboard? |
Author
Hey @Vishisht16 I have signed the CLA but forgot to talk with @asnaassalam can we invite her here? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR enhances the native Admin Analytics API in
admin.pyby improving escalation insights and session analytics for dashboard integration and operational monitoring.Closes #16
Changes Included
/admin/statsAdded:
category_percentagesperiod_comparisonfield comparing current week vs previous week escalation countsGET /admin/analytics/top-triggersImplemented a new analytics endpoint that:
aggregates escalation trigger keywords
ranks triggers by occurrence frequency
supports:
limitcategoryfiltering/admin/sessions/{id}/riskAdded:
peak_risk_scorepeak_risk_timestampcategory_transitionsType of Change
Testing
Added comprehensive tests covering:
Validation
Checklist
pytest tests/ -v)Notes