Feat/invoice categories tags#48
Merged
Merged
Conversation
Create categories table with company scoping and unique name constraint, tags table with company scoping, and invoice_tags join table. Add category_id foreign key to invoices table with nilify_all on delete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add three new Ecto schemas for invoice categorization: - Category with group:target name format validation - Tag with virtual usage_count field - InvoiceTag join schema Update Invoice schema with category/tag associations and category_changeset/2 for dedicated category assignment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement full CRUD for categories and tags with company scoping, invoice-category assignment, invoice-tag associations (add/remove/set/list), and filter extensions for category_id and tag_ids in list_invoices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Create 12 new OpenAPI schema files for category/tag CRUD request/response types. Update Invoice schema with category_id, category, and tags properties. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New controllers: - CategoryController: full CRUD at /api/categories - TagController: full CRUD at /api/tags Extended InvoiceController: - PUT /api/invoices/:id/category (set/clear category) - POST /api/invoices/:id/tags (add tags) - PUT /api/invoices/:id/tags (replace tags) - DELETE /api/invoices/:id/tags/:tag_id (remove tag) - category_id and tag_ids[] query filters on index - Show endpoint preloads category and tags All endpoints include OpenAPI specs, company scoping, and cross-company validation. 76 controller tests passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Apply mix format and replace single-clause with/else with case in add_tags action per credo --strict. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract category_json/1, tag_json/1, atomize_keys/2 into shared KsefHubWeb.JsonHelpers module, removing duplicates across 3 controllers - Move Repo.preload from controller into context (get_invoice_with_details!) - Replace wasteful validate_tags_company (loaded all tags with GROUP BY) with efficient tags_belong_to_company? (single COUNT query) - Fix count_invoices + tag_ids distinct bug by wrapping in subquery() - Replace raw maps in set_invoice_tags with proper InvoiceTag changesets - DRY up get_category/get_category! and get_tag/get_tag! with base queries - Add missing OpenAPI parameters for category_id and tag_ids[] filters - Remove redundant Map.take calls (atomize_keys already filters) 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 Category and Tag domain models, invoice associations and join table, context APIs for category/tag CRUD and invoice-tag management, controller endpoints and routes, JSON helpers and OpenAPI schemas, DB migrations, and comprehensive tests for filtering and association behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as rgba(70,130,180,0.5) "API Controller"
participant Context as rgba(34,139,34,0.5) "Invoices Context"
participant DB as rgba(128,0,128,0.5) "Repo/DB"
Client->>API: PUT /api/invoices/:id/category {category_id}
API->>Context: set_invoice_category(invoice_id, category_id, company_id)
Context->>DB: fetch invoice scoped by company
DB-->>Context: invoice
Context->>DB: update invoice (category_changeset)
DB-->>Context: {:ok, updated_invoice}
Context->>DB: preload category and tags
DB-->>Context: invoice with associations
Context-->>API: {:ok, invoice}
API-->>Client: 200 {data: invoice_json}
sequenceDiagram
participant Client
participant API as rgba(70,130,180,0.5) "API Controller"
participant Context as rgba(34,139,34,0.5) "Invoices Context"
participant DB as rgba(128,0,128,0.5) "Repo/DB"
Client->>API: POST /api/invoices/:id/tags {tag_ids}
API->>Context: set_invoice_tags(invoice_id, tag_ids, company_id)
Context->>DB: verify tags belong to company (tag_query)
DB-->>Context: tag records
Context->>DB: delete existing invoice_tags for invoice (if replacing)
DB-->>Context: {:ok, _}
Context->>DB: insert invoice_tag rows
DB-->>Context: {:ok, inserted_rows}
Context->>DB: preload tags for invoice
DB-->>Context: invoice with tags
Context-->>API: {:ok, tags}
API-->>Client: 200 {data: [tag_json,...]}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lib/ksef_hub/invoices/tag.ex (1)
12-19: Use theInvoiceTagjoin schema module reference for consistency.The Invoice schema already uses
join_through: KsefHub.Invoices.InvoiceTagon its side of the relationship. Update the Tag schema to match, which enables Ecto to leverage the InvoiceTag validations and constraints when managing associations from either side.♻️ Suggested change
- many_to_many :invoices, KsefHub.Invoices.Invoice, join_through: "invoice_tags" + many_to_many :invoices, KsefHub.Invoices.Invoice, + join_through: KsefHub.Invoices.InvoiceTag🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/tag.ex` around lines 12 - 19, The Tag schema's many_to_many association uses a string join table; change the many_to_many :invoices association in KsefHub.Invoices.Tag (schema "tags") to use the join_through module KsefHub.Invoices.InvoiceTag instead of "invoice_tags" so it matches the Invoice schema and allows Ecto to apply InvoiceTag validations/constraints when managing associations from either side.test/ksef_hub_web/controllers/api/tag_controller_test.exs (1)
91-113: Consider adding a test for invalid update validation.The update describe block tests successful updates and cross-company 404s, but doesn't test validation errors (e.g., empty name). This would align with the create and CategoryController patterns.
💡 Suggested test addition
test "returns 422 for invalid update", %{conn: conn} do %{company: company, token: token} = create_owner_with_token() tag = insert(:tag, company: company) body = Jason.encode!(%{name: ""}) conn = conn |> api_conn(token) |> put("/api/tags/#{tag.id}", body) assert conn.status == 422 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/tag_controller_test.exs` around lines 91 - 113, Add a test in the "update" describe block that asserts validation errors return 422: using create_owner_with_token() to get company and token, insert(:tag, company: company) to create the tag, call api_conn(token) |> put("/api/tags/#{tag.id}", body) with a JSON body where name is "" and assert conn.status == 422; place it alongside the existing "updates a tag" and cross-company 404 tests so update validations are covered like in the create and CategoryController tests.lib/ksef_hub_web/schemas/invoice.ex (1)
86-108: Consider adding new fields to the example.The example payload doesn't include
category_id,category, ortags. Adding these would improve API documentation clarity.📝 Suggested example additions
source: "ksef", + category_id: "b2c3d4e5-f6a7-8901-bcde-f12345678901", + category: %{ + id: "b2c3d4e5-f6a7-8901-bcde-f12345678901", + name: "ops:invoices" + }, + tags: [ + %{id: "c3d4e5f6-a7b8-9012-cdef-123456789012", name: "urgent"} + ], duplicate_of_id: nil,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/schemas/invoice.ex` around lines 86 - 108, Update the example map in the Invoice schema's `example` (in module Invoice) to include the missing fields `category_id`, `category`, and `tags`; add `category_id` as a UUID string (e.g. "d4c3b2a1-..."), `category` as a short human-readable string (e.g. "office supplies"), and `tags` as a list of strings (e.g. ["urgent","quarterly"]) so the documented example reflects those schema attributes.
🤖 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/controllers/api/invoice_controller.ex`:
- Around line 699-703: maybe_put_list/3 currently only matches lists and nil,
causing a FunctionClauseError when query params like tag_ids arrive as a single
string; add a clause to handle binaries (e.g., when tag_ids is a string) by
converting the string to a single-element list (Map.put(map, key, [value])), and
add a fallback clause defp maybe_put_list(map, _key, _invalid), do: map to
safely ignore other invalid types; update function maybe_put_list/3 to include
these clauses so tag_ids and similar params are handled without raising.
- Around line 558-603: The handlers add_tags and set_tags currently read
params["tag_ids"] directly and pass it to Invoices.tags_belong_to_company?/2
which can crash on non-list input; validate/normalize tag_ids before calling
downstream functions: ensure tag_ids is nil or a list (e.g., coerce nil to []
and reject non-list types), verify elements are the expected type/format
(strings/UUIDs) or at least ensure Enum-safe usage, and if invalid return conn
|> put_status(:unprocessable_entity) |> json(%{error: "Invalid tag_ids
payload"}) instead of proceeding; update both add_tags and set_tags to perform
this check prior to calling Invoices.tags_belong_to_company?/2 (and before
Enum.each or Invoices.set_invoice_tags/1) so malformed input yields 422 rather
than a 500.
In `@lib/ksef_hub/invoices.ex`:
- Around line 493-508: The set_invoice_tags/2 function currently uses
Repo.insert!/1 inside the Repo.transaction which will raise on unique_constraint
violations; replace the Enum.each + Repo.insert!/1 with a safe insert flow using
Repo.insert/1 and rollback on error: iterate tag_ids (e.g., with
Enum.reduce_while), call InvoiceTag.changeset/1 and Repo.insert/1 for each, on
{:ok, _} continue and on {:error, changeset} call Repo.rollback(changeset) so
the transaction returns {:error, changeset} instead of crashing; ensure the
final successful branch returns the list from list_invoice_tags(invoice_id)
wrapped as {:ok, tags} to match the `@spec`.
In `@lib/ksef_hub/invoices/invoice_tag.ex`:
- Around line 21-27: Remove invoice_id and tag_id from being mass-assignable in
the changeset: in the changeset/2 function of invoice_tag.ex remove
[:invoice_id, :tag_id] from the cast and from validate_required (do not rely on
attrs to supply them). Keep the constraints (unique_constraint,
foreign_key_constraint) but ensure callers (e.g., add_invoice_tag/2 and the bulk
tag operations in invoices.ex) set those values using put_change when building
the changeset, following the same pattern used for server-set fields in
credentials.ex.
---
Nitpick comments:
In `@lib/ksef_hub_web/schemas/invoice.ex`:
- Around line 86-108: Update the example map in the Invoice schema's `example`
(in module Invoice) to include the missing fields `category_id`, `category`, and
`tags`; add `category_id` as a UUID string (e.g. "d4c3b2a1-..."), `category` as
a short human-readable string (e.g. "office supplies"), and `tags` as a list of
strings (e.g. ["urgent","quarterly"]) so the documented example reflects those
schema attributes.
In `@lib/ksef_hub/invoices/tag.ex`:
- Around line 12-19: The Tag schema's many_to_many association uses a string
join table; change the many_to_many :invoices association in
KsefHub.Invoices.Tag (schema "tags") to use the join_through module
KsefHub.Invoices.InvoiceTag instead of "invoice_tags" so it matches the Invoice
schema and allows Ecto to apply InvoiceTag validations/constraints when managing
associations from either side.
In `@test/ksef_hub_web/controllers/api/tag_controller_test.exs`:
- Around line 91-113: Add a test in the "update" describe block that asserts
validation errors return 422: using create_owner_with_token() to get company and
token, insert(:tag, company: company) to create the tag, call api_conn(token) |>
put("/api/tags/#{tag.id}", body) with a JSON body where name is "" and assert
conn.status == 422; place it alongside the existing "updates a tag" and
cross-company 404 tests so update validations are covered like in the create and
CategoryController tests.
…d changeset - Add binary and fallback clauses to maybe_put_list/3 for single-string tag_ids query params - Validate/normalize tag_ids in add_tags and set_tags before passing to context — malformed input now returns 422 instead of 500 - Replace Repo.insert!/1 with Repo.insert/1 + Repo.rollback in set_invoice_tags so unique_constraint violations don't crash the transaction - Remove invoice_id/tag_id from InvoiceTag cast — FKs are now set via Ecto.Changeset.change/2, matching the project's server-set-field pattern - Use join_through module (InvoiceTag) instead of string in Tag schema - Add category_id, category, tags to Invoice OpenAPI example - Add missing "returns 422 for invalid update" test for TagController Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/invoice_tag.exlib/ksef_hub/invoices/tag.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/schemas/invoice.extest/ksef_hub_web/controllers/api/tag_controller_test.exs
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/ksef_hub/invoices/invoice_tag.ex
- lib/ksef_hub/invoices/tag.ex
🤖 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/controllers/api/invoice_controller.ex`:
- Around line 521-534: The current validators validate_tag_ids and
validate_category_company only check is_binary/1 and allow malformed UUIDs to
hit the DB; update those functions to use Ecto.UUID.cast/1 to validate each UUID
(e.g., return {:ok, uuid} for valid or {:error, :invalid_uuid} for any bad
value) and make the controller actions that call them (set_category /
Invoices.set_invoice_category flow shown, plus add_tags and set_tags handlers)
handle the new {:error, :invalid_uuid} case by returning
put_status(:unprocessable_entity) and a JSON error (e.g., %{error: "Invalid UUID
format"}) instead of letting the DB raise. Ensure you reference
validate_tag_ids, validate_category_company, Ecto.UUID.cast/1, and the
controller actions set_category, add_tags, set_tags when locating and changing
the code.
- validate_tag_ids now checks each element with Ecto.UUID.cast/1
instead of just is_binary/1, rejecting malformed UUIDs early
- validate_category_company checks UUID format before querying,
returning {:error, :invalid_uuid} for bad values
- set_category action handles :invalid_uuid with 422 response
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/controllers/api/invoice_controller.ex`:
- Around line 649-664: The remove_tag action doesn't validate that the incoming
"tag_id" is a UUID before calling Invoices.remove_invoice_tag/2; add a UUID
format check in remove_tag (e.g., using Ecto.UUID.cast/1 or a UUID validation
helper) and if the check fails immediately return conn |>
put_status(:unprocessable_entity) |> json(%{error: "Invalid tag_id"}) instead of
calling Invoices.remove_invoice_tag(id, tag_id), otherwise proceed as before
(retain the existing invoice lookup via Invoices.get_invoice!/3 and the current
OK/:not_found branches).
- Around line 604-625: The set_tags action's with-chain doesn't handle an error
return from Invoices.set_invoice_tags/2, so a {:error, changeset} will crash;
update set_tags to match and handle that case in the else block (or add an
additional with clause) by pattern-matching {:error, changeset} from
Invoices.set_invoice_tags and returning a 422 JSON response containing the
changeset errors (e.g., render or json %{error: "Could not set tags", details:
translate_errors(changeset)}), keeping existing handling for {:error,
:invalid_tag_ids} and false from validate_tag_ids and
Invoices.tags_belong_to_company?.
- Around line 514-540: The response omits category/tags because
set_invoice_category returns an invoice without preloads; update set_category to
return a fully preloaded invoice before calling invoice_json by either (A)
changing Invoices.set_invoice_category to preload :category and :tags on the
returned struct, or (B) refetching after update using
Invoices.get_invoice_with_details!(company_id, updated.id, role:
conn.assigns[:current_role]) and pass that preloaded invoice to invoice_json;
locate references to set_category, Invoices.set_invoice_category, invoice_json
and get_invoice_with_details! to implement the preload/refetch fix.
…oad set_category response
- remove_tag now validates tag_id with Ecto.UUID.cast before querying,
returning 422 for malformed UUIDs
- set_tags else block now handles {:error, changeset} from
set_invoice_tags instead of letting it crash
- set_category refetches with get_invoice_with_details! after update
so the response includes preloaded category and tags
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai, review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ksef_hub_web/controllers/api/invoice_controller.ex (1)
569-573: Consider handlingadd_invoice_tagreturn values to avoid silent failures.Line 571 uses
Enum.each/2which discards the return value fromInvoices.add_invoice_tag/2. If an insertion fails (e.g., due to a database error), the failure is silently ignored and the API returns success. While duplicate tag insertions failing silently could be intentional (idempotent behavior), other failures would go unnoticed.Consider collecting results and handling failures:
♻️ Proposed pattern to handle failures
- Enum.each(tag_ids, fn tag_id -> Invoices.add_invoice_tag(id, tag_id) end) - tags = Invoices.list_invoice_tags(id) - json(conn, %{data: Enum.map(tags, &tag_json/1)}) + results = Enum.map(tag_ids, fn tag_id -> Invoices.add_invoice_tag(id, tag_id) end) + + case Enum.find(results, &match?({:error, _}, &1)) do + nil -> + tags = Invoices.list_invoice_tags(id) + json(conn, %{data: Enum.map(tags, &tag_json/1)}) + + {:error, changeset} -> + conn + |> put_status(:unprocessable_entity) + |> json(%{error: changeset_errors(changeset)}) + endAlternatively, if idempotent behavior is desired, use
on_conflict: :nothingin the insert or document that duplicate additions are silently ignored.🤖 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 569 - 573, The current Enum.each calling Invoices.add_invoice_tag/2 swallows failures; change it to collect and handle results (e.g., use Enum.reduce_while or Enum.map to run Invoices.add_invoice_tag(id, tag_id) for each tag and pattern-match results), return an error JSON if any call returns {:error, reason}, or alternatively make add_invoice_tag idempotent by implementing on_conflict: :nothing in the underlying insert and document that duplicate inserts are ignored; update the block around validate_tag_ids, Invoices.tags_belong_to_company?, Invoices.add_invoice_tag, Invoices.list_invoice_tags and tag_json to propagate and respond to errors instead of silently succeeding.
🤖 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 569-573: The current Enum.each calling Invoices.add_invoice_tag/2
swallows failures; change it to collect and handle results (e.g., use
Enum.reduce_while or Enum.map to run Invoices.add_invoice_tag(id, tag_id) for
each tag and pattern-match results), return an error JSON if any call returns
{:error, reason}, or alternatively make add_invoice_tag idempotent by
implementing on_conflict: :nothing in the underlying insert and document that
duplicate inserts are ignored; update the block around validate_tag_ids,
Invoices.tags_belong_to_company?, Invoices.add_invoice_tag,
Invoices.list_invoice_tags and tag_json to propagate and respond to errors
instead of silently succeeding.
Duplicate tag associations are now silently ignored instead of returning an error, so the Enum.each in add_tags can't swallow failures — there are none to swallow. Update test to verify idempotency (second add succeeds, count stays at 1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/ksef_hub/invoices.ex (1)
495-506:⚠️ Potential issue | 🟡 MinorDeduplicate tag IDs to avoid rollbacks on duplicates.
Iftag_idscontains duplicates, the unique constraint will rollback the transaction even though the desired final set is valid.✅ Suggested fix
def set_invoice_tags(invoice_id, tag_ids) do + tag_ids = Enum.uniq(tag_ids) + Repo.transaction(fn -> InvoiceTag |> where([it], it.invoice_id == ^invoice_id) |> Repo.delete_all()🤖 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 495 - 506, set_invoice_tags will fail the transaction if tag_ids contains duplicates because insert_invoice_tag! triggers the InvoiceTag unique constraint; deduplicate the list before inserting to avoid unnecessary rollbacks. Modify set_invoice_tags to call Enum.uniq(tag_ids) (or similar) and iterate over the unique IDs when calling insert_invoice_tag! (keep deletion of existing tags and the final list_invoice_tags(invoice_id) as-is). Ensure you reference set_invoice_tags and insert_invoice_tag! so the change targets the right code path.
🤖 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/invoices.ex`:
- Around line 461-470: add_invoice_tag currently inserts an InvoiceTag without
verifying the tag belongs to the same company as the invoice (same for
set_invoice_tags/2); update both functions to first load the invoice (by
invoice_id) and the Tag (by tag_id), compare invoice.company_id and
tag.company_id, and if they differ return {:error, changeset} (add an error on
the InvoiceTag changeset or a new changeset via InvoiceTag.changeset) so the API
shape remains {:ok, InvoiceTag.t()} | {:error, Ecto.Changeset.t()}; only call
Repo.insert(on_conflict: :nothing) when the company_ids match. Ensure you keep
using InvoiceTag.changeset and preserve idempotency behavior.
- Around line 450-456: In set_invoice_category, validate that category_id is
either nil or belongs to the same company as the invoice before calling
Invoice.category_changeset and Repo.update: if category_id is nil allow proceed,
otherwise query Repo.get_by(Category, id: category_id, company_id:
invoice.company_id) (or Repo.exists?/1) and return an {:error, ...} (or an
Ecto.Changeset with an added error on :category_id) when the lookup fails; only
call Invoice.category_changeset and Repo.update when the category belongs to
invoice.company_id. Use the existing Invoice, Invoice.category_changeset, Repo,
and Category identifiers to locate where to perform this guard.
---
Duplicate comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 495-506: set_invoice_tags will fail the transaction if tag_ids
contains duplicates because insert_invoice_tag! triggers the InvoiceTag unique
constraint; deduplicate the list before inserting to avoid unnecessary
rollbacks. Modify set_invoice_tags to call Enum.uniq(tag_ids) (or similar) and
iterate over the unique IDs when calling insert_invoice_tag! (keep deletion of
existing tags and the final list_invoice_tags(invoice_id) as-is). Ensure you
reference set_invoice_tags and insert_invoice_tag! so the change targets the
right code path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/ksef_hub/invoices.extest/ksef_hub/invoices/tags_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/ksef_hub/invoices/tags_test.exs
Comment on lines
+461
to
+470
| @doc """ | ||
| Adds a tag to an invoice. Idempotent — duplicate associations are silently ignored. | ||
| """ | ||
| @spec add_invoice_tag(Ecto.UUID.t(), Ecto.UUID.t()) :: | ||
| {:ok, InvoiceTag.t()} | {:error, Ecto.Changeset.t()} | ||
| def add_invoice_tag(invoice_id, tag_id) do | ||
| invoice_id | ||
| |> InvoiceTag.changeset(tag_id) | ||
| |> Repo.insert(on_conflict: :nothing) | ||
| end |
There was a problem hiding this comment.
Validate tag ownership when associating to invoices.
This can link a tag from another company to the invoice. Please verify the tag belongs to the invoice’s company (and apply the same check in set_invoice_tags/2).
🔒 Suggested guard (keeps {:error, changeset} shape)
def add_invoice_tag(invoice_id, tag_id) do
- invoice_id
- |> InvoiceTag.changeset(tag_id)
- |> Repo.insert(on_conflict: :nothing)
+ with %Invoice{company_id: company_id} <- Repo.get(Invoice, invoice_id),
+ true <- tags_belong_to_company?([tag_id], company_id) do
+ invoice_id
+ |> InvoiceTag.changeset(tag_id)
+ |> Repo.insert(on_conflict: :nothing)
+ else
+ _ ->
+ invoice_id
+ |> InvoiceTag.changeset(tag_id)
+ |> Ecto.Changeset.add_error(:tag_id, "does not belong to company")
+ |> then(&{:error, &1})
+ 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 461 - 470, add_invoice_tag currently
inserts an InvoiceTag without verifying the tag belongs to the same company as
the invoice (same for set_invoice_tags/2); update both functions to first load
the invoice (by invoice_id) and the Tag (by tag_id), compare invoice.company_id
and tag.company_id, and if they differ return {:error, changeset} (add an error
on the InvoiceTag changeset or a new changeset via InvoiceTag.changeset) so the
API shape remains {:ok, InvoiceTag.t()} | {:error, Ecto.Changeset.t()}; only
call Repo.insert(on_conflict: :nothing) when the company_ids match. Ensure you
keep using InvoiceTag.changeset and preserve idempotency behavior.
- set_invoice_category verifies category belongs to invoice's company - add_invoice_tag verifies tag belongs to invoice's company - set_invoice_tags deduplicates tag_ids with Enum.uniq before inserting - Controller handles new :category_not_in_company error from context Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai make final review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
> [!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/invoices/invoice.ex (1)
49-91:⚠️ Potential issue | 🟠 MajorPrevent cross‑company category assignment via the generic changeset.
changeset/2now casts:category_id, but it only enforces a foreign key. Any API path using this changeset can associate an invoice to a category from another company (if the UUID is known), bypassing the company check enforced inset_invoice_category/2. This is a multi‑tenant authorization risk.Safer options:
- Remove
:category_idfrom the generic changeset and requireset_invoice_category/2for assignment, or- Add an explicit validation that the category belongs to
invoice.company_idbefore accepting it.🛠️ Suggested fix (require set_invoice_category/2 for category assignment)
|> cast(attrs, [ :ksef_number, :type, :xml_content, :seller_nip, :seller_name, :buyer_nip, :buyer_name, :invoice_number, :issue_date, :net_amount, :vat_amount, :gross_amount, :currency, :status, :source, :duplicate_of_id, :duplicate_status, :ksef_acquisition_date, - :permanent_storage_date, - :category_id + :permanent_storage_date ]) @@ - |> foreign_key_constraint(:category_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/invoice.ex` around lines 49 - 91, The changeset/2 currently casts :category_id which allows cross-company assignment; either remove :category_id from the cast/validation pipeline so category can only be set via set_invoice_category/2, and also remove or move the foreign_key_constraint(:category_id) out of changeset/2 into the dedicated setter, OR keep :category_id but add an explicit validation (e.g. validate_category_belongs_to_company/1 called from changeset/2) that loads the Category by the supplied :category_id and verifies its company_id matches invoice.company_id (fail the changeset if not); update references to changeset/2 and set_invoice_category/2 accordingly.
🧹 Nitpick comments (2)
lib/ksef_hub_web/controllers/api/tag_controller.ex (1)
162-175: Handle potential delete failure gracefully.The pattern match
{:ok, _} = Invoices.delete_tag(tag)will crash if the delete fails (e.g., due to a database constraint). While unlikely for tags, defensive error handling would be more robust.♻️ Suggested defensive error handling
def delete(conn, %{"id" => id}) do company_id = conn.assigns.current_company.id case Invoices.get_tag(company_id, id) do {:ok, tag} -> - {:ok, _} = Invoices.delete_tag(tag) - json(conn, %{message: "Tag deleted"}) + case Invoices.delete_tag(tag) do + {:ok, _} -> + json(conn, %{message: "Tag deleted"}) + + {:error, _changeset} -> + conn + |> put_status(:unprocessable_entity) + |> json(%{error: "Failed to delete tag"}) + end {:error, :not_found} -> conn |> put_status(:not_found) |> json(%{error: "Tag not found"}) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/tag_controller.ex` around lines 162 - 175, In delete/2 (TagController) the pattern match {:ok, _} = Invoices.delete_tag(tag) will crash on failure; change it to handle both {:ok, _} and {:error, reason} returned by Invoices.delete_tag, returning json(conn, %{message: "Tag deleted"}) on success and responding with an appropriate error status (e.g., 500) and json body including the reason or a generic error message on failure; reference the Invoices.delete_tag/1 call and the delete(conn, %{"id" => id}) action when making the change.lib/ksef_hub_web/controllers/api/category_controller.ex (1)
161-176: Handle potential delete failure gracefully.Same pattern as TagController — the match
{:ok, _} = Invoices.delete_category(category)will crash if delete fails. Consider defensive handling.♻️ Suggested defensive error handling
def delete(conn, %{"id" => id}) do company_id = conn.assigns.current_company.id case Invoices.get_category(company_id, id) do {:ok, category} -> - {:ok, _} = Invoices.delete_category(category) - json(conn, %{message: "Category deleted"}) + case Invoices.delete_category(category) do + {:ok, _} -> + json(conn, %{message: "Category deleted"}) + + {:error, _changeset} -> + conn + |> put_status(:unprocessable_entity) + |> json(%{error: "Failed to delete category"}) + end {:error, :not_found} -> conn |> put_status(:not_found) |> json(%{error: "Category not found"}) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/controllers/api/category_controller.ex` around lines 161 - 176, The delete/2 action in CategoryController currently pattern-matches {:ok, _} = Invoices.delete_category(category) which will crash on failure; change it to handle both {:ok, _} and {:error, reason} from Invoices.delete_category(category) (in function delete/2) and return json(conn, %{message: "Category deleted"}) on success, or set an appropriate status (e.g., put_status(:internal_server_error)) and return json(conn, %{error: "Failed to delete category", details: reason}) on error so deletion failures are handled gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/ksef_hub/invoices/invoice.ex`:
- Around line 49-91: The changeset/2 currently casts :category_id which allows
cross-company assignment; either remove :category_id from the cast/validation
pipeline so category can only be set via set_invoice_category/2, and also remove
or move the foreign_key_constraint(:category_id) out of changeset/2 into the
dedicated setter, OR keep :category_id but add an explicit validation (e.g.
validate_category_belongs_to_company/1 called from changeset/2) that loads the
Category by the supplied :category_id and verifies its company_id matches
invoice.company_id (fail the changeset if not); update references to changeset/2
and set_invoice_category/2 accordingly.
---
Nitpick comments:
In `@lib/ksef_hub_web/controllers/api/category_controller.ex`:
- Around line 161-176: The delete/2 action in CategoryController currently
pattern-matches {:ok, _} = Invoices.delete_category(category) which will crash
on failure; change it to handle both {:ok, _} and {:error, reason} from
Invoices.delete_category(category) (in function delete/2) and return json(conn,
%{message: "Category deleted"}) on success, or set an appropriate status (e.g.,
put_status(:internal_server_error)) and return json(conn, %{error: "Failed to
delete category", details: reason}) on error so deletion failures are handled
gracefully.
In `@lib/ksef_hub_web/controllers/api/tag_controller.ex`:
- Around line 162-175: In delete/2 (TagController) the pattern match {:ok, _} =
Invoices.delete_tag(tag) will crash on failure; change it to handle both {:ok,
_} and {:error, reason} returned by Invoices.delete_tag, returning json(conn,
%{message: "Tag deleted"}) on success and responding with an appropriate error
status (e.g., 500) and json body including the reason or a generic error message
on failure; reference the Invoices.delete_tag/1 call and the delete(conn, %{"id"
=> id}) action when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/category.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/invoices/invoice_tag.exlib/ksef_hub/invoices/tag.exlib/ksef_hub_web/controllers/api/category_controller.exlib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/controllers/api/tag_controller.exlib/ksef_hub_web/json_helpers.exlib/ksef_hub_web/router.exlib/ksef_hub_web/schemas/category.exlib/ksef_hub_web/schemas/category_list_response.exlib/ksef_hub_web/schemas/category_response.exlib/ksef_hub_web/schemas/create_category_request.exlib/ksef_hub_web/schemas/create_tag_request.exlib/ksef_hub_web/schemas/invoice.exlib/ksef_hub_web/schemas/invoice_tags_request.exlib/ksef_hub_web/schemas/set_category_request.exlib/ksef_hub_web/schemas/tag.exlib/ksef_hub_web/schemas/tag_list_response.exlib/ksef_hub_web/schemas/tag_response.exlib/ksef_hub_web/schemas/update_category_request.exlib/ksef_hub_web/schemas/update_tag_request.expriv/repo/migrations/20260223063931_create_categories.exspriv/repo/migrations/20260223063932_create_tags.exstest/ksef_hub/invoices/categories_test.exstest/ksef_hub/invoices/tags_test.exstest/ksef_hub_web/controllers/api/category_controller_test.exstest/ksef_hub_web/controllers/api/invoice_controller_test.exstest/ksef_hub_web/controllers/api/tag_controller_test.exstest/support/factory.ex
…gracefully - Remove :category_id from changeset/2 cast list so categories can only be assigned via set_invoice_category/2 which validates company ownership - Remove dead foreign_key_constraint(:category_id) from changeset/2 (already present in category_changeset/2) - Handle delete failures gracefully in CategoryController and TagController instead of crashing on MatchError Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 14, 2026
This was referenced Apr 1, 2026
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
Documentation
Tests
Chores