refactor: simplify invoice tags API to single PUT endpoint, improve O…#80
Merged
Merged
Conversation
…penAPI docs Remove POST /invoices/:id/tags (add_tags) and DELETE /invoices/:id/tags/:tag_id (remove_tag) endpoints, keeping only PUT /invoices/:id/tags (set_tags). The frontend always has the full tag list, so a single replace-all endpoint is sufficient. Also improve OpenAPI response descriptions across all API controllers to describe specific error conditions instead of generic "Unauthorized" / "Validation error". Replace manually-maintained endpoint list in README with pointer to SwaggerUI. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidated invoice tag operations to a single PUT-based replacement flow; removed POST/DELETE invoice tag routes and corresponding controller methods; updated README to point to OpenAPI/SwaggerUI; clarified OpenAPI response descriptions across invoice, category, tag, and token controllers; tests adjusted for set_tags semantics. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,230,240,0.5)
participant Client
participant Router
participant InvoiceController
participant TagService
participant DB
end
Client->>Router: PUT /api/invoices/:id/tags (tag_ids list)
Router->>InvoiceController: dispatch set_tags/2
InvoiceController->>TagService: validate tag_ids, company lookup
TagService->>DB: fetch tags by ids
DB-->>TagService: tags (or missing)
TagService-->>InvoiceController: validated tags or error
InvoiceController->>DB: update invoice tag associations (replace)
DB-->>InvoiceController: updated invoice
InvoiceController-->>Client: 200 OK with updated invoice payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🤖 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/category_controller.ex`:
- Around line 27-28: The file fails CI formatting; run mix format on the module
(CategoryController) source and commit the changes so the map entries like the
response tuple lines (200 => {"Category list ordered by sort_order then name",
"application/json", Schemas.CategoryListResponse}, 401 => {"Unauthorized —
missing or invalid API token", "application/json", Schemas.ErrorResponse})
conform to project’s formatter rules; simply run `mix format` (or apply the
formatter to this file), verify formatting, and push the formatted file.
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 103-104: The file fails the project's formatter; run mix format to
correct formatting issues so the map entries (e.g. the 200 => {"Paginated
invoice list with metadata", "application/json", Schemas.InvoiceListResponse}
and 401 => {"Unauthorized — missing or invalid API token", "application/json",
Schemas.ErrorResponse}) and the surrounding InvoiceController module conform to
mix format rules; simply run mix format (or format this file) and commit the
resulting changes to resolve the CI formatting failure.
In `@lib/ksef_hub_web/controllers/api/tag_controller.ex`:
- Around line 28-29: The CI failure is due to formatting issues in the
TagController module (e.g., the spec lines referencing Schemas.TagListResponse
and Schemas.ErrorResponse); run mix format on the codebase (or format this file)
to correct indentation and spacing so the lines like the 200/401 response map
conform to Elixir formatting rules, then commit the formatted file.
In `@lib/ksef_hub_web/controllers/api/token_controller.ex`:
- Around line 26-27: The file has formatting issues causing the CI mix format
check to fail; run the Elixir formatter and commit the changes for
lib/ksef_hub_web/controllers/api/token_controller.ex (ensure the module
TokenController and the map entries referencing Schemas.TokenListResponse and
Schemas.ErrorResponse are formatted by running mix format or formatting the file
in your editor), then re-run the pipeline so the formatted file passes the CI
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc6e65cd-f1f9-460c-92e8-cc7b9470bad7
📒 Files selected for processing (7)
README.mdlib/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/controllers/api/token_controller.exlib/ksef_hub_web/router.extest/ksef_hub_web/controllers/api/invoice_controller_test.exs
💤 Files with no reviewable changes (1)
- lib/ksef_hub_web/router.ex
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 172-174: The OpenAPI 415 response text claims "accepted: PDF, PNG,
JPG, HEIC, WEBP" but the actual validator validate_content_type/1 only checks
for PDF magic bytes; either extend validate_content_type/1 to detect
PNG/JPG/HEIC/WEBP by adding their respective magic-byte checks (and update
tests) or change the 415 mapping's message (the tuple at the 415 key in the
response map) to accurately state "accepted: PDF" so docs match implementation;
reference validate_content_type/1 and the 415 response tuple to locate the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffe29403-9ab6-435e-bd23-88599934a424
📒 Files selected for processing (4)
lib/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/controllers/api/token_controller.ex
✅ Files skipped from review due to trivial changes (1)
- lib/ksef_hub_web/controllers/api/token_controller.ex
…ests - Fix InvoiceTagsRequest moduledoc (no longer mentions "adding") - Remove dead render_tag_error(:tag_not_in_company) clause - Clarify set_tags description: empty list clears all tags - Add tests for invalid tag_ids payloads (non-list, invalid UUIDs) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The upload endpoint only accepts PDF files (magic byte check), but the description incorrectly listed PNG, JPG, HEIC, WEBP as accepted types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/ksef_hub_web/controllers/api/invoice_controller.ex (1)
172-174:⚠️ Potential issue | 🟡 MinorThe 415 description still overstates accepted upload formats.
Line 172 says PNG/JPG/HEIC/WEBP are accepted, but
validate_content_type/1at Lines 920-927 only accepts PDF magic bytes and the runtime error at Line 232 also says the file must be a PDF. Either expand the validator/tests or narrow the OpenAPI text to PDF only.🤖 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 172 - 174, The OpenAPI 415 response text in the invoice_controller mapping overstates accepted formats while validate_content_type/1 and the runtime error only allow PDF; update the 415 description to state only PDF is accepted (replace "PDF, PNG, JPG, HEIC, WEBP" with "PDF") so the controller's error message and the validate_content_type/1 behavior (and the runtime error thrown at the invoice upload path) are consistent; alternatively, if you intend to accept other formats, extend validate_content_type/1 and related tests to recognize those magic bytes and adjust the runtime error accordingly — but by default change the description to "PDF" to match validate_content_type/1 and the runtime error.
🤖 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 482-484: The 422 response description for dismiss_duplicate is
incorrect: update the 422 entry in dismiss_duplicate's response map (the tuple
currently containing "Invoice is not flagged as a suspected duplicate",
"application/json", Schemas.ErrorResponse) to accurately list the real failure
cases ("invoice is not a suspected duplicate" and "invoice is already
dismissed") or replace the single message with a neutral phrase like "invoice is
not a duplicate or already dismissed" so it matches the behavior asserted by
test/ksef_hub_web/controllers/api/invoice_controller_test.exs and the
controller's dismiss_duplicate logic.
- Around line 178-180: The API contract currently documents extractor outages as
returning 502 (the tuple keyed by 502), but the tests
(test/ksef_hub_web/controllers/api/invoice_controller_test.exs lines ~579-603)
exercise that path and the controller actually returns 201 with
extraction_status == "failed"; update the contract to reflect the real behavior
by replacing (or adding) the 502 entry so the response for extractor failures is
a 201 response using the InvoiceResponse schema (with extraction_status
"failed") instead of claiming a 502 Schema/ErrorResponse; ensure the tuple that
currently reads 502 => {"Extraction service unavailable or returned an error",
"application/json", Schemas.ErrorResponse} is changed to the 201 form matching
the controller behavior.
- Around line 745-746: The current validate_tag_ids/1 helper returns {:ok, []}
for nil which lets omitted tag_ids act like an explicit clear; change
validate_tag_ids(nil) to return an error tuple (e.g. {:error, :missing_tag_ids}
or {:error, "tag_ids required"}) so the controller will translate that into a
422, and update the controller branch that calls validate_tag_ids (the call
around Line 853 in invoice_controller.ex) to handle this error case and return a
422 response instead of proceeding to clear tags.
---
Duplicate comments:
In `@lib/ksef_hub_web/controllers/api/invoice_controller.ex`:
- Around line 172-174: The OpenAPI 415 response text in the invoice_controller
mapping overstates accepted formats while validate_content_type/1 and the
runtime error only allow PDF; update the 415 description to state only PDF is
accepted (replace "PDF, PNG, JPG, HEIC, WEBP" with "PDF") so the controller's
error message and the validate_content_type/1 behavior (and the runtime error
thrown at the invoice upload path) are consistent; alternatively, if you intend
to accept other formats, extend validate_content_type/1 and related tests to
recognize those magic bytes and adjust the runtime error accordingly — but by
default change the description to "PDF" to match validate_content_type/1 and the
runtime error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37025238-4bab-4705-bac5-7b3240c7d6cf
📒 Files selected for processing (3)
lib/ksef_hub_web/controllers/api/invoice_controller.exlib/ksef_hub_web/schemas/invoice_tags_request.extest/ksef_hub_web/controllers/api/invoice_controller_test.exs
✅ Files skipped from review due to trivial changes (1)
- lib/ksef_hub_web/schemas/invoice_tags_request.ex
Comment on lines
+745
to
+746
| description: | ||
| "Replaces all tags on an invoice with the given list. Pass an empty list to clear all tags.", |
There was a problem hiding this comment.
Missing tag_ids still behaves like “clear all tags”.
Line 746 makes empty-list clearing explicit, but validate_tag_ids(nil) at Line 853 still returns {:ok, []}. A client sending {} or omitting tag_ids will therefore wipe all tags instead of getting a 422, which makes this endpoint unexpectedly destructive.
💡 Suggested fix
- `@spec` validate_tag_ids(term()) :: {:ok, [String.t()]} | {:error, :invalid_tag_ids}
- defp validate_tag_ids(nil), do: {:ok, []}
+ `@spec` validate_tag_ids(term()) ::
+ {:ok, [String.t()]} | {:error, :missing_tag_ids | :invalid_tag_ids}
+ defp validate_tag_ids(nil), do: {:error, :missing_tag_ids}+ defp render_tag_error(conn, _id, {:error, :missing_tag_ids}) do
+ conn
+ |> put_status(:unprocessable_entity)
+ |> json(%{error: "Missing required tag_ids payload"})
+ 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/invoice_controller.ex` around lines 745 -
746, The current validate_tag_ids/1 helper returns {:ok, []} for nil which lets
omitted tag_ids act like an explicit clear; change validate_tag_ids(nil) to
return an error tuple (e.g. {:error, :missing_tag_ids} or {:error, "tag_ids
required"}) so the controller will translate that into a 422, and update the
controller branch that calls validate_tag_ids (the call around Line 853 in
invoice_controller.ex) to handle this error case and return a 422 response
instead of proceeding to clear tags.
…e 422 - Remove 502 from upload spec: extractor failures return 201 with extraction_status 'failed', not 502. Update 201 description to mention extraction_status (complete/partial/failed). - Fix dismiss_duplicate 422: now mentions both error cases (not a duplicate, or already dismissed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…penAPI docs
Remove POST /invoices/:id/tags (add_tags) and DELETE /invoices/:id/tags/:tag_id (remove_tag) endpoints, keeping only PUT /invoices/:id/tags (set_tags). The frontend always has the full tag list, so a single replace-all endpoint is sufficient.
Also improve OpenAPI response descriptions across all API controllers to describe specific error conditions instead of generic "Unauthorized" / "Validation error".
Replace manually-maintained endpoint list in README with pointer to SwaggerUI.
Summary by CodeRabbit
Documentation
Breaking Changes
Tests