Feat/generic files table#63
Merged
Merged
Conversation
Introduce a generic `files` table for immutable binary content storage. This is the first step of extracting inline `xml_content` and `pdf_content` from invoices and inbound_emails into a separate table (ADR 0026). - Migration: create `files` table (id, content, content_type, filename, byte_size, inserted_at) - Schema: `KsefHub.Files.File` with changeset that computes byte_size and validates 10MB max - Context: `KsefHub.Files` with create_file/1, get_file!/1, get_file/1 - Factory: file_factory for test data - Tests: 8 tests covering creation, validation, and retrieval Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add nullable xml_file_id and pdf_file_id to invoices, and pdf_file_id to inbound_emails, all referencing the files table with on_delete: :nilify_all. - Migration with indexes on all FK columns - belongs_to associations on Invoice and InboundEmail schemas - FK columns added to changeset casts with foreign_key_constraints - Tests verifying preload works for both schemas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All invoice creation paths now create file records in the files table alongside inline content (dual-write for backward compat): - create_invoice/1: pops xml_content/pdf_content, creates file records, sets xml_file_id/pdf_file_id, keeps inline content until column drop - do_upsert/2: creates xml_file for KSeF sync, adds xml_file_id to upsert replace fields - build_pdf_upload_attrs/6 and do_create_pdf_failed/5: create pdf_file via the create_invoice pipeline - get_invoice!/get_invoice/get_invoice_with_details: preload file assocs - Helper functions: maybe_create_xml_file/2, maybe_create_pdf_file/2 Tests verify xml_file_id and pdf_file_id are set on creation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- create_inbound_email/2: pops pdf_content, creates file record, sets pdf_file_id while keeping inline content for backward compat - get_inbound_email/1 and get_inbound_email!/1: preload :pdf_file - Test verifies pdf_file_id is set with correct content Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch all content read paths from inline columns to file associations: - invoice_controller.ex: html/2 checks invoice.xml_file instead of invoice.xml_content; xml/2 serves invoice.xml_file.content; serve_pdf/2 matches pdf_file presence for BOTH :pdf_upload and :email sources (fixes bug where email-sourced PDFs returned 422); do_pdf/2 and do_html/3 read from invoice.xml_file.content - invoice_pdf_controller.ex: with_invoice/3 checks invoice.xml_file; xml/2 and generate_and_send_pdf/2 read from invoice.xml_file.content - invoice_live/show.ex: generate_preview/1 reads from invoice.xml_file; download dropdown conditioned on @invoice.xml_file Tests updated to provide xml_file/pdf_file associations on factory inserts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds DataMigration module that migrates existing inline content (xml_content, pdf_content) from invoices and inbound_emails into the files table. Idempotent — skips rows that already have file IDs set. Processes in batches of 100 within transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Removes the inline content columns from invoices and inbound_emails now that all content is stored in the files table. Updates schemas, validations (ksef requires xml_file_id, pdf_upload/email require pdf_file_id), factories, and tests. Removes the @list_fields hack that excluded content from list queries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Oban cron worker (daily at 03:30) that deletes file records not referenced by any invoice or inbound_email and older than 24 hours. The retention period prevents deleting files that are in-flight during a transaction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove eager file preloading from get_invoice!/get_invoice to avoid loading multi-MB blobs on metadata-only operations; content endpoints now use get_invoice_with_details! which preloads file associations - Fix data migration to use limit/offset pagination instead of loading all rows into memory (prevents OOM on large datasets) - Fix create_invoice spec to reflect transaction wrapping (error can be changeset or term) - Add test for email-source PDF download (the serve_pdf bug fix) - Add test for file creation failure when content exceeds 10MB - Add test verifying list_invoices doesn't preload file associations - Add test for upsert creating new file on update (old becomes orphan) - Add FK maintenance note to orphan cleanup worker Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract do_insert_invoice, do_upsert_invoice, do_insert_inbound_email, maybe_create_pdf_file, and migrate_row helpers to keep transaction bodies at max nesting depth of 2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 new immutable Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as InvoiceController
participant Invoices as InvoicesCtx
participant Files as FilesCtx
participant DB as Repo/DB
Client->>Controller: POST /invoices (xml_content or pdf_content)
Controller->>Invoices: create_invoice(attrs)
Invoices->>Files: maybe_create_xml_file(xml_content) / maybe_create_pdf_file(pdf_content)
Files->>DB: INSERT INTO files (content, content_type, filename, byte_size)
DB-->>Files: file_id
Files-->>Invoices: {:ok, file_id}
Invoices->>DB: INSERT/UPSERT invoice with xml_file_id/pdf_file_id
DB-->>Invoices: invoice_record
Invoices-->>Controller: {:ok, invoice_record}
Controller-->>Client: 201 Created / response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Orphaned files are rare (only possible during upsert conflict) and harmless — not worth the complexity of a cron worker. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai perform full review |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
priv/repo/migrations/20260301142418_drop_legacy_content_columns.exs (1)
1-25: Add a comment noting the data migration dependency.This migration is structurally correct and reversible at the schema level. However, running
down/0after data has been migrated will only recreate empty columns — the original content won't be restored. Consider adding a comment to make this explicit for future maintainers.📝 Suggested documentation improvement
defmodule KsefHub.Repo.Migrations.DropLegacyContentColumns do + `@moduledoc` """ + Drops legacy inline content columns after data has been migrated to the files table. + + IMPORTANT: This migration is data-destructive. The `down/0` function recreates the + columns but does NOT restore the original content. Ensure the data migration + (20260301141405_populate_files_from_existing_content) has been verified before running. + """ use Ecto.Migration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260301142418_drop_legacy_content_columns.exs` around lines 1 - 25, Add an explicit comment in the migration module KsefHub.Repo.Migrations.DropLegacyContentColumns (near defmodule or above def up) that documents the data-migration dependency: state that removing :xml_content and :pdf_content from the invoices table and :pdf_content from inbound_emails in def up permanently drops data and that def down only recreates empty columns (no data restoration); mention any external data-migration task or backup that must be run before executing this migration and reference the affected tables (:invoices, :inbound_emails) and columns (:xml_content, :pdf_content) so future maintainers understand the limitation.docs/adr/0026-generic-files-table.md (1)
71-73: Update recommendation section to reflect current implementation status.The recommendation suggests implementing "as a standalone refactor after the inbound email feature is stable," but this PR implements the change now. Consider updating this section to reflect the actual implementation timeline, or remove it since the status already says "Implemented."
📝 Suggested update
### Recommendation -Implement as a standalone refactor after the inbound email feature is stable in production. The current inline storage works — this is a cleanliness improvement, not urgent. +Implemented as part of the inbound email feature rollout. The orphan cleanup worker runs on a scheduled basis to remove unreferenced files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/0026-generic-files-table.md` around lines 71 - 73, Update the "### Recommendation" section (the heading and its paragraph) to reflect that the change is already implemented in this PR: remove or replace the sentence that says "Implement as a standalone refactor after the inbound email feature is stable in production" with a short statement noting that the change has been implemented in this PR (or delete the recommendation paragraph entirely), and ensure the section aligns with the existing "Status: Implemented" wording so the doc is consistent.test/support/factory.ex (1)
127-131: Keepbyte_sizeconsistent with overridden filecontent.
file_factory/0has a fixedbyte_size, butbuild(:file, content: ...)overrides content in invoice factories without updating size. This can create inconsistent test records.🔧 Proposed fix
def invoice_factory do + xml_content = File.read!("test/support/fixtures/sample_income.xml") + %Invoice{ @@ xml_file: build(:file, - content: File.read!("test/support/fixtures/sample_income.xml"), - content_type: "application/xml" + content: xml_content, + content_type: "application/xml", + byte_size: byte_size(xml_content) ), @@ def pdf_upload_invoice_factory do + pdf_content = "%PDF-1.4 fake content" + %Invoice{ @@ pdf_file: build(:file, - content: "%PDF-1.4 fake content", + content: pdf_content, content_type: "application/pdf", - filename: "invoice.pdf" + filename: "invoice.pdf", + byte_size: byte_size(pdf_content) ),Also applies to: 181-186, 250-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/factory.ex` around lines 127 - 131, The factory sets a fixed byte_size in file_factory while some invoice factories (e.g., xml_file in invoice factories that call build(:file, content: File.read!("test/support/fixtures/sample_income.xml"))) override the content, causing size mismatch; update those factories (or file_factory) to compute byte_size from the actual content (for example, set byte_size: byte_size(content) when content is provided) or remove the hardcoded byte_size and derive it dynamically within file_factory so xml_file and other overrides (lines around xml_file and the other occurrences referenced) always produce consistent byte_size values.test/ksef_hub/invoices_test.exs (1)
17-73: Add regression tests for string-keyed content payloads.A couple of cases with
"xml_content"/"pdf_content"(string keys) would guard controller/webhook-style params and prevent silent regressions.Also applies to: 138-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoices_test.exs` around lines 17 - 73, Add tests that pass string-keyed payloads for "xml_content" and "pdf_content" to cover controller/webhook-style params and prevent regressions: modify the tests in invoices_test.exs (e.g., the "creates xml_file for ksef source invoice" and "creates pdf_file for pdf_upload source invoice" cases, and the large-file error case) to include additional assertions where attrs is built with Map.put/Map.merge using string keys ("xml_content" and "pdf_content") instead of atom keys, then call Invoices.create_invoice(attrs) and assert the same outcomes (file created, content and content_type checks, and error for oversize) to ensure Invoices.create_invoice handles string-keyed content payloads correctly.
🤖 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/files/data_migration.ex`:
- Around line 75-87: The insert_all and update_all calls (Repo.insert_all for
"files" and Repo.update_all on table_name with fk_field) ignore affected-row
counts, risking orphan or broken FK; modify the flow to capture the returned
{count, _} from Repo.insert_all and Repo.update_all (or switch to
Ecto.Multi.insert/update) and validate counts: ensure insert_all returns 1
(otherwise raise/rollback and do not run update), and ensure update_all returns
1 (otherwise delete the inserted file or raise/rollback) using file_id and
row.id for verification; fail fast with a descriptive error so the transaction
does not silently succeed.
In `@lib/ksef_hub/invoices.ex`:
- Around line 152-153: create_invoice/1's spec was widened to allow {:error,
term()}, but create_or_retry_duplicate/2 only matches {:error,
%Ecto.Changeset{}} causing a CaseClauseError for other errors; update
create_or_retry_duplicate/2 to handle a generic {:error, reason} fallback (in
addition to the existing {:error, %Ecto.Changeset{}} clause) and return or
propagate {:error, reason} so non-changeset rollback reasons from
create_invoice/1 are handled safely.
In `@lib/ksef_hub/invoices/invoice.ex`:
- Around line 98-100: The cast list in the invoice changeset currently includes
:xml_file_id and :pdf_file_id which allows external callers (e.g.,
update_invoice/2) to rebind file associations; remove :xml_file_id and
:pdf_file_id from the Ecto.Changeset.cast call (leave :original_filename) and
rely on the existing foreign_key_constraint validations for those fields, and
when you create/attach file records set the ids explicitly via
Ecto.Changeset.change/2 (same pattern used for company_id) instead of permitting
them through the public cast.
---
Nitpick comments:
In `@docs/adr/0026-generic-files-table.md`:
- Around line 71-73: Update the "### Recommendation" section (the heading and
its paragraph) to reflect that the change is already implemented in this PR:
remove or replace the sentence that says "Implement as a standalone refactor
after the inbound email feature is stable in production" with a short statement
noting that the change has been implemented in this PR (or delete the
recommendation paragraph entirely), and ensure the section aligns with the
existing "Status: Implemented" wording so the doc is consistent.
In `@priv/repo/migrations/20260301142418_drop_legacy_content_columns.exs`:
- Around line 1-25: Add an explicit comment in the migration module
KsefHub.Repo.Migrations.DropLegacyContentColumns (near defmodule or above def
up) that documents the data-migration dependency: state that removing
:xml_content and :pdf_content from the invoices table and :pdf_content from
inbound_emails in def up permanently drops data and that def down only recreates
empty columns (no data restoration); mention any external data-migration task or
backup that must be run before executing this migration and reference the
affected tables (:invoices, :inbound_emails) and columns (:xml_content,
:pdf_content) so future maintainers understand the limitation.
In `@test/ksef_hub/invoices_test.exs`:
- Around line 17-73: Add tests that pass string-keyed payloads for "xml_content"
and "pdf_content" to cover controller/webhook-style params and prevent
regressions: modify the tests in invoices_test.exs (e.g., the "creates xml_file
for ksef source invoice" and "creates pdf_file for pdf_upload source invoice"
cases, and the large-file error case) to include additional assertions where
attrs is built with Map.put/Map.merge using string keys ("xml_content" and
"pdf_content") instead of atom keys, then call Invoices.create_invoice(attrs)
and assert the same outcomes (file created, content and content_type checks, and
error for oversize) to ensure Invoices.create_invoice handles string-keyed
content payloads correctly.
In `@test/support/factory.ex`:
- Around line 127-131: The factory sets a fixed byte_size in file_factory while
some invoice factories (e.g., xml_file in invoice factories that call
build(:file, content: File.read!("test/support/fixtures/sample_income.xml")))
override the content, causing size mismatch; update those factories (or
file_factory) to compute byte_size from the actual content (for example, set
byte_size: byte_size(content) when content is provided) or remove the hardcoded
byte_size and derive it dynamically within file_factory so xml_file and other
overrides (lines around xml_file and the other occurrences referenced) always
produce consistent byte_size values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
config/config.exsconfig/runtime.exsdocs/adr/0021-future-invoice-documents-table.mddocs/adr/0026-generic-files-table.mdlib/ksef_hub/files.exlib/ksef_hub/files/data_migration.exlib/ksef_hub/files/file.exlib/ksef_hub/files/orphan_cleanup_worker.exlib/ksef_hub/inbound_email.exlib/ksef_hub/inbound_email/inbound_email.exlib/ksef_hub/inbound_email/inbound_email_worker.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/controllers/invoice_pdf_controller.exlib/ksef_hub_web/live/invoice_live/show.expriv/repo/migrations/20260301134650_create_files.exspriv/repo/migrations/20260301135106_add_file_references.exspriv/repo/migrations/20260301141405_populate_files_from_existing_content.exspriv/repo/migrations/20260301142418_drop_legacy_content_columns.exstest/ksef_hub/files/orphan_cleanup_worker_test.exstest/ksef_hub/files_test.exstest/ksef_hub/inbound_email/inbound_email_worker_test.exstest/ksef_hub/inbound_email_test.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/controllers/invoice_pdf_controller_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/support/factory.ex
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
331-342:⚠️ Potential issue | 🟠 MajorDownload menu is hidden for PDF-only invoices.
Line 331 gates the entire dropdown on
@invoice.xml_file, so invoices that only havepdf_file(no XML) won’t show any download action even if PDF is available.💡 Suggested fix
- <div :if={`@invoice.xml_file`} class="dropdown dropdown-end"> + <div :if={`@invoice.xml_file` || `@invoice.pdf_file`} class="dropdown dropdown-end"> @@ - <li><a href={~p"/invoices/#{`@invoice.id`}/pdf"}>PDF</a></li> - <li><a href={~p"/invoices/#{`@invoice.id`}/xml"}>XML</a></li> + <li :if={`@invoice.pdf_file` || `@invoice.xml_file`}> + <a href={~p"/invoices/#{`@invoice.id`}/pdf"}>PDF</a> + </li> + <li :if={`@invoice.xml_file`}> + <a href={~p"/invoices/#{`@invoice.id`}/xml"}>XML</a> + </li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 331 - 342, The dropdown is currently rendered only when `@invoice.xml_file` is present, hiding download actions for PDF-only invoices; change the outer condition to check for either `@invoice.pdf_file` or `@invoice.xml_file` (e.g., use `@invoice.pdf_file` || `@invoice.xml_file`) so the dropdown renders when any download exists, and inside the menu conditionally render the XML link only if `@invoice.xml_file` and the PDF link only if `@invoice.pdf_file` (update the template around the `@invoice.xml_file` gating and the <a href={~p"/invoices/#{`@invoice.id`}/pdf"}> and <a href={~p"/invoices/#{`@invoice.id`}/xml"}> entries accordingly).
🧹 Nitpick comments (3)
priv/repo/migrations/20260301141405_populate_files_from_existing_content.exs (1)
4-6: Consider migration resilience for future schema changes.Calling application code (
KsefHub.Files.DataMigration.run/0) from migrations creates coupling that may break if the module is modified or removed. For production migrations that must remain runnable on fresh deployments, consider:
- Inlining the migration SQL directly, or
- Ensuring
DataMigrationis versioned/immutableThis is acceptable if the module is kept stable, but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260301141405_populate_files_from_existing_content.exs` around lines 4 - 6, The migration currently calls application code KsefHub.Files.DataMigration.run/0 which can break if that module changes; either replace the call with self-contained SQL using Ecto.Migration.execute/1 (inline the queries the DataMigration performs) so the migration is immutable, or move/inline the exact DataMigration.run implementation into this migration file as private functions/constants so the migration does not depend on external module changes; reference KsefHub.Files.DataMigration.run/0 when locating the current call and ensure any data transformations or SQL statements are copied verbatim into the migration to preserve behavior.test/ksef_hub/invoices_test.exs (2)
9-10: Reuse@sample_xmlin the XML file creation test.Line 34 re-reads the same fixture already loaded at Line 9. Using
@sample_xmlthere avoids duplicated file I/O and keeps setup consistent.♻️ Suggested simplification
- test "creates xml_file for ksef source invoice", %{company: company} do - xml = File.read!("test/support/fixtures/sample_income.xml") + test "creates xml_file for ksef source invoice", %{company: company} do + xml = `@sample_xml`Also applies to: 33-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoices_test.exs` around lines 9 - 10, The test duplicates reading the fixture file; instead of calling File.read! again in the XML creation test, reuse the module attribute `@sample_xml` already defined at the top of the file—replace the second File.read!/literal with `@sample_xml` in the test (the test that builds/creates the XML file and currently re-reads the fixture), so all code uses the same fixture data and avoids duplicated file I/O.
23-24: Consider a tiny helper to reduce repeated XML setup.The repeated
|> Map.put(:xml_content,@sample_xml)is consistent but duplicated across many tests; a helper would make future changes easier.♻️ Optional helper extraction
+ defp with_sample_xml(attrs), do: Map.put(attrs, :xml_content, `@sample_xml`)- attrs = - params_for(:invoice, ksef_number: "upsert-1", company_id: company.id) - |> Map.put(:xml_content, `@sample_xml`) + attrs = + params_for(:invoice, ksef_number: "upsert-1", company_id: company.id) + |> with_sample_xml()Also applies to: 75-78, 94-97, 128-131, 139-142, 186-189, 212-215, 913-914, 933-934, 1150-1151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/invoices_test.exs` around lines 23 - 24, Several tests repeatedly call |> Map.put(:xml_content, `@sample_xml`); add a small private helper to DRY this up (e.g., defp with_sample_xml(attrs) or defp add_xml(attrs)) and replace each Map.put usage with that helper; reference the module attribute `@sample_xml` and update usages in test functions that currently call Map.put(:xml_content, `@sample_xml`) so they call the new helper instead, or alternatively modify the factory/fixture function used in those tests to include xml_content: `@sample_xml` by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/0026-generic-files-table.md`:
- Around line 71-73: The Recommendation section currently reads as if the change
should be deferred, which conflicts with the ADR's implemented status; edit the
"Recommendation" heading text so it is written in past tense and framed
explicitly as historical context (e.g., "Recommendation (historical):
Implemented as a standalone refactor after inbound email was stable") and remove
or rephrase any present-tense deferment language so the section documents what
was done rather than suggesting future action; update the paragraph under the
"Recommendation" heading to reflect that the inline storage was a temporary
approach and the refactor was performed after stability, keeping the meaning but
changing verbs to past tense.
In `@lib/ksef_hub/inbound_email/inbound_email.ex`:
- Around line 41-42: Remove :pdf_file_id from the Ecto changeset cast list in
the inbound_email schema so external input cannot set it; this field is assigned
internally in maybe_create_pdf_file (where Files.create_file is called and
Map.put(attrs, :pdf_file_id, file.id) sets it), so leave the
foreign_key_constraint(:pdf_file_id) in place but delete :pdf_file_id from the
cast/permit list to prevent parameter binding from callers.
In `@test/ksef_hub/invoices_test.exs`:
- Around line 157-175: The test "creates new xml_file on update, leaving old one
as orphan" relies on a factory default for original.xml_file_id; make the
precondition explicit by creating or inserting a file and passing its id into
insert(:invoice, xml_file_id: <new_file_id>, ksef_number: "upsert-orphan",
company: company, inserted_at: ... ) so original_file_id is guaranteed, then
keep the assertion assert KsefHub.Files.get_file(original_file_id) as-is; also
replace the outdated comment about OrphanCleanupWorker with a neutral comment
(e.g., "Old file still exists") to avoid referencing removed/changed workers.
---
Outside diff comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 331-342: The dropdown is currently rendered only when
`@invoice.xml_file` is present, hiding download actions for PDF-only invoices;
change the outer condition to check for either `@invoice.pdf_file` or
`@invoice.xml_file` (e.g., use `@invoice.pdf_file` || `@invoice.xml_file`) so the
dropdown renders when any download exists, and inside the menu conditionally
render the XML link only if `@invoice.xml_file` and the PDF link only if
`@invoice.pdf_file` (update the template around the `@invoice.xml_file` gating and
the <a href={~p"/invoices/#{`@invoice.id`}/pdf"}> and <a
href={~p"/invoices/#{`@invoice.id`}/xml"}> entries accordingly).
---
Nitpick comments:
In
`@priv/repo/migrations/20260301141405_populate_files_from_existing_content.exs`:
- Around line 4-6: The migration currently calls application code
KsefHub.Files.DataMigration.run/0 which can break if that module changes; either
replace the call with self-contained SQL using Ecto.Migration.execute/1 (inline
the queries the DataMigration performs) so the migration is immutable, or
move/inline the exact DataMigration.run implementation into this migration file
as private functions/constants so the migration does not depend on external
module changes; reference KsefHub.Files.DataMigration.run/0 when locating the
current call and ensure any data transformations or SQL statements are copied
verbatim into the migration to preserve behavior.
In `@test/ksef_hub/invoices_test.exs`:
- Around line 9-10: The test duplicates reading the fixture file; instead of
calling File.read! again in the XML creation test, reuse the module attribute
`@sample_xml` already defined at the top of the file—replace the second
File.read!/literal with `@sample_xml` in the test (the test that builds/creates
the XML file and currently re-reads the fixture), so all code uses the same
fixture data and avoids duplicated file I/O.
- Around line 23-24: Several tests repeatedly call |> Map.put(:xml_content,
`@sample_xml`); add a small private helper to DRY this up (e.g., defp
with_sample_xml(attrs) or defp add_xml(attrs)) and replace each Map.put usage
with that helper; reference the module attribute `@sample_xml` and update usages
in test functions that currently call Map.put(:xml_content, `@sample_xml`) so they
call the new helper instead, or alternatively modify the factory/fixture
function used in those tests to include xml_content: `@sample_xml` by default.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
docs/adr/0021-future-invoice-documents-table.mddocs/adr/0026-generic-files-table.mdlib/ksef_hub/files.exlib/ksef_hub/files/data_migration.exlib/ksef_hub/files/file.exlib/ksef_hub/inbound_email.exlib/ksef_hub/inbound_email/inbound_email.exlib/ksef_hub/inbound_email/inbound_email_worker.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/controllers/invoice_pdf_controller.exlib/ksef_hub_web/live/invoice_live/show.expriv/repo/migrations/20260301134650_create_files.exspriv/repo/migrations/20260301135106_add_file_references.exspriv/repo/migrations/20260301141405_populate_files_from_existing_content.exspriv/repo/migrations/20260301142418_drop_legacy_content_columns.exstest/ksef_hub/files_test.exstest/ksef_hub/inbound_email/inbound_email_worker_test.exstest/ksef_hub/inbound_email_test.exstest/ksef_hub/invoices_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/controllers/invoice_pdf_controller_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/support/factory.ex
Comment on lines
+157
to
+175
| test "creates new xml_file on update, leaving old one as orphan", %{company: company} do | ||
| original = | ||
| insert(:invoice, | ||
| ksef_number: "upsert-orphan", | ||
| company: company, | ||
| inserted_at: NaiveDateTime.add(NaiveDateTime.utc_now(), -60) | ||
| ) | ||
|
|
||
| original_file_id = original.xml_file_id | ||
|
|
||
| attrs = | ||
| params_for(:invoice, ksef_number: "upsert-orphan", company_id: company.id) | ||
| |> Map.put(:xml_content, @sample_xml) | ||
|
|
||
| {:ok, updated, :updated} = Invoices.upsert_invoice(attrs) | ||
| assert updated.xml_file_id != original_file_id | ||
| # Old file still exists (will be cleaned by OrphanCleanupWorker) | ||
| assert KsefHub.Files.get_file(original_file_id) | ||
| end |
There was a problem hiding this comment.
Make orphan-file test precondition explicit and remove stale cleanup wording.
Line 165 assumes original.xml_file_id exists from factory defaults. If that default changes, the test can fail for the wrong reason. Also, the Line 173 comment references OrphanCleanupWorker, which appears outdated in this PR history.
✅ Safer assertion + neutral comment
original_file_id = original.xml_file_id
+ assert original_file_id
attrs =
params_for(:invoice, ksef_number: "upsert-orphan", company_id: company.id)
|> Map.put(:xml_content, `@sample_xml`)
{:ok, updated, :updated} = Invoices.upsert_invoice(attrs)
assert updated.xml_file_id != original_file_id
- # Old file still exists (will be cleaned by OrphanCleanupWorker)
+ # Old file still exists (cleanup handled by separate maintenance flow)
assert KsefHub.Files.get_file(original_file_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test "creates new xml_file on update, leaving old one as orphan", %{company: company} do | |
| original = | |
| insert(:invoice, | |
| ksef_number: "upsert-orphan", | |
| company: company, | |
| inserted_at: NaiveDateTime.add(NaiveDateTime.utc_now(), -60) | |
| ) | |
| original_file_id = original.xml_file_id | |
| attrs = | |
| params_for(:invoice, ksef_number: "upsert-orphan", company_id: company.id) | |
| |> Map.put(:xml_content, @sample_xml) | |
| {:ok, updated, :updated} = Invoices.upsert_invoice(attrs) | |
| assert updated.xml_file_id != original_file_id | |
| # Old file still exists (will be cleaned by OrphanCleanupWorker) | |
| assert KsefHub.Files.get_file(original_file_id) | |
| end | |
| test "creates new xml_file on update, leaving old one as orphan", %{company: company} do | |
| original = | |
| insert(:invoice, | |
| ksef_number: "upsert-orphan", | |
| company: company, | |
| inserted_at: NaiveDateTime.add(NaiveDateTime.utc_now(), -60) | |
| ) | |
| original_file_id = original.xml_file_id | |
| assert original_file_id | |
| attrs = | |
| params_for(:invoice, ksef_number: "upsert-orphan", company_id: company.id) | |
| |> Map.put(:xml_content, `@sample_xml`) | |
| {:ok, updated, :updated} = Invoices.upsert_invoice(attrs) | |
| assert updated.xml_file_id != original_file_id | |
| # Old file still exists (cleanup handled by separate maintenance flow) | |
| assert KsefHub.Files.get_file(original_file_id) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ksef_hub/invoices_test.exs` around lines 157 - 175, The test "creates
new xml_file on update, leaving old one as orphan" relies on a factory default
for original.xml_file_id; make the precondition explicit by creating or
inserting a file and passing its id into insert(:invoice, xml_file_id:
<new_file_id>, ksef_number: "upsert-orphan", company: company, inserted_at: ...
) so original_file_id is guaranteed, then keep the assertion assert
KsefHub.Files.get_file(original_file_id) as-is; also replace the outdated
comment about OrphanCleanupWorker with a neutral comment (e.g., "Old file still
exists") to avoid referencing removed/changed workers.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Validate row counts in data migration (insert_all/update_all must each affect exactly 1 row, otherwise the match raises and rolls back) - Fix CaseClauseError in create_or_retry_duplicate — handle non-changeset errors from create_invoice (e.g. file creation failure) - Remove xml_file_id/pdf_file_id from Invoice changeset cast — set via Ecto.Changeset.change/2 like company_id to prevent external rebinding - Update ADR 0026 recommendation section to reflect implemented status - Add moduledoc to drop_legacy_content_columns migration documenting data-migration dependency and non-reversibility - Remove hardcoded byte_size from file factory to avoid stale values when content is overridden Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove pdf_file_id from InboundEmail changeset cast — set internally via Ecto.Changeset.change/2 like the invoice file IDs - Show download dropdown for PDF-only invoices (pdf_upload/email source) — previously hidden because it only checked xml_file presence - Conditionally render XML link only when xml_file exists - Replace duplicate File.read! with @sample_xml module attribute in test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/files/data_migration.ex`:
- Around line 61-67: The batch migration currently ignores Repo.transaction/1
results and keeps recursing, so change migrate_batch to capture the transaction
result (pattern match on Repo.transaction(fn -> Enum.each(batch,
&migrate_row(&1, content_type, table_name, fk_field)) end)), and on {:ok, _}
continue with the recursive call but on {:error, reason} immediately return or
raise {:error, reason} (i.e., stop recursion) so a permanently failing row does
not cause an infinite loop; reference migrate_batch, migrate_row and
Repo.transaction when locating where to add the pattern match and error
propagation.
In `@lib/ksef_hub/invoices.ex`:
- Around line 155-157: The code currently pops :pdf_content and :xml_content
from attrs but misses string-keyed versions, so file records are not created
when attrs use string keys; update the handling to normalize or robustly extract
both atom and string keys: read pdf_content = Map.get(attrs, :pdf_content) ||
Map.get(attrs, "pdf_content") (similarly for xml_content, original_filename,
company_id), remove both variants from attrs (e.g., attrs = Map.drop(attrs,
[:pdf_content, "pdf_content", :xml_content, "xml_content", :original_filename,
"original_filename"])), and then use the extracted values when creating file
records and assigning *_file_id fields; apply the same fix to the other
occurrences that read these keys so file creation always sees values regardless
of atom vs string keys.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/adr/0026-generic-files-table.mdlib/ksef_hub/files/data_migration.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice.expriv/repo/migrations/20260301142418_drop_legacy_content_columns.exstest/support/factory.ex
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ksef_hub/invoices/invoice.ex
- docs/adr/0026-generic-files-table.md
Comment on lines
155
to
157
| {pdf_content, attrs} = Map.pop(attrs, :pdf_content) | ||
| {xml_content, attrs} = Map.pop(attrs, :xml_content) | ||
|
|
There was a problem hiding this comment.
Handle mixed atom/string content keys before file creation.
company_id supports both atom and string keys, but xml_content, pdf_content, and original_filename are read only via atom keys. With string-key attrs, file records won’t be created and required *_file_id fields can be missed.
🔧 Proposed fix
- {pdf_content, attrs} = Map.pop(attrs, :pdf_content)
- {xml_content, attrs} = Map.pop(attrs, :xml_content)
+ {pdf_content, attrs} = pop_mixed_key(attrs, :pdf_content)
+ {xml_content, attrs} = pop_mixed_key(attrs, :xml_content)- {xml_content, attrs} = Map.pop(attrs, :xml_content)
+ {xml_content, attrs} = pop_mixed_key(attrs, :xml_content)- filename = attrs[:original_filename]
+ filename = attrs[:original_filename] || attrs["original_filename"]+ `@spec` pop_mixed_key(map(), atom()) :: {term(), map()}
+ defp pop_mixed_key(attrs, key) do
+ case Map.pop(attrs, key) do
+ {nil, attrs2} -> Map.pop(attrs2, Atom.to_string(key))
+ result -> result
+ end
+ endAlso applies to: 213-214, 680-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices.ex` around lines 155 - 157, The code currently pops
:pdf_content and :xml_content from attrs but misses string-keyed versions, so
file records are not created when attrs use string keys; update the handling to
normalize or robustly extract both atom and string keys: read pdf_content =
Map.get(attrs, :pdf_content) || Map.get(attrs, "pdf_content") (similarly for
xml_content, original_filename, company_id), remove both variants from attrs
(e.g., attrs = Map.drop(attrs, [:pdf_content, "pdf_content", :xml_content,
"xml_content", :original_filename, "original_filename"])), and then use the
extracted values when creating file records and assigning *_file_id fields;
apply the same fix to the other occurrences that read these keys so file
creation always sees values regardless of atom vs string keys.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/invoice_live/show.ex`:
- Around line 298-303: The crash happens because handlers like approve_invoice,
reject_invoice, and update_invoice_fields return structs with associations not
preloaded, so code in generate_preview (accessing invoice.xml_file.content) and
the template dropdown checks treat %Ecto.Association.NotLoaded{} as truthy; fix
by either (A) changing guards to use Ecto.assoc_loaded?/1 and nil-checks before
dereferencing (e.g., in generate_preview and the template dropdown logic use
assoc_loaded?(invoice.xml_file) and assoc_loaded?(invoice.pdf_file) && not
is_nil(invoice.xml_file.content)), or (B) mirror set_category/reload_details
behavior and reload the invoice associations after Repo.update (call the
existing reload_details/1 or a helper that preloads [:xml_file, :pdf_file, ...]
in approve_invoice, reject_invoice, and update_invoice_fields) so templates and
generate_preview always receive preloaded associations.
In `@lib/ksef_hub/inbound_email.ex`:
- Around line 57-65: The code sets company_id via
Ecto.Changeset.change(internal) but InboundEmailRecord.changeset/2 still allows
:company_id to be cast from attrs, letting callers override tenancy; remove
:company_id from the public cast list in InboundEmailRecord.changeset/2 (in the
InboundEmailRecord module) so company_id is only set programmatically, and keep
the existing Ecto.Changeset.change(internal) / Repo.insert flow; if any
validations reference attrs for company_id, update them to read the field via
changeset/get_field after change instead of relying on cast.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/ksef_hub/inbound_email.exlib/ksef_hub/inbound_email/inbound_email.exlib/ksef_hub_web/live/invoice_live/show.extest/ksef_hub/invoices_test.exs
- Data migration: assert {:ok, _} on Repo.transaction result to prevent
infinite loop on permanent row failures
- LiveView show: reload_details after approve, reject, and save_edit to
ensure file associations are preloaded (prevents NotLoaded crash in
generate_preview and template dropdown)
- InboundEmail: remove :company_id from changeset cast — set only
programmatically via change/2 to prevent tenancy override
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/invoice_live/show.ex (1)
342-344: Remove redundant PDF<li>condition inside already-guarded dropdown.The parent dropdown (Line 333) already checks
@invoice.xml_file ||@invoice.pdf_file``, so this inner guard is unnecessary noise.♻️ Simplification
- <li :if={`@invoice.xml_file` || `@invoice.pdf_file`}> + <li> <a href={~p"/invoices/#{`@invoice.id`}/pdf"}>PDF</a> </li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/show.ex` around lines 342 - 344, In the render block of InvoiceLive.Show (the LiveView template around the dropdown guarded by `@invoice.xml_file` || `@invoice.pdf_file`), remove the redundant inner condition on the PDF list item — change <li :if={`@invoice.xml_file` || `@invoice.pdf_file`}> to a plain <li> so the PDF link is always rendered when inside the already-guarded dropdown; keep the <a href={~p"/invoices/#{`@invoice.id`}/pdf"}>PDF</a> unchanged.
🤖 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/live/invoice_live/show.ex`:
- Around line 342-344: In the render block of InvoiceLive.Show (the LiveView
template around the dropdown guarded by `@invoice.xml_file` || `@invoice.pdf_file`),
remove the redundant inner condition on the PDF list item — change <li
:if={`@invoice.xml_file` || `@invoice.pdf_file`}> to a plain <li> so the PDF link is
always rendered when inside the already-guarded dropdown; keep the <a
href={~p"/invoices/#{`@invoice.id`}/pdf"}>PDF</a> unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/ksef_hub/files/data_migration.exlib/ksef_hub/inbound_email/inbound_email.exlib/ksef_hub_web/live/invoice_live/show.ex
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ksef_hub/files/data_migration.ex
- lib/ksef_hub/inbound_email/inbound_email.ex
The outer dropdown div already guards on xml_file || pdf_file, so the inner <li> condition was a no-op. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 1, 2026
Merged
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 by CodeRabbit
New Features
Database Changes
Behavioral Changes
Documentation
Tests