fix(viper): align integrationUpload payload with Viper OpenAPI (#160)#169
Merged
Conversation
ViperAsset now emits the keys Viper actually requires:
- ip from asset.ip_address (was missing)
- upstream_api from BASE_URL + asset.id (was "")
- role from asset.category (was "")
- vendor_id snake_case (was vendorID)
- utilization projected from Usage rows (was missing)
- status "Active" (was "active")
- location {facility, building, floor, room} (was {})
- BlueFlow PK id dropped (extras rejected by strict schema)
- cpe/role always present as "" (no longer pruned)
ViperWebhookResponse aligns wrapper keys:
- total -> total_count
- next_page/previous_page -> next/previous
- since, before, request_id, webhook_path stripped from wire
payload (still on the dataclass for URL generation)
Adds blueflow/tests/test_viper_models.py with 21 unit tests
covering the dataclass to_dict() outputs end-to-end.
Pytest declares fixtures via the test function's argument list; the body never reads them by name (their value is the side effect, not the return). Ruff's "unused argument" check is a false positive in test contexts and was forcing per-line noqa pragmas. Centralize the exemption in pyproject.toml and let ruff strip the now-redundant noqas. ruff-format also reflowed two pre-existing call sites in test_upsert.py.
📝 WalkthroughWalkthroughRestructures Viper dataclasses and webhook serialization (utilization projection, JSON-safe fields, status casing), renames pagination/wrapper keys to Changes Viper webhook schema alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blueflow/models/viper.py (1)
197-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefetch
usagerows before buildingViperAssetitems.
ViperAsset.__init__calls_project_usage(asset), which touchesasset.usage.all()per asset. Without prefetching, large pages incur N+1 DB queries.Proposed fix
- assets = assets.order_by("modified").all() + assets = assets.prefetch_related("usage").order_by("modified").all()🤖 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 `@blueflow/models/viper.py` around lines 197 - 208, The loop causes N+1 queries because ViperAsset.__init__ calls _project_usage which accesses asset.usage.all(); fix by prefetching those related rows on the queryset before materializing/chunking it (e.g. call prefetch_related('usage') or a Prefetch for the usage relation on the Asset queryset referenced in the assets variable) so the usage rows are loaded in bulk prior to constructing ViperAsset instances.
🤖 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 `@blueflow/tests/test_csv_byte_order_mark.py`:
- Line 47: The tests referencing the missing fixture setup_db (e.g.,
test_process_csv_without_bom) fail because setup_db doesn't exist; replace the
fixture usage with pytest-django's built-in db fixture or annotate the module
with `@pytest.mark.django_db` so the tests can access the database; update calls
to use db instead of setup_db (or add the module-level marker) for both tests
that currently pass setup_db.
In `@blueflow/tests/test_csv_custom_field.py`:
- Line 12: Replace the unavailable fixture usage in the test function
test_process_csv_with_custom_field (and the other tests referencing setup_db) by
using the available Django DB fixture: change the parameter setup_db to db or
decorate the test with `@pytest.mark.django_db` so the ORM is accessible; update
the function signature(s) (e.g., def test_process_csv_with_custom_field(db):)
and remove references to setup_db within those tests to ensure CI uses the
provided db fixture.
---
Outside diff comments:
In `@blueflow/models/viper.py`:
- Around line 197-208: The loop causes N+1 queries because ViperAsset.__init__
calls _project_usage which accesses asset.usage.all(); fix by prefetching those
related rows on the queryset before materializing/chunking it (e.g. call
prefetch_related('usage') or a Prefetch for the usage relation on the Asset
queryset referenced in the assets variable) so the usage rows are loaded in bulk
prior to constructing ViperAsset instances.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33f354d6-c62f-4fa8-a5be-a7a0c9528177
📒 Files selected for processing (8)
blueflow/celery/tests/test_viper.pyblueflow/models/viper.pyblueflow/tests/test_asset_hostname.pyblueflow/tests/test_csv_byte_order_mark.pyblueflow/tests/test_csv_custom_field.pyblueflow/tests/test_upsert.pyblueflow/tests/test_viper_models.pypyproject.toml
The autouse `_auto_db` fixture in the root conftest.py already enables database access for every test, so declaring `db` as a function argument is dead weight. Removes the arg from 10 test functions across 4 files. Pre-change and post-change runs of the touched files report the same 26 baseline failures and 3 baseline lints — no new errors introduced.
Collaborator
Author
|
xnorkl
requested changes
May 21, 2026
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 `@tests/test_models.py`:
- Around line 4-7: The tests contain function-local imports (e.g., "from
blueflow.models import Asset" inside test_asset_model and similar import(s) in
the other test around lines 12-15) which trigger PLC0415; move these imports to
module scope at the top of tests/test_models.py so both test_asset_model and the
other test(s) import their dependencies from blueflow.models (e.g., Asset and
any other model names referenced) once at file scope rather than inside the test
functions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfc02ae6-88bc-4bc7-8c88-dcfaf14626e9
📒 Files selected for processing (5)
blueflow/models/viper.pyblueflow/tests/test_asset.pyblueflow/tests/test_asset_hostname.pyblueflow/tests/test_upsert.pytests/test_models.py
Comment on lines
+4
to
7
| def test_asset_model(): | ||
| """Smoke test: Asset can be created.""" | ||
| from blueflow.models import Asset | ||
|
|
There was a problem hiding this comment.
Move function-local imports to module scope to fix PLC0415.
Line 6 and Line 14 violate the active lint rule (PLC0415), and CI is already failing on this file.
💡 Proposed fix
"""Tests for blueflow models (mirrors blueflow/models)."""
+from blueflow.models import Asset, Tag
+
def test_asset_model():
"""Smoke test: Asset can be created."""
- from blueflow.models import Asset
asset = Asset.objects.create()
assert asset.pk is not None
assert Asset.objects.filter(pk=asset.pk).exists()
def test_tag_model():
"""Smoke test: Tag model exists and has expected attributes."""
- from blueflow.models import Tag
assert hasattr(Tag, "objects")Also applies to: 12-15
🧰 Tools
🪛 GitHub Actions: Lint / lint
[error] 6-6: PLC0415 import should be at the top-level of a file
🤖 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_models.py` around lines 4 - 7, The tests contain function-local
imports (e.g., "from blueflow.models import Asset" inside test_asset_model and
similar import(s) in the other test around lines 12-15) which trigger PLC0415;
move these imports to module scope at the top of tests/test_models.py so both
test_asset_model and the other test(s) import their dependencies from
blueflow.models (e.g., Asset and any other model names referenced) once at file
scope rather than inside the test functions.
xnorkl
approved these changes
May 21, 2026
Contributor
xnorkl
left a comment
xnorkl
left a comment
There was a problem hiding this comment.
Even better solution than the proposed.
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
2 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.
Summary
Fixes #160. Aligns BlueFlow's outbound
integrationUploadpayload with Viper's actual OpenAPI schema so pages stop coming back as400.ViperAssetip(fromasset.ip_address) andutilization(projected fromUsagerows in the Monday-first sparse{hour: count}shape).upstream_apifromBASE_URL + asset.id(was"");rolefromasset.category(was"")."active"→"Active"(Viper enum isActive | Decommissioned | Maintenance).locationfrom{}to{facility, building, floor, room}with empty-string defaults (not nullable per Viper docs).vendorID→vendor_id.idfrom the payload (strict schema rejects extras).cpe/role— both ship as""when unset.ViperWebhookResponsetotal→total_count,next_page/previous_pageproperties →next/previous.since,before,request_id,webhook_path) are kept on the dataclass for URL generation but stripped fromto_dict()output via a new_INTERNAL_KEYSClassVar.Tests
blueflow/tests/test_viper_models.py— 21 unit tests onViperAsset.to_dict()andViperWebhookResponse.to_dict()covering required keys, dropped extras, status enum, utilization projection, role-from-category mapping, upstream URL pattern, IP fallback, location shape, and all wrapper renames.blueflow/celery/tests/test_viper.pyassertions to reflect the new wire shape (total_count,next,previous, internal fields stripped).Drive-by lint/cleanup commits
chore(lint): exempt ARG001 in test files— pytest fixtures are declared via the test function's argument list and their value is the side effect, not the return; ruff's "unused argument" rule is a false positive in test contexts. Centralizes the exemption inpyproject.tomland removes 6 now-redundant# noqa: ARG001pragmas across 4 unrelated test files. ruff-format also reflowed two pre-existing call sites intest_upsert.py.chore(tests): drop redundant db arg from test functions— the autouse_auto_dbfixture in the rootconftest.pyalready enables DB access for every test, so declaringdbas a function argument is dead weight. Removes the arg from 10 test functions across 4 files. Verified by stash-and-rerun: pre-change and post-change runs of the touched files report the same 26 baseline failures and 3 baseline lints (no new errors introduced).Decisions worth flagging
max_pages=1 × page_size=500ceiling — Viper's inbound webhook currently sends those values, so any hospital with >500 assets will silently lose the tail. Out of scope here, but worth a follow-up issue or a Viper-side bump.statusmapping — hardcoded"Active". BlueFlow has no status concept today (noAsset.statusfield, no tag convention). When one lands, swap the literal for a mapping; not building a one-branch helper now.locationshape — empty strings on all four sub-keys until a BlueFlow source (custom fields? tags?) is decided.pageSize,totalCount, etc.) — not in this PR. The plan is that snake_case will be handled at serialize/requests-lib time. The casing converter needs to be scoped to the outbound view only — applying it globally would silently break the inbound webhook parser, which uses snake_case (max_pages,page_size).Test plan
uv run pytest blueflow/tests/test_viper_models.py blueflow/celery/tests/test_viper.py blueflow/views/tests/test_viper.py blueflow/tests/test_asset_usage.py→ 43/43 green on a fresh DBdbsweep — same 26 baseline failures and 3 baseline lints before/after; no regressionsjust boot && just capture && just integrateagainstdemo-0.3.1with theinit/blueflow-patches/tasks.pyoverlay unmounted — no400from Viper,just check vipermatchesjust check blueflowdemo-0.3.2Closes #160.
Summary by CodeRabbit
Bug Fixes
cpe,role) are now always present with proper defaults.New Features
total_count,next,previous).Tests