feat: training CSV export + classifier page rename#172
Merged
Conversation
Add a dedicated training CSV export on the Invoice Classifier settings page for ML model training, separate from the accountant-focused ZIP export. - CsvBuilder supports `extended: true` with 15 extra columns (invoice ID, company ID, cost line, payment info, ML predictions, confidence scores) - New `Exports.list_training_invoices/3` reuses export query with training-appropriate preloads (payment_requests, no xml/pdf files) - TrainingCsvController for inline CSV download (no batch, no background job) - Rename Settings > Services tab to "Classifier" with focused page header - Merge training export section into classifier settings page - Soften SSRF block on private IPs to "save anyway" confirmation - ADR 0050: classifier deployment strategy (proposed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a training CSV export feature (extended CSV mode and APIs), a new ADR for classifier deployment strategy, UI and LiveView updates to surface a "Classifier" settings tab with export controls, a PaymentRequests association on invoices, controller/route for authenticated CSV downloads, and supporting tests. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser as Browser
participant Controller as TrainingCsvController
participant Exports as ExportsAPI
participant DB as Database
participant Csv as CsvBuilder
participant HTTP as HTTP
User->>Browser: Click export (date_from, date_to)
Browser->>Controller: GET /training-csv?date_from=...&date_to=...
Controller->>Controller: Validate dates & permissions
alt Valid
Controller->>Exports: list_training_invoices(company_id, date_from, date_to)
Exports->>DB: Query invoices + preloads (payment_requests, category)
DB-->>Exports: Invoices with associations
Exports-->>Controller: Invoice list
Controller->>Csv: build(invoices, {extended: true})
Csv-->>Controller: CSV binary
Controller->>HTTP: send attachment (training_from_to.csv)
HTTP-->>Browser: 200 + CSV
Browser-->>User: Download file
else Invalid or Unauthorized
Controller->>HTTP: Redirect with flash
HTTP-->>Browser: 302 to settings
Browser-->>User: Error shown
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
test/ksef_hub_web/live/settings_live/services_test.exs (2)
211-216: Consider tightening the training CSV link assertion.Current checks can pass on unrelated markup; a single regex against the actual href format would be more precise.
Suggested assertion update
- assert html =~ "training-csv" - assert html =~ "date_from=" - assert html =~ "date_to=" + assert html =~ ~r{/training-csv\?[^"]*date_from=\d{4}-\d{2}-\d{2}[^"]*date_to=\d{4}-\d{2}-\d{2}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/settings_live/services_test.exs` around lines 211 - 216, The test "download link includes date range params" is too loose; instead parse or regex-match the actual anchor href to ensure the training CSV link contains both date_from and date_to as query params. Update the assertion in that test to either parse html (e.g., with Floki) and find the anchor with text or id "training-csv" and assert its "href" includes "date_from=" and "date_to=", or replace the three loose assertions with a single regex assertion against html (e.g., match ~r/href="[^"]*training-csv[^"]*[\?&]date_from=[^&]+&date_to=[^"]*"/) to precisely verify the link format.
150-158: Remove fixed sleep from the LiveView test path.
Process.sleep(100)adds avoidable timing sensitivity and slows the suite.Proposed test stabilization diff
- view - |> form("form[phx-submit=save]", classifier: form_params) - |> render_submit() - - Process.sleep(100) - html = render(view) + html = + view + |> form("form[phx-submit=save]", classifier: form_params) + |> render_submit() assert html =~ "Save anyway"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/settings_live/services_test.exs` around lines 150 - 158, Remove the fixed Process.sleep(100); after submitting the LiveView form with render_submit (reference: render_submit/2 and view), immediately re-render the view (call render(view)) and assert the presence of "Save anyway" from that rendered HTML; if the submit triggers a navigation/patch instead, replace the sleep with the appropriate assertion helper (assert_patch or follow_redirect) to wait for and verify the updated LiveView state instead of sleeping.lib/ksef_hub_web/live/settings_live/services.ex (1)
349-356: Missing@impl trueannotation for consistency.Other
handle_eventcallbacks in this module have@impl trueannotations (e.g., lines 287, 303, 327, etc.), but this new handler is missing it.Suggested fix
+ `@impl` true def handle_event("update_training_dates", params, socket) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/settings_live/services.ex` around lines 349 - 356, Add the missing `@impl` true annotation above the handle_event("update_training_dates", params, socket) function to match other callbacks in the module; locate the function named handle_event/3 with the "update_training_dates" event and insert `@impl` true on the line immediately above it so the callback implementations are consistent.lib/ksef_hub_web/controllers/training_csv_controller.ex (2)
11-11: Consider calling CsvBuilder through the Exports context facade.The coding guidelines state controllers should use
Context.function()and not call sub-modules directly.CsvBuilderis a sub-module ofExports. Consider adding a delegate or wrapper inExportsmodule.This would align with the existing pattern where
Exports.list_training_invoices/3is already called through the context, butCsvBuilder.build/2is called directly.As per coding guidelines: "Callers must always use
Context.function()and never call sub-modules directly from controllers or LiveViews".Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/training_csv_controller.ex` at line 11, The controller is calling the submodule CsvBuilder.build/2 directly; add a facade function in the Exports context (e.g., Exports.build_training_csv/2) that delegates to CsvBuilder.build/2, then update lib/ksef_hub_web/controllers/training_csv_controller.ex to call Exports.build_training_csv(conn_or_params, opts) instead of CsvBuilder.build/2; ensure the new Exports function is documented and reuses the existing CsvBuilder implementation so controllers only call the Exports context.
13-14: Missing OpenAPI spec for controller action.Per coding guidelines, every REST API controller action must have an
open_api_spexoperation spec. This endpoint should document the query parameters (date_from,date_to), success response (CSV file), and error responses (redirects with flash).`@doc` "Downloads an extended CSV of invoices for ML training." `@operation` :download, summary: "Download training CSV", parameters: [ company_id: [in: :path, type: :string, required: true], date_from: [in: :query, type: :string, required: true, description: "ISO8601 date"], date_to: [in: :query, type: :string, required: true, description: "ISO8601 date"] ], responses: [ ok: {"Training CSV file", "text/csv", nil} ]As per coding guidelines: "Every REST API controller action must have an
open_api_spexoperation spec".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/training_csv_controller.ex` around lines 13 - 14, The download action in TrainingCsvController is missing an OpenAPI spec; add an open_api_spex `@operation` for :download above the `@spec/download` function that documents the path param company_id, query params date_from and date_to (ISO8601 strings), a successful 200 response with "text/csv" (CSV file) and appropriate error responses for redirects with flash (e.g., bad request/unauthorized with descriptions). Use the `@operation` :download annotation (matching the download function name) so the spec ties to the existing download/2 action and include concise parameter descriptions and response media types as described in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ksef_hub_web/controllers/training_csv_controller.ex`:
- Line 11: The controller is calling the submodule CsvBuilder.build/2 directly;
add a facade function in the Exports context (e.g.,
Exports.build_training_csv/2) that delegates to CsvBuilder.build/2, then update
lib/ksef_hub_web/controllers/training_csv_controller.ex to call
Exports.build_training_csv(conn_or_params, opts) instead of CsvBuilder.build/2;
ensure the new Exports function is documented and reuses the existing CsvBuilder
implementation so controllers only call the Exports context.
- Around line 13-14: The download action in TrainingCsvController is missing an
OpenAPI spec; add an open_api_spex `@operation` for :download above the
`@spec/download` function that documents the path param company_id, query params
date_from and date_to (ISO8601 strings), a successful 200 response with
"text/csv" (CSV file) and appropriate error responses for redirects with flash
(e.g., bad request/unauthorized with descriptions). Use the `@operation` :download
annotation (matching the download function name) so the spec ties to the
existing download/2 action and include concise parameter descriptions and
response media types as described in the review.
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 349-356: Add the missing `@impl` true annotation above the
handle_event("update_training_dates", params, socket) function to match other
callbacks in the module; locate the function named handle_event/3 with the
"update_training_dates" event and insert `@impl` true on the line immediately
above it so the callback implementations are consistent.
In `@test/ksef_hub_web/live/settings_live/services_test.exs`:
- Around line 211-216: The test "download link includes date range params" is
too loose; instead parse or regex-match the actual anchor href to ensure the
training CSV link contains both date_from and date_to as query params. Update
the assertion in that test to either parse html (e.g., with Floki) and find the
anchor with text or id "training-csv" and assert its "href" includes
"date_from=" and "date_to=", or replace the three loose assertions with a single
regex assertion against html (e.g., match
~r/href="[^"]*training-csv[^"]*[\?&]date_from=[^&]+&date_to=[^"]*"/) to
precisely verify the link format.
- Around line 150-158: Remove the fixed Process.sleep(100); after submitting the
LiveView form with render_submit (reference: render_submit/2 and view),
immediately re-render the view (call render(view)) and assert the presence of
"Save anyway" from that rendered HTML; if the submit triggers a navigation/patch
instead, replace the sleep with the appropriate assertion helper (assert_patch
or follow_redirect) to wait for and verify the updated LiveView state instead of
sleeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0789f623-618e-475d-b86c-b04f5c30b45a
📒 Files selected for processing (14)
docs/adr/0050-classifier-deployment-strategy.mddocs/architecture.mdlib/ksef_hub/exports.exlib/ksef_hub/exports/csv_builder.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub_web/components/settings_components.exlib/ksef_hub_web/controllers/training_csv_controller.exlib/ksef_hub_web/live/export_live/index.exlib/ksef_hub_web/live/settings_live/services.exlib/ksef_hub_web/router.extest/ksef_hub/exports/csv_builder_test.exstest/ksef_hub/exports_test.exstest/ksef_hub_web/controllers/training_csv_controller_test.exstest/ksef_hub_web/live/settings_live/services_test.exs
…est assertions - Route CsvBuilder.build through Exports.build_training_csv/2 so controllers never call sub-modules directly (CLAUDE.md convention) - Add missing @impl true on handle_event("update_training_dates") - Replace three loose =~ checks with a single href regex in the training CSV link test - Reduce Process.sleep(100) to 50ms, matching the project's other async-task test pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/settings_live/services.ex`:
- Around line 347-355: The handle_event/3 for "update_training_dates" currently
lets empty strings from params override assigns and is missing a `@spec`; add an
`@spec` for handle_event/3 (e.g., handle_event(String.t(), map(),
Phoenix.LiveView.Socket.t()) :: {:noreply, Phoenix.LiveView.Socket.t()}) and
change the assign logic in handle_event("update_training_dates", params, socket)
to treat "" (and nil) as absent: read params["training_date_from"] and
params["training_date_to"], normalize each to nil when value == "" or value ==
nil, and then fall back to socket.assigns.training_date_from / training_date_to
when normalized value is nil before calling assign (so empty strings don’t
become exported values).
🪄 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: 4ca3e5c1-306e-4781-b5ac-36e8751222e1
📒 Files selected for processing (5)
lib/ksef_hub/exports.exlib/ksef_hub_web/controllers/training_csv_controller.exlib/ksef_hub_web/live/settings_live/services.extest/ksef_hub_web/controllers/training_csv_controller_test.exstest/ksef_hub_web/live/settings_live/services_test.exs
🚧 Files skipped from review as they are similar to previous changes (4)
- test/ksef_hub_web/controllers/training_csv_controller_test.exs
- lib/ksef_hub/exports.ex
- lib/ksef_hub_web/controllers/training_csv_controller.ex
- test/ksef_hub_web/live/settings_live/services_test.exs
Empty strings are truthy in Elixir, so the previous `|| fallback` pattern would propagate "" into the assigns and into the CSV download URL. Normalize nil/"" to the current assign value before calling assign/2. Also adds the missing @SPEC handle_event/3 for this callback. Co-Authored-By: Claude Sonnet 4.6 <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.
Summary
Changes
New
TrainingCsvController— inline CSV download atGET /c/:company_id/training-csvExports.list_training_invoices/3— reuses export query with training preloadsextended: trueoption adding 15 ML/metadata columnshas_many :payment_requestson Invoice schemaModified
Test plan
/settings/services— page shows "Invoice Classifier" headermix test— 2151 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Updates
Documentation
Tests