Feat/manual invoice creation#47
Merged
Merged
Conversation
Add migration for source, duplicate_of_id, duplicate_status columns. Make xml_content nullable. Replace unique index with partial index that only enforces uniqueness for non-duplicate invoices. Update Invoice schema with new fields, belongs_to association, source-conditional validation, and duplicate changeset. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add create_manual_invoice/2 that forces source: "manual" and auto-detects duplicates when ksef_number matches an existing invoice. Add confirm_duplicate/1 and dismiss_duplicate/1 for managing duplicate status. Add source filter to apply_filters/2. Update get_invoice_by_ksef_number to exclude duplicates. Update upsert to use partial index. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add create action for manual invoice creation with OpenAPI spec. Add confirm-duplicate and dismiss-duplicate POST endpoints. Add source filter param to index operation. Handle nil xml_content in xml/html/pdf endpoints (return 422). Update invoice JSON response with source, duplicate_of_id, duplicate_status. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update factory with source field and manual_invoice_factory. Add context tests for create_manual_invoice, confirm/dismiss_duplicate, source filter, and source validation. Add controller tests for create, confirm-duplicate, dismiss-duplicate, nil xml_content handling, and source filter endpoints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents the design for manual invoice creation with duplicate detection, partial unique index strategy, and duplicate review workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add :source filter to list_invoices @doc. Add 422 response to xml/html/pdf OpenAPI operation specs for invoices without XML content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
confirm_duplicate now only works on "suspected" status.
dismiss_duplicate works on "suspected" or "confirmed" status.
Both return {:error, :invalid_status} for invalid transitions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When concurrent requests both miss the duplicate check but one fails on the unique constraint, retry by looking up the original and marking the new invoice as a suspected duplicate. Extract find_original_id/2 for reuse in both detect_duplicate and the retry path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add :source to upsert_replace_fields so KSeF sync overwrites source: "manual" -> "ksef" when the same ksef_number is synced. Add has_many :duplicates inverse association to Invoice schema for listing all duplicates of a given invoice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, and nil xml_content - Fix upsert override test (backdate inserted_at for reliable insert/update detection) - Add context tests for confirm/dismiss invalid status transitions - Add context test for income type manual invoice - Add controller tests for html/pdf with nil xml_content (422) - Add controller tests for confirm/dismiss invalid status transitions 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 API support for manual invoice creation with source tracking, non‑destructive duplicate detection by Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as InvoiceController
participant Context as InvoicesContext
participant Detector as DuplicateDetector
participant DB as Database
Client->>Controller: POST /api/invoices (manual)
Controller->>Context: create_manual_invoice(company_id, attrs)
Context->>Detector: detect_duplicate(company_id, ksef_number)
Detector->>DB: SELECT invoice WHERE company_id=... AND ksef_number=... AND duplicate_of_id IS NULL
DB-->>Detector: existing_invoice or nil
alt Duplicate Detected
Detector-->>Context: {true, original_id}
Context->>DB: INSERT invoice (source="manual", duplicate_of_id=original_id, duplicate_status="suspected")
DB-->>Context: created_invoice
Context-->>Controller: {:ok, created_invoice}
Controller-->>Client: 201 Created (includes duplicate_of_id, duplicate_status)
else No Duplicate
Detector-->>Context: {false, nil}
Context->>DB: INSERT invoice (source="manual")
DB-->>Context: created_invoice
Context-->>Controller: {:ok, created_invoice}
Controller-->>Client: 201 Created
end
sequenceDiagram
participant Client
participant Controller as InvoiceController
participant Context as InvoicesContext
participant DB as Database
Client->>Controller: POST /api/invoices/:id/confirm-duplicate
Controller->>DB: GET invoice by id (scoped to company)
DB-->>Controller: invoice (with duplicate_of_id, duplicate_status)
Controller->>Context: confirm_duplicate(invoice)
Context->>DB: validate duplicate_of_id present and duplicate_status == "suspected"
alt Valid
Context->>DB: UPDATE invoice SET duplicate_status="confirmed"
DB-->>Context: updated_invoice
Context-->>Controller: {:ok, updated_invoice}
Controller-->>Client: 200 OK
else Invalid
Context-->>Controller: {:error, :not_a_duplicate} or {:error, :invalid_status}
Controller-->>Client: 422 Unprocessable Entity
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (6)
lib/ksef_hub/invoices.ex (2)
177-180: Unsafe fragment for conflict_target is necessary but worth documenting.The
{:unsafe_fragment, ...}approach is required here because Ecto'sconflict_targetdoesn't natively support partial unique indexes with WHERE clauses. This works correctly but introduces a SQL injection surface if the fragment contents were ever dynamically constructed.Since the fragment is hardcoded, this is safe, but consider adding an inline comment explaining why
unsafe_fragmentis needed.📝 Optional: Add explanatory comment
|> Repo.insert( on_conflict: {:replace, `@upsert_replace_fields`}, + # unsafe_fragment required for partial unique index with WHERE clause conflict_target: {:unsafe_fragment, ~s|("company_id","ksef_number") WHERE ksef_number IS NOT NULL AND duplicate_of_id IS NULL|}, returning: true )🤖 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 177 - 180, Add a brief inline comment immediately above the conflict_target tuple that explains why {:unsafe_fragment, ~s|("company_id","ksef_number") WHERE ksef_number IS NOT NULL AND duplicate_of_id IS NULL|} is used (Ecto does not support partial unique index WHERE clauses in conflict_target), state that the fragment is hardcoded so it is safe from SQL injection, and note that it must remain static if altered; reference the conflict_target tuple in the invoices module so future maintainers understand the rationale.
325-336: Consider explicitelseclause for clarity.The
ifblock implicitly returnsnilwhenksef_numberis falsy or empty. While this works correctly, an explicitelse nilwould make the intent clearer to readers.♻️ Optional: Add explicit else clause
defp find_original_id(company_id, attrs) do ksef_number = attrs[:ksef_number] || attrs["ksef_number"] if ksef_number && ksef_number != "" do Invoice |> where([i], i.company_id == ^company_id and i.ksef_number == ^ksef_number) |> where([i], is_nil(i.duplicate_of_id)) |> select([i], i.id) |> Repo.one() + else + nil end end🤖 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 325 - 336, The function find_original_id currently relies on an implicit nil return when ksef_number is falsy; make this explicit by adding an else clause that returns nil so intent is clearer to readers. Locate the private function find_original_id(company_id, attrs) and modify the if ksef_number && ksef_number != "" block to include an else nil branch, leaving the existing query (Invoice |> where ... |> Repo.one()) unchanged.test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
269-299: Consider adding test for dismissing a confirmed duplicate.The
dismiss_duplicatetests cover dismissing asuspectedduplicate (success) and dismissing an alreadydismissedduplicate (error). However, per the domain logic inlib/ksef_hub/invoices.ex(lines 286-291), dismissing aconfirmedduplicate is also a valid transition that should succeed. Adding a test for this case would improve coverage.💚 Suggested test to add
test "dismisses a confirmed duplicate invoice", %{conn: conn} do %{company: company, token: token} = create_owner_with_token() original = insert(:invoice, ksef_number: "dismiss-confirmed", company: company) duplicate = insert(:manual_invoice, ksef_number: "dismiss-confirmed", company: company, duplicate_of_id: original.id, duplicate_status: "confirmed" ) conn = conn |> api_conn(token) |> post("/api/invoices/#{duplicate.id}/dismiss-duplicate") assert conn.status == 200 assert Jason.decode!(conn.resp_body)["data"]["duplicate_status"] == "dismissed" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 269 - 299, Add a new test in the describe "dismiss_duplicate" block that mirrors the existing "dismisses a suspected duplicate invoice" test but creates the duplicate invoice with duplicate_status: "confirmed" (use create_owner_with_token(), insert(:invoice) for original and insert(:manual_invoice, duplicate_of_id: original.id, duplicate_status: "confirmed") for the duplicate), POSTs to post("/api/invoices/#{duplicate.id}/dismiss-duplicate") and asserts conn.status == 200 and Jason.decode!(conn.resp_body)["data"]["duplicate_status"] == "dismissed"; place it alongside the other tests so it validates that confirmed -> dismissed is accepted by the dismiss_duplicate endpoint.lib/ksef_hub_web/controllers/api/invoice_controller.ex (3)
249-272: Missing@docand@specfor publicconfirm_duplicate/2action.Per coding guidelines, every public function must have
@docdocumentation and@spectype specification.📝 Proposed fix to add documentation and type spec
+ `@doc` """ + Confirm a suspected duplicate invoice. + + Marks the invoice as a confirmed duplicate of the referenced original. + Only invoices with duplicate_status "suspected" can be confirmed. + """ + `@spec` confirm_duplicate(Plug.Conn.t(), map()) :: Plug.Conn.t() def confirm_duplicate(conn, %{"id" => id}) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 249 - 272, Add a `@doc` and `@spec` for the public controller action confirm_duplicate/2: document its purpose, expected params (conn and a map with "id"), possible responses, and side effects; add a typespec like `@spec` confirm_duplicate(Plug.Conn.t(), map()) :: Plug.Conn.t() (or more specific types if available) placed immediately above the def confirm_duplicate clause in InvoiceController so the function is documented and has a type specification.
116-130: Missing@docand@specfor publiccreate/2action.Per coding guidelines, every public function must have
@docdocumentation and@spectype specification.📝 Proposed fix to add documentation and type spec
+ `@doc` """ + Create a manual invoice. + + Creates an invoice with source "manual" for the company associated with the + authenticated API token. If a ksef_number is provided and matches an existing + invoice, the new invoice is flagged as a suspected duplicate. + """ + `@spec` create(Plug.Conn.t(), map()) :: Plug.Conn.t() def create(conn, params) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 116 - 130, The public controller action create/2 is missing `@doc` and `@spec`; add a short `@doc` describing the endpoint behavior and expected params/response, and add a `@spec` for create(conn :: Plug.Conn.t(), params :: map()) :: Plug.Conn.t(); place these annotations immediately above the def create(conn, params) declaration in InvoiceController (module containing create/2) so tools and dialyzer have documentation and type info for this public function.
292-315: Missing@docand@specfor publicdismiss_duplicate/2action.Per coding guidelines, every public function must have
@docdocumentation and@spectype specification.📝 Proposed fix to add documentation and type spec
+ `@doc` """ + Dismiss a duplicate invoice flag. + + Marks the duplicate flag as dismissed. Can be called on invoices with + duplicate_status "suspected" or "confirmed". + """ + `@spec` dismiss_duplicate(Plug.Conn.t(), map()) :: Plug.Conn.t() def dismiss_duplicate(conn, %{"id" => id}) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex` around lines 292 - 315, Add an `@doc` and `@spec` for the public controller action dismiss_duplicate/2: document that it dismisses a duplicate invoice for the current company and describe expected parameters/returns, and add a `@spec` like dismiss_duplicate(Plug.Conn.t(), map()) :: Plug.Conn.t(); place the `@doc` immediately above the def and the `@spec` above the function definition (referencing dismiss_duplicate/2 and mentioning used helpers Invoices.dismiss_duplicate, invoice_json/1 and changeset_errors/1 in the doc for clarity).
🤖 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/api/invoice_controller.ex`:
- Around line 249-272: Add a `@doc` and `@spec` for the public controller action
confirm_duplicate/2: document its purpose, expected params (conn and a map with
"id"), possible responses, and side effects; add a typespec like `@spec`
confirm_duplicate(Plug.Conn.t(), map()) :: Plug.Conn.t() (or more specific types
if available) placed immediately above the def confirm_duplicate clause in
InvoiceController so the function is documented and has a type specification.
- Around line 116-130: The public controller action create/2 is missing `@doc` and
`@spec`; add a short `@doc` describing the endpoint behavior and expected
params/response, and add a `@spec` for create(conn :: Plug.Conn.t(), params ::
map()) :: Plug.Conn.t(); place these annotations immediately above the def
create(conn, params) declaration in InvoiceController (module containing
create/2) so tools and dialyzer have documentation and type info for this public
function.
- Around line 292-315: Add an `@doc` and `@spec` for the public controller action
dismiss_duplicate/2: document that it dismisses a duplicate invoice for the
current company and describe expected parameters/returns, and add a `@spec` like
dismiss_duplicate(Plug.Conn.t(), map()) :: Plug.Conn.t(); place the `@doc`
immediately above the def and the `@spec` above the function definition
(referencing dismiss_duplicate/2 and mentioning used helpers
Invoices.dismiss_duplicate, invoice_json/1 and changeset_errors/1 in the doc for
clarity).
In `@lib/ksef_hub/invoices.ex`:
- Around line 177-180: Add a brief inline comment immediately above the
conflict_target tuple that explains why {:unsafe_fragment,
~s|("company_id","ksef_number") WHERE ksef_number IS NOT NULL AND
duplicate_of_id IS NULL|} is used (Ecto does not support partial unique index
WHERE clauses in conflict_target), state that the fragment is hardcoded so it is
safe from SQL injection, and note that it must remain static if altered;
reference the conflict_target tuple in the invoices module so future maintainers
understand the rationale.
- Around line 325-336: The function find_original_id currently relies on an
implicit nil return when ksef_number is falsy; make this explicit by adding an
else clause that returns nil so intent is clearer to readers. Locate the private
function find_original_id(company_id, attrs) and modify the if ksef_number &&
ksef_number != "" block to include an else nil branch, leaving the existing
query (Invoice |> where ... |> Repo.one()) unchanged.
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 269-299: Add a new test in the describe "dismiss_duplicate" block
that mirrors the existing "dismisses a suspected duplicate invoice" test but
creates the duplicate invoice with duplicate_status: "confirmed" (use
create_owner_with_token(), insert(:invoice) for original and
insert(:manual_invoice, duplicate_of_id: original.id, duplicate_status:
"confirmed") for the duplicate), POSTs to
post("/api/invoices/#{duplicate.id}/dismiss-duplicate") and asserts conn.status
== 200 and Jason.decode!(conn.resp_body)["data"]["duplicate_status"] ==
"dismissed"; place it alongside the other tests so it validates that confirmed
-> dismissed is accepted by the dismiss_duplicate endpoint.
- Add inline comment explaining {:unsafe_fragment, ...} conflict_target
- Add explicit else nil to find_original_id for clarity
- Add controller test for confirmed -> dismissed transition
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
421-433: Consider adding symmetric test forsource=kseffilter.The test verifies filtering by
source=manualworks. Adding a test forsource=ksefwould ensure both filter values work correctly.✨ Optional: Add test for ksef source filter
test "filters invoices by source=ksef in index", %{conn: conn} do %{company: company, token: token} = create_owner_with_token() insert(:invoice, company: company, source: "ksef") insert(:manual_invoice, company: company, source: "manual") conn = conn |> api_conn(token) |> get("/api/invoices?source=ksef") body = Jason.decode!(conn.resp_body) assert length(body["data"]) == 1 assert hd(body["data"])["source"] == "ksef" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 421 - 433, Add a symmetric test to cover the "ksef" source path: duplicate the existing test in the describe "source filter" block (the test "filters invoices by source in index" that uses create_owner_with_token, insert(:invoice) and insert(:manual_invoice)) and change the query to get("/api/invoices?source=ksef") and assertions to expect one result with hd(body["data"])["source"] == "ksef"; place the new test alongside the existing one to ensure both source=manual and source=ksef filters are validated.lib/ksef_hub/invoices.ex (1)
232-257: Consider wrapping in a transaction for atomicity.The current flow has a TOCTOU window: between
detect_duplicatefinding no original andcreate_invoiceexecuting, another process could insert an invoice with the sameksef_number. While the retry logic on unique constraint violation mitigates this, wrapping the operation inRepo.transaction/1would provide stronger guarantees and clearer intent.The current defensive approach (catching constraint violations and retrying) does work, but:
- The
find_original_id/2call on line 249 could also race if the conflicting record is deleted.- The retry path duplicates the duplicate-marking logic.
♻️ Optional: Consider using a transaction
def create_manual_invoice(company_id, attrs) do + Repo.transaction(fn -> attrs = attrs |> Map.drop([:ksef_acquisition_date, :permanent_storage_date]) |> Map.drop(["ksef_acquisition_date", "permanent_storage_date"]) |> Map.merge(%{source: "manual", company_id: company_id}) attrs = detect_duplicate(company_id, attrs) case create_invoice(attrs) do {:ok, invoice} -> - {:ok, invoice} + invoice {:error, %Ecto.Changeset{} = changeset} -> if unique_ksef_number_conflict?(changeset) do - attrs - |> Map.merge(%{ - duplicate_of_id: find_original_id(company_id, attrs), - duplicate_status: "suspected" - }) - |> create_invoice() + case attrs + |> Map.merge(%{ + duplicate_of_id: find_original_id(company_id, attrs), + duplicate_status: "suspected" + }) + |> create_invoice() do + {:ok, invoice} -> invoice + {:error, changeset} -> Repo.rollback(changeset) + end else - {:error, changeset} + Repo.rollback(changeset) end end + end) endAs per coding guidelines: "Use Ecto transactions and unique constraints for atomicity in database operations"
🤖 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 232 - 257, Wrap the whole duplicate-detection-and-insert sequence in a single Repo.transaction: call detect_duplicate(company_id, attrs) inside the transaction, attempt create_invoice(attrs) there, and if you get {:error, %Ecto.Changeset{}} and unique_ksef_number_conflict?(changeset) then re-query the original (via find_original_id(company_id, attrs)) and perform the duplicate-marking update (setting duplicate_of_id and duplicate_status) inside the same transaction before retrying/returning; ensure create_manual_invoice uses Repo.transaction(fn -> ... end) so the detection, insert, re-query and duplicate update are atomic and avoid the TOCTOU race.
🤖 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/invoices.ex`:
- Around line 232-257: Wrap the whole duplicate-detection-and-insert sequence in
a single Repo.transaction: call detect_duplicate(company_id, attrs) inside the
transaction, attempt create_invoice(attrs) there, and if you get {:error,
%Ecto.Changeset{}} and unique_ksef_number_conflict?(changeset) then re-query the
original (via find_original_id(company_id, attrs)) and perform the
duplicate-marking update (setting duplicate_of_id and duplicate_status) inside
the same transaction before retrying/returning; ensure create_manual_invoice
uses Repo.transaction(fn -> ... end) so the detection, insert, re-query and
duplicate update are atomic and avoid the TOCTOU race.
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 421-433: Add a symmetric test to cover the "ksef" source path:
duplicate the existing test in the describe "source filter" block (the test
"filters invoices by source in index" that uses create_owner_with_token,
insert(:invoice) and insert(:manual_invoice)) and change the query to
get("/api/invoices?source=ksef") and assertions to expect one result with
hd(body["data"])["source"] == "ksef"; place the new test alongside the existing
one to ensure both source=manual and source=ksef filters are validated.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
237-318: Consider adding cross-company scoping tests for duplicate endpoints.The existing
approve,reject, andxmlendpoints all include tests verifying that accessing invoices from a different company returns 404. For consistency and to ensure proper authorization, consider adding similar tests forconfirm_duplicateanddismiss_duplicate.🧪 Example test for cross-company scoping
test "returns 404 when confirming duplicate from different company", %{conn: conn} do %{token: token} = create_owner_with_token() other_company = insert(:company) original = insert(:invoice, ksef_number: "other-company", company: other_company) duplicate = insert(:manual_invoice, ksef_number: "other-company", company: other_company, duplicate_of_id: original.id, duplicate_status: "suspected" ) assert_error_sent 404, fn -> conn |> api_conn(token) |> post("/api/invoices/#{duplicate.id}/confirm-duplicate") end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 237 - 318, Add cross-company scoping tests for the confirm_duplicate and dismiss_duplicate endpoints: create a different company and its original and manual duplicate invoices (with duplicate_status "suspected"), then call POST "/api/invoices/:id/confirm-duplicate" and POST "/api/invoices/:id/dismiss-duplicate" using a token from another company and assert a 404 is raised (use assert_error_sent 404). Reference the existing test names and endpoints (confirm_duplicate, dismiss_duplicate) and mirror the pattern used in approve/reject/xml cross-company tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 237-318: Add cross-company scoping tests for the confirm_duplicate
and dismiss_duplicate endpoints: create a different company and its original and
manual duplicate invoices (with duplicate_status "suspected"), then call POST
"/api/invoices/:id/confirm-duplicate" and POST
"/api/invoices/:id/dismiss-duplicate" using a token from another company and
assert a 404 is raised (use assert_error_sent 404). Reference the existing test
names and endpoints (confirm_duplicate, dismiss_duplicate) and mirror the
pattern used in approve/reject/xml cross-company tests.
Mirrors the existing 404 pattern from approve/reject/xml to ensure confirm_duplicate and dismiss_duplicate reject invoices from other companies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
199-223: Consider adding a test for cross-company duplicate non-detection.The test verifies that duplicate detection works when
ksef_numbermatches an invoice within the same company. However, there's no test verifying that an invoice in a different company with the sameksef_numberdoes not trigger duplicate detection. This edge case would confirm company-scoped deduplication works correctly.💡 Suggested test case
test "does not detect duplicate when ksef_number matches invoice in different company", %{conn: conn} do %{token: token} = create_owner_with_token() other_company = insert(:company) _other_invoice = insert(:invoice, ksef_number: "cross-company-123", company: other_company) body = Jason.encode!(%{ type: "expense", ksef_number: "cross-company-123", seller_nip: "1234567890", seller_name: "Seller Sp. z o.o.", buyer_nip: "0987654321", buyer_name: "Buyer S.A.", invoice_number: "FV/2026/003", issue_date: "2026-02-20", net_amount: "1000.00", gross_amount: "1230.00" }) conn = conn |> api_conn(token) |> post("/api/invoices", body) assert conn.status == 201 data = Jason.decode!(conn.resp_body)["data"] assert is_nil(data["duplicate_of_id"]) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 199 - 223, Add a new test that ensures duplicate detection is scoped to a company: use create_owner_with_token() to get a token for the request-owning company, insert(:company) to create a different company and insert(:invoice, ksef_number: "cross-company-123", company: other_company) there, then build a request body with ksef_number "cross-company-123", call conn |> api_conn(token) |> post("/api/invoices", body) and assert conn.status == 201 and that Jason.decode!(conn.resp_body)["data"]["duplicate_of_id"] is nil; place this alongside the existing "creates manual invoice and detects duplicate by ksef_number" test so it exercises the same controller paths used by the duplicate logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 199-223: Add a new test that ensures duplicate detection is scoped
to a company: use create_owner_with_token() to get a token for the
request-owning company, insert(:company) to create a different company and
insert(:invoice, ksef_number: "cross-company-123", company: other_company)
there, then build a request body with ksef_number "cross-company-123", call conn
|> api_conn(token) |> post("/api/invoices", body) and assert conn.status == 201
and that Jason.decode!(conn.resp_body)["data"]["duplicate_of_id"] is nil; place
this alongside the existing "creates manual invoice and detects duplicate by
ksef_number" test so it exercises the same controller paths used by the
duplicate logic.
Ensures that creating a manual invoice with a ksef_number matching an invoice from a different company does not flag it as a duplicate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/ksef_hub_web/controllers/api/invoice_controller_test.exs (1)
382-416: Consider consolidating similar test blocks.These three describe blocks test the same behavior (422 for nil xml_content) across three endpoints. While the current separation is acceptable, you could optionally consolidate them using a parameterized approach to reduce duplication.
♻️ Optional: Parameterized test approach
describe "content endpoints with nil xml_content" do for endpoint <- ["xml", "html", "pdf"] do test "#{endpoint} returns 422 for invoice without xml_content", %{conn: conn} do %{company: company, token: token} = create_owner_with_token() invoice = insert(:manual_invoice, company: company) conn = conn |> api_conn(token) |> get("/api/invoices/#{invoice.id}/#{unquote(endpoint)}") assert conn.status == 422 assert Jason.decode!(conn.resp_body)["error"] == "Invoice has no XML content" end end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs` around lines 382 - 416, The three separate describe blocks testing 422 for nil xml_content (tests named "returns 422 for invoice without xml_content" that call get("/api/invoices/#{invoice.id}/xml"), get("/api/invoices/#{invoice.id}/html"), get("/api/invoices/#{invoice.id}/pdf")) should be consolidated into a single parameterized describe to remove duplication: create one describe (e.g., "content endpoints with nil xml_content") and loop over the endpoints ["xml","html","pdf"], reusing the existing setup calls create_owner_with_token(), insert(:manual_invoice, company: company) and api_conn(token), and make the GET against "/api/invoices/#{invoice.id}/#{endpoint}" (use unquote(endpoint) in the test body) while asserting status 422 and the same JSON error; keep existing helper calls and assertions 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 `@test/ksef_hub_web/controllers/api/invoice_controller_test.exs`:
- Around line 382-416: The three separate describe blocks testing 422 for nil
xml_content (tests named "returns 422 for invoice without xml_content" that call
get("/api/invoices/#{invoice.id}/xml"), get("/api/invoices/#{invoice.id}/html"),
get("/api/invoices/#{invoice.id}/pdf")) should be consolidated into a single
parameterized describe to remove duplication: create one describe (e.g.,
"content endpoints with nil xml_content") and loop over the endpoints
["xml","html","pdf"], reusing the existing setup calls
create_owner_with_token(), insert(:manual_invoice, company: company) and
api_conn(token), and make the GET against
"/api/invoices/#{invoice.id}/#{endpoint}" (use unquote(endpoint) in the test
body) while asserting status 422 and the same JSON error; keep existing helper
calls and assertions unchanged.
Replace three identical describe blocks (xml/html/pdf) with a single compile-time loop over endpoints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 23, 2026
Merged
Merged
Merged
This was referenced Mar 1, 2026
Merged
This was referenced Mar 18, 2026
Merged
This was referenced Apr 1, 2026
This was referenced Apr 8, 2026
6 tasks
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
Bug Fixes / Validation
Chores
Tests