Feature/refactor all - #53
Open
sadrultoaha wants to merge 18 commits into
Open
Conversation
`make quality` was passing on a stale .ruff_cache verdict: running ruff with --no-cache surfaced 103 latent violations already present in HEAD (98 unsorted imports, 2 unused imports, 2 quoted annotations) plus formatting drift in 13 files. All auto-fixed with ruff format + ruff check --fix; no behavioral changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Real bugs fixed: - edit_product crashed with NameError (undefined `formats`) when a GAM product was saved with an explicit line item type; now passes product.format_ids to generate_default_config - product suggestions computed already_exists against the first *character* of each product ID (scalars() rows are plain strings, `product[0]` sliced them); now compares full IDs - scheduled delivery webhooks computed max+1 sequence_number then dropped it: payloads never carried the AdCP sequence_number field and WebhookDeliveryLog rows always recorded 1; now assigned to the response alongside the other webhook metadata fields Dead code removed: - /api/gam/test-connection and /api/gam/get-advertisers routes: unreferenced legacy duplicates of the tenant-scoped routes in gam.py/principals.py; the former crashed with NameError (undefined oauth_client_id/tenant_id) swallowed by a catch-all except - ~25 write-only variables, a try/except that only re-raised, a redundant GAMAuthManager construction, unused imports, ambiguous `l` loop names - test_auth Test 3 mocked the legacy session.query path that the SQLAlchemy 2.0 production code never calls (and referenced an undefined mock); rewired to the scalars seam, assertion unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The blanket ignores documented counts that no longer exist (the "12 bare excepts" and "4 F821 false positives" were already fixed; the remaining F841/E741 sites were removed in the previous commit). Fix the last six violations in scripts/ and promote all four rules to enforced so regressions fail the build. tests/ keeps a scoped per-file exemption for F841/E741 (154 pre-existing arrange-only bindings) to avoid churning guarded test files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ruff --fix for RET (else-after-return family), SIM (needless bool, same-arm ifs, yoda conditions), PIE790 (unnecessary placeholders), PERF and C4 comprehension rules: 298 sites, safe fixes only — no unsafe fixes applied, no behavior changes. Enforce RET501/505/506/ 507/508 and PIE790 in the lint gate now that src/ and scripts/ are at zero (tests keep pre-existing occurrences via per-file-ignores). Also convert two stray prints to logging: a [NAMING DEBUG] print in the mock adapter's media-buy path and an exception print in default_products; deliberate console banners (config validation, database provisioning, health report CLI) are left as prints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three date.today() call sites (GAM forecast window, GAM order projection, update-media-buy default) depended on the server's local timezone; use datetime.now(UTC).date() so behavior is identical across deployments. In strategy time-jump handling, current_time is initialized timezone-aware but the jump assigned a naive strptime result, so a later aware/naive comparison could raise TypeError — the parsed target date is now UTC-aware. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The four sync endpoints passed the LazyScopedSession proxy where GAMOrdersService is annotated to take a sqlalchemy Session; calling the proxy returns the underlying thread-local Session (the same object the surrounding code commits through), which satisfies the annotation without behavior change. Clears the 4 mypy errors that had been masked by the incremental cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9 of the 20 unit test files that failed collection (all pre-existing: the same failures reproduce at the branch base commit in a clean worktree) are repaired with verified equivalents; assertions are unchanged: - src.app was deleted in "Sales Agent 2.0" (7fbd30e): the four REST/transport files now build the app via core.main.build_app(). NOTE: build_app() fail-fasts on the DB health check, so these four still need a DB-less bootstrap mode or a move to tests/integration. - adcp 6.4: FormatId moved to adcp.types; Assets classes renumbered (Assets5->Assets25 video, Assets9->Assets29 html, Assets18->Assets39 group, Assets19->Assets41, Assets22->Assets44) — mappings verified against the installed package. - MediaBuyStatus.pending_activation no longer exists; the faithful wire-enum substitute for "accepted, awaiting start" is pending_start. - test_delivery.py: dropped imports of the deleted flat-param MCP/A2A wrappers; the two wrapper-only tests are removed with a pointer to the delegate/harness transport coverage; the 11 _impl-based test classes are unchanged. - test_delivery_date_window.py: kept the _normalize_reporting_window half; removed the half testing _clamp_target_date_to_now, which was deleted with the GAM freshness validator call site. Duplication baseline refresh: 19 fingerprints surfaced by formatting normalization and recent merges (both halves of each pair predate this work); accepted via the hook's --update-baseline mechanism. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…_any
Executes step 1 of mypy.ini's documented strictness roadmap ("bugs
hide in Any returns"). Every function returning Any where a concrete
type is declared now uses a typed intermediate or cast — no behavior
changes, no type: ignore, no annotations loosened. Verified with a
cold (--no-incremental) mypy run over all of src/ and an unchanged
unit-suite result (307/5962/24, byte-identical totals).
One annotation was genuinely wrong and is corrected:
json_type.process_bind_param deliberately passes BaseModel through
to the engine's pydantic serializer but declared it could not.
Latent bugs noted for follow-up (not changed here): broadstreet
delete_* endpoints can return None where dict is declared (empty
204 bodies); broadstreet create_placement skips the response-envelope
unwrap its siblings do; mock adapter _is_simulation returns None
instead of False; `from adcp import WholesaleFeedEvent` types as Any
despite the adcp mypy plugin (upstream re-export gap).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
format_activity_from_audit_log (cyclomatic complexity 30) is now a small orchestrator over focused helpers: _classify_activity, _parse_audit_details, _build_detail_sections, _quoted_excerpt. Behavior verified byte-identical across 90 golden input/output cases (every operation type x timestamp bucket, naive and aware). The extraction exposed the same relative-time bucketing copied in three modules; all three now delegate to core.utils.time_format.relative_time_ago. One deliberate improvement: activity_feed previously returned "Unknown" for naive datetimes (a TypeError swallowed by its catch-all); the shared helper treats naive as UTC like the other two copies always did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
delete_campaign/delete_advertisement/delete_zone declared dict returns
but DELETE endpoints answer with empty 204 bodies, so callers doing
.get() on the result would crash on None — normalize to {}.
create_placement now unwraps the response envelope like every sibling
create_* method. Mock adapter _is_simulation returned None (falsy
object) instead of False when no strategy context is set.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three complexity hotspots become small orchestrators over focused module-level helpers, behavior locked before/after: - get_principal_from_context (C=41 -> under 20): golden-mastered with 28 synthetic inputs covering every branch, external lookups patched at their real import sites, and the chronological dependency call log asserted — outputs, exceptions, and lookup ORDER byte-identical. - parse_form_data_to_policy_updates (C=43) and parse_pricing_options_from_form (C=31): golden-locked over branch-covering synthetic form inputs — both diffs empty. Full unit suite unchanged: 307/5962/24 with a per-test set-diff of zero new failures. Duplication ratchet tightened 18 -> 17 blocks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Dashboard revenue trend issued one DB query per day (30 per dashboard load, 365 for YTD). MediaBuyRepository gains list_in_flight_between(); the trend fetches the window once and buckets per day in Python with identical per-buy attribution math. list_in_flight_on_date delegates to the windowed query. - Product-suggestions API rebuilt the default-products catalog inside the sort key and again per suggestion (O(n^2)); the default-ID set is now computed once per request. - src/services/ai_parsing_comparison.py (unimported CLI tool, never part of the server) moves to scripts/ops/ where it is runnable without shipping in the service import tree. Unit suite: totals unchanged (307/5962/24), per-test set-diff zero new failures; mypy/ruff/duplication gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The details page called adapter.get_media_buy_delivery() inline — a live GAM Reporting API round trip blocking every page visit. The page now renders instantly from the persisted delivery snapshot (delivered_impressions/delivered_amount/delivery_synced_at), and a new GET /media-buy/<id>/delivery-metrics JSON endpoint (api_mode auth, so expired sessions get JSON not HTML) performs the reporting fetch after page load, refreshes the snapshot, and the template updates the metrics in place — with an explicit snapshot/live/unavailable status line. Package IDs from the adapter are inserted via textContent, never as HTML. Route-conflict hook clean; unit totals unchanged (307/5962/24, zero new failures). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Docker image bakes ENV ADCP_PORT=8000 for the run_all_services
entrypoint; the compose test path launches run_server.py with
ADCP_SALES_PORT=8080 and healthchecks 8080. setdefault("ADCP_PORT")
silently lost to the baked ENV, so the unified server bound 8000
while nginx proxied to 8080 — the stack could never become healthy
and ./run_all_tests.sh aborted before any suite ran. Assign instead
of setdefault; verified by a full stack run reaching healthy with
zero nginx upstream errors and the admin (128 passed) and ui
(7 passed) suites completing through the proxy.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
No description provided.