refactor: modularize Invoices and Accounts contexts#144
Merged
Conversation
…modules The Invoices context (2,774 lines) and Accounts context (530 lines) had grown too large to maintain comfortably. This extracts cohesive function groups into dedicated sub-modules while preserving the existing public API — all callers still use `Invoices.function()` and `Accounts.function()` via facade delegates. Invoices.ex (2,774 → 1,300 lines): - Invoices.Extraction — parsing, field extraction, billing date computation - Invoices.Analytics — dashboard aggregation queries - Invoices.Comments — comment CRUD - Invoices.AccessControl — access grants, visibility filtering - Invoices.Classification — categories, tags, cost lines, predictions - Invoices.Queries — filtering, pagination, text search - Invoices.Reextraction — re-parse XML, re-extract PDF - Invoices.Duplicates — confirm/dismiss/detect duplicates Accounts.ex (530 → 370 lines): - Accounts.ApiTokens — token creation, validation, revocation Also removes dead code: - Invoice.types/0, statuses/0, sources/0 (zero callers) - Accounts.update_user_name/2 (zero production callers) - Invoice.company_fields/1 made private (only internal caller) DRYs duplicated bank field validations in Invoice schema. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors Accounts and Invoices by extracting API-token and invoice responsibilities into dedicated sub-modules. The top-level context modules become thin facades delegating token lifecycle, query/pagination, access control, extraction/re-extraction, duplicate handling, classification, analytics, and comments to new Changes
Sequence Diagram(s)(Skipped — changes span many independent flows implemented across multiple new modules; no single concise sequence diagram provided.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. Comment |
Establishes ~500-line threshold for context modules and documents the facade + defdelegate pattern used for sub-module extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 (5)
lib/ksef_hub/invoices.ex (2)
944-948: Consider keeping@doc falsefor internal helper.The
maybe_enqueue_prediction/2function has@doc falsebut is still defined withdefrather thandefp. If this function is only used internally within the facade, consider making it private withdefp.Proposed change
- `@doc` false - `@spec` maybe_enqueue_prediction(atom(), Invoice.t()) :: :ok | :skip | :enqueue_failed - def maybe_enqueue_prediction(:complete, invoice), do: enqueue_prediction(invoice) - def maybe_enqueue_prediction(_status, _invoice), do: :ok + `@spec` maybe_enqueue_prediction(atom(), Invoice.t()) :: :ok | :skip | :enqueue_failed + defp maybe_enqueue_prediction(:complete, invoice), do: enqueue_prediction(invoice) + defp maybe_enqueue_prediction(_status, _invoice), do: :ok🤖 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 944 - 948, maybe_enqueue_prediction/2 is marked `@doc` false but declared public; make it private to reflect intended internal use by changing def to defp for maybe_enqueue_prediction/2 and ensure any callers use the private function only from within the same module (verify no external calls to maybe_enqueue_prediction/2); keep the current clause heads (maybe_enqueue_prediction(:complete, invoice) delegating to enqueue_prediction(invoice) and the fallback returning :ok) and leave enqueue_prediction/1 visibility as-is.
263-277: Mixed delegation pattern for Extraction functions.Some Extraction functions use
defdelegate(lines 263-266) whilerecalculate_extraction_statushas a wrapper function with its own@docand@spec(lines 268-277). This is acceptable since the wrapper adds facade-level documentation, but consider usingdefdelegateconsistently if the documentation can be inherited.🤖 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 263 - 277, The file mixes defdelegate for several Extraction functions and a manual wrapper for recalculate_extraction_status (the function name recalculate_extraction_status/2) with its own `@doc` and `@spec`; either convert recalculate_extraction_status to a defdelegate to match the others (defdelegate recalculate_extraction_status(invoice, attrs), to: Extraction) or move the `@doc/`@spec into the Extraction module and keep a simple defdelegate here; update references to Invoice.t() in the Extraction module spec if you move the spec so types remain correct and ensure Extraction.recalculate_extraction_status/2 is public.lib/ksef_hub/invoices/classification.ex (2)
184-201: Consider adding access control tolist_project_tags/1.Unlike
list_distinct_tags/3which appliesAccessControl.maybe_filter_by_access(opts), thelist_project_tags/1function doesn't filter by access control. If project tags on restricted invoices should not be visible to all users, this could leak information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/classification.ex` around lines 184 - 201, list_project_tags/1 currently returns project_tag values without applying access control and can leak tags from restricted invoices; change the function to accept an opts parameter (e.g., list_project_tags(company_id, opts \\ [])), build the Ecto query exactly as before, then pass the query through AccessControl.maybe_filter_by_access(opts) (the same helper used by list_distinct_tags/3) before calling Repo.all(), and update any call sites accordingly so tag results are filtered by user access.
266-273: Consider preloading category to avoid N+1 query.The
current_category_name/1function at lines 269-270 performs a database query when the invoice has acategory_idbut thecategoryassociation isn't loaded. Ifset_invoice_category/3is called frequently with non-preloaded invoices, this could cause N+1 queries.Since this is for metadata tracking in activity logs, the current approach is acceptable but could be optimized if it becomes a bottleneck.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/classification.ex` around lines 266 - 273, current_category_name/1 can trigger N+1 queries when invoices lack a preloaded :category; fix by ensuring invoices are preloaded before calling set_invoice_category/3 or make current_category_name/1 check the association load state: use Ecto.assoc_loaded?(invoice.category) to return the name when loaded and only fall back to the Category |> where(...) |> select(...) |> Repo.one() query when the assoc is not loaded (or alternatively return nil to avoid queries); update callers (e.g., set_invoice_category/3) to Repo.preload(invoice, :category) where appropriate or implement the assoc_loaded? guard inside current_category_name/1.lib/ksef_hub/accounts/api_tokens.ex (1)
66-91: Token validation is functional but not timing-attack resistant.The
validate_api_token/1function uses direct equality comparison (t.token_hash == ^token_hash) in the database query. While this is standard practice and the hash comparison happens at the database level (which typically uses constant-time comparison), be aware that:
- The early return on
nil(line 78-79) vs checking expiration (lines 81-86) could theoretically leak timing information about whether a token exists.For most applications this is acceptable, but if you need stronger guarantees against timing attacks, consider always performing the same operations regardless of the result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/accounts/api_tokens.ex` around lines 66 - 91, The validate_api_token/1 function can leak timing info because it returns immediately on Repo.one(query) == nil; change it to perform the same work path regardless of a missing record: after computing token_hash with hash_token/1 and calling Repo.one(query), if Repo.one returns nil build a fake ApiToken struct (or placeholder fields) and then run the same expiry check and a constant-time comparison (use Plug.Crypto.secure_compare) between the stored token_hash and the computed token_hash before returning {:ok, api_token} or {:error, :expired}/:invalid; keep the same function names (validate_api_token, hash_token) and still preload/company logic but ensure both found and not-found paths execute the same sequence of operations to reduce timing variance.
🤖 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/accounts.ex`:
- Around line 311-369: Several public functions in this module
(create_api_token/2, create_api_token/3, validate_api_token, revoke_api_token/2,
revoke_api_token/3, list_api_tokens/1, list_api_tokens/2, track_token_usage) are
simple pass-throughs to ApiTokens; replace each plain def with defdelegate to
forward the call to ApiTokens (e.g., defdelegate create_api_token(user_id,
attrs), to: ApiTokens) so the context becomes a thin facade and avoids
duplicating the submodule API surface.
In `@lib/ksef_hub/invoices/access_control.ex`:
- Around line 47-73: The grant_access function can emit
Events.invoice_access_granted even when Repo.insert with on_conflict: :nothing
didn't create a record; update grant_access to only call
Events.invoice_access_granted when the inserted grant is actually persisted
(e.g., check the returned grant struct from Repo.insert in the {:ok, grant}
branch and only call Events.invoice_access_granted if grant.id (or another
persistence marker) is present), otherwise return {:ok, grant} without emitting
the event; alternatively, use Repo.insert(..., returning: true) and then guard
on the returned struct to decide whether to emit the event.
In `@lib/ksef_hub/invoices/analytics.ex`:
- Around line 195-225: The allocation currently uses the raw from date as the
first bucket key; normalize to month starts by computing start_month =
Date.beginning_of_month(from) and end_month = Date.beginning_of_month(to) at the
top of allocate_across_months, call months_between(start_month, end_month)
(instead of months_between(from, to)), and return tuples using those month-start
dates (so the single-month branch returns {start_month, net_amount} and the
multi-month branch maps init_months/last_month which are already month starts).
Update only allocate_across_months (keep months_between as-is).
In `@lib/ksef_hub/invoices/duplicates.ex`:
- Around line 90-104: The function maybe_mark_business_field_duplicate currently
treats the found older invoice as the duplicate and updates it; instead, when
DuplicateDetector.find_original_id/3 returns an older_id you must mark the newly
inserted invoice as duplicate of that older/original invoice. Change the call
that currently invokes mark_as_duplicate(older_invoice, invoice.id,
Event.ksef_sync_opts()) to mark the new invoice as duplicate of the older_id
(e.g., mark_as_duplicate(invoice, older_id, Event.ksef_sync_opts())), and keep
the Repo.get(Invoice, older_id) check only to skip if the original was deleted
(do nothing) — ensure you reference maybe_mark_business_field_duplicate,
DuplicateDetector.find_original_id, Repo.get, Invoice and mark_as_duplicate when
making this fix.
---
Nitpick comments:
In `@lib/ksef_hub/accounts/api_tokens.ex`:
- Around line 66-91: The validate_api_token/1 function can leak timing info
because it returns immediately on Repo.one(query) == nil; change it to perform
the same work path regardless of a missing record: after computing token_hash
with hash_token/1 and calling Repo.one(query), if Repo.one returns nil build a
fake ApiToken struct (or placeholder fields) and then run the same expiry check
and a constant-time comparison (use Plug.Crypto.secure_compare) between the
stored token_hash and the computed token_hash before returning {:ok, api_token}
or {:error, :expired}/:invalid; keep the same function names
(validate_api_token, hash_token) and still preload/company logic but ensure both
found and not-found paths execute the same sequence of operations to reduce
timing variance.
In `@lib/ksef_hub/invoices.ex`:
- Around line 944-948: maybe_enqueue_prediction/2 is marked `@doc` false but
declared public; make it private to reflect intended internal use by changing
def to defp for maybe_enqueue_prediction/2 and ensure any callers use the
private function only from within the same module (verify no external calls to
maybe_enqueue_prediction/2); keep the current clause heads
(maybe_enqueue_prediction(:complete, invoice) delegating to
enqueue_prediction(invoice) and the fallback returning :ok) and leave
enqueue_prediction/1 visibility as-is.
- Around line 263-277: The file mixes defdelegate for several Extraction
functions and a manual wrapper for recalculate_extraction_status (the function
name recalculate_extraction_status/2) with its own `@doc` and `@spec`; either
convert recalculate_extraction_status to a defdelegate to match the others
(defdelegate recalculate_extraction_status(invoice, attrs), to: Extraction) or
move the `@doc/`@spec into the Extraction module and keep a simple defdelegate
here; update references to Invoice.t() in the Extraction module spec if you move
the spec so types remain correct and ensure
Extraction.recalculate_extraction_status/2 is public.
In `@lib/ksef_hub/invoices/classification.ex`:
- Around line 184-201: list_project_tags/1 currently returns project_tag values
without applying access control and can leak tags from restricted invoices;
change the function to accept an opts parameter (e.g.,
list_project_tags(company_id, opts \\ [])), build the Ecto query exactly as
before, then pass the query through AccessControl.maybe_filter_by_access(opts)
(the same helper used by list_distinct_tags/3) before calling Repo.all(), and
update any call sites accordingly so tag results are filtered by user access.
- Around line 266-273: current_category_name/1 can trigger N+1 queries when
invoices lack a preloaded :category; fix by ensuring invoices are preloaded
before calling set_invoice_category/3 or make current_category_name/1 check the
association load state: use Ecto.assoc_loaded?(invoice.category) to return the
name when loaded and only fall back to the Category |> where(...) |> select(...)
|> Repo.one() query when the assoc is not loaded (or alternatively return nil to
avoid queries); update callers (e.g., set_invoice_category/3) to
Repo.preload(invoice, :category) where appropriate or implement the
assoc_loaded? guard inside current_category_name/1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80980b20-d1c9-48a2-986d-f0ef3b24fdb4
📒 Files selected for processing (23)
CLAUDE.mdlib/ksef_hub/accounts.exlib/ksef_hub/accounts/api_tokens.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/access_control.exlib/ksef_hub/invoices/analytics.exlib/ksef_hub/invoices/classification.exlib/ksef_hub/invoices/comments.exlib/ksef_hub/invoices/duplicates.exlib/ksef_hub/invoices/extraction.exlib/ksef_hub/invoices/invoice.exlib/ksef_hub/invoices/queries.exlib/ksef_hub/invoices/reextraction.extest/ksef_hub/accounts/api_tokens_test.exstest/ksef_hub/accounts_test.exstest/ksef_hub/invoices/access_control_test.exstest/ksef_hub/invoices/analytics_test.exstest/ksef_hub/invoices/classification_test.exstest/ksef_hub/invoices/comments_test.exstest/ksef_hub/invoices/duplicates_test.exstest/ksef_hub/invoices/extraction_test.exstest/ksef_hub/invoices/reextraction_test.exstest/ksef_hub/invoices_test.exs
💤 Files with no reviewable changes (1)
- test/ksef_hub/accounts_test.exs
Comment on lines
+90
to
+104
| def maybe_mark_business_field_duplicate(invoice, :inserted) do | ||
| attrs = Map.from_struct(invoice) | ||
|
|
||
| case DuplicateDetector.find_original_id(invoice.company_id, attrs, exclude_id: invoice.id) do | ||
| nil -> | ||
| invoice | ||
|
|
||
| older_id -> | ||
| case Repo.get(Invoice, older_id) do | ||
| nil -> :ok | ||
| older_invoice -> mark_as_duplicate(older_invoice, invoice.id, Event.ksef_sync_opts()) | ||
| end | ||
|
|
||
| invoice | ||
| end |
There was a problem hiding this comment.
Mark the inserted invoice as duplicate, not the original.
DuplicateDetector.find_original_id/3 returns the existing/original invoice id. This branch currently flips the relationship by updating that older row to point at the newly inserted invoice.
🐛 Suggested fix
def maybe_mark_business_field_duplicate(invoice, :inserted) do
attrs = Map.from_struct(invoice)
case DuplicateDetector.find_original_id(invoice.company_id, attrs, exclude_id: invoice.id) do
nil ->
invoice
older_id ->
- case Repo.get(Invoice, older_id) do
- nil -> :ok
- older_invoice -> mark_as_duplicate(older_invoice, invoice.id, Event.ksef_sync_opts())
- end
-
- invoice
+ mark_as_duplicate(invoice, older_id, Event.ksef_sync_opts())
end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/duplicates.ex` around lines 90 - 104, The function
maybe_mark_business_field_duplicate currently treats the found older invoice as
the duplicate and updates it; instead, when DuplicateDetector.find_original_id/3
returns an older_id you must mark the newly inserted invoice as duplicate of
that older/original invoice. Change the call that currently invokes
mark_as_duplicate(older_invoice, invoice.id, Event.ksef_sync_opts()) to mark the
new invoice as duplicate of the older_id (e.g., mark_as_duplicate(invoice,
older_id, Event.ksef_sync_opts())), and keep the Repo.get(Invoice, older_id)
check only to skip if the original was deleted (do nothing) — ensure you
reference maybe_mark_business_field_duplicate,
DuplicateDetector.find_original_id, Repo.get, Invoice and mark_as_duplicate when
making this fix.
- accounts.ex: replace pass-through wrappers with defdelegate - access_control.ex: skip event emission when grant already exists (on_conflict: :nothing was emitting events on idempotent no-ops) - analytics.ex: normalize allocation dates to month starts in allocate_across_months to avoid mid-month bucket keys - invoices.ex: convert recalculate_extraction_status to defdelegate - classification.ex: guard current_category_name with assoc_loaded? to avoid N+1 queries when category is not preloaded Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
lib/ksef_hub/invoices.ex (3)
263-268: Consider adding@doc falseor documenting delegated functions.These
defdelegatecalls expose functions to the public API without accompanying@docor@specannotations. Per coding guidelines, every public function should have documentation. Either:
- Add
@doc falseif these are internal APIs- Add
@docand@specbefore each delegate for true public APIs📝 Example for one function
+ `@doc` "Computes the billing date from invoice attributes (uses sales_date or issue_date)." + `@spec` compute_billing_date(map()) :: Date.t() | nil defdelegate compute_billing_date(attrs), to: Extraction🤖 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 263 - 268, These defdelegate functions (compute_billing_date, missing_critical_fields, determine_extraction_status_from_attrs, populate_company_fields, recalculate_extraction_status) are public but lack docs/specs; either mark them internal by adding `@doc` false above each defdelegate, or add proper `@doc` and `@spec` annotations describing their purpose and types before each defdelegate (e.g., provide a short docstring and a type spec matching Extraction.* functions) to satisfy the guideline.
1248-1265: Consider adding documentation for delegated comment and access control functions.These
defdelegatecalls expose comment and access control operations without@docannotations. For a public API facade, consider either documenting them or marking as@doc falseif they're internal.🤖 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 1248 - 1265, Add documentation metadata for the delegated comment and access-control APIs: for each defdelegate group (list_invoice_comments, create_invoice_comment, update_invoice_comment, delete_invoice_comment) and (list_access_grants, grant_access, revoke_access, set_access_restricted) either add an `@doc` "..." with a short description of the facade function or mark them `@doc` false if they are internal-only; place the `@doc/`@doc false immediately above the defdelegate lines so the generated module docs clearly reflect the intended public/internal surface.
1093-1107: Minor: Consider usingdefdelegatefor consistency.These wrapper functions could be replaced with
defdelegatefor consistency with other analytics delegations, sincedefdelegatesupports default arguments:♻️ Suggested refactor
- def expense_monthly_totals(company_id, filters \\ %{}) do - Analytics.expense_monthly_totals(company_id, filters) - end + defdelegate expense_monthly_totals(company_id, filters \\ %{}), to: Analytics ... - def expense_by_category(company_id, filters \\ %{}) do - Analytics.expense_by_category(company_id, filters) - end + defdelegate expense_by_category(company_id, filters \\ %{}), to: Analytics🤖 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 1093 - 1107, Replace the two thin wrapper functions expense_monthly_totals/2 and expense_by_category/2 with defdelegate calls to Analytics.expense_monthly_totals/2 and Analytics.expense_by_category/2 respectively, preserving the default argument behavior (filters \\ %{}). Update the module to use defdelegate for both functions so they delegate directly to the Analytics module while keeping the same specs and docs (keep `@spec` and `@doc` for expense_by_category if needed).lib/ksef_hub/invoices/classification.ex (1)
203-243: Consider usingTrackedRepofor prediction status updates.Lines 220 and 295 use
Repo.update/1andRepo.update!/1instead ofTrackedRepo. Per coding guidelines, context mutations affecting user-visible state should emit activity events. If prediction status changes are intentionally not tracked, a brief comment explaining why would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/classification.ex` around lines 203 - 243, Replace direct Repo updates for prediction status with the tracked variant so activity events are emitted: in mark_prediction_manual/1 swap Repo.update/1 for TrackedRepo.update/1, and in do_mark_prediction_manual_in_txn!/1 (the transactional helper called by with_manual_prediction/2) swap Repo.update!/1 for TrackedRepo.update!/1; if the omission of TrackedRepo was intentional, add a short comment above both mark_prediction_manual/1 and do_mark_prediction_manual_in_txn!/1 explaining why these prediction-status changes should not emit activity events.
🤖 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/analytics.ex`:
- Around line 113-116: The current where clause uses i.billing_date_from <=
^current_month_start which wrongly excludes invoices that start mid-month;
change that comparison to i.billing_date_from <= ^current_month_end (or an
equivalent end-of-month date) so invoices with billing_date_from within the
current month are included, and ensure you compute and pass a proper
current_month_end (e.g., end of current month via Date.end_of_month/1 or your
date util) alongside the existing last_month_start/current_month_start variables
in the query building code in the KsefHub.Invoices.Analytics module.
---
Nitpick comments:
In `@lib/ksef_hub/invoices.ex`:
- Around line 263-268: These defdelegate functions (compute_billing_date,
missing_critical_fields, determine_extraction_status_from_attrs,
populate_company_fields, recalculate_extraction_status) are public but lack
docs/specs; either mark them internal by adding `@doc` false above each
defdelegate, or add proper `@doc` and `@spec` annotations describing their purpose
and types before each defdelegate (e.g., provide a short docstring and a type
spec matching Extraction.* functions) to satisfy the guideline.
- Around line 1248-1265: Add documentation metadata for the delegated comment
and access-control APIs: for each defdelegate group (list_invoice_comments,
create_invoice_comment, update_invoice_comment, delete_invoice_comment) and
(list_access_grants, grant_access, revoke_access, set_access_restricted) either
add an `@doc` "..." with a short description of the facade function or mark them
`@doc` false if they are internal-only; place the `@doc/`@doc false immediately
above the defdelegate lines so the generated module docs clearly reflect the
intended public/internal surface.
- Around line 1093-1107: Replace the two thin wrapper functions
expense_monthly_totals/2 and expense_by_category/2 with defdelegate calls to
Analytics.expense_monthly_totals/2 and Analytics.expense_by_category/2
respectively, preserving the default argument behavior (filters \\ %{}). Update
the module to use defdelegate for both functions so they delegate directly to
the Analytics module while keeping the same specs and docs (keep `@spec` and `@doc`
for expense_by_category if needed).
In `@lib/ksef_hub/invoices/classification.ex`:
- Around line 203-243: Replace direct Repo updates for prediction status with
the tracked variant so activity events are emitted: in mark_prediction_manual/1
swap Repo.update/1 for TrackedRepo.update/1, and in
do_mark_prediction_manual_in_txn!/1 (the transactional helper called by
with_manual_prediction/2) swap Repo.update!/1 for TrackedRepo.update!/1; if the
omission of TrackedRepo was intentional, add a short comment above both
mark_prediction_manual/1 and do_mark_prediction_manual_in_txn!/1 explaining why
these prediction-status changes should not emit activity events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35d53f3b-0222-40e8-b166-a66570d51427
📒 Files selected for processing (5)
lib/ksef_hub/accounts.exlib/ksef_hub/invoices.exlib/ksef_hub/invoices/access_control.exlib/ksef_hub/invoices/analytics.exlib/ksef_hub/invoices/classification.ex
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ksef_hub/invoices/access_control.ex
- analytics.ex: fix billing_date_from filter using current_month_end instead of current_month_start so mid-month invoices are included - invoices.ex: add @doc false to all facade defdelegate functions (docs live in the sub-modules) - classification.ex: add comments explaining intentional use of Repo (not TrackedRepo) for prediction_status — Trackable returns :skip Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI (Elixir 1.18.4/OTP 28) formatter flags the trailing newline before `end`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/ksef_hub/invoices/classification.ex (2)
272-278: Consider adding a clarifying comment for the subtle logic.The logic here is correct but non-obvious: if we reach this clause (line 270 didn't match) and
assoc_loaded?is true, the loaded value must benil(cleared association). A brief comment would help future readers.📝 Suggested comment
defp current_category_name(%Invoice{category_id: id} = invoice) when is_binary(id) do + # If assoc is loaded but line 270 didn't match, category must be nil (cleared) if Ecto.assoc_loaded?(invoice.category) do nil else Category |> where([c], c.id == ^id) |> select([c], c.name) |> Repo.one() end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/classification.ex` around lines 272 - 278, Add a brief clarifying comment inside the current_category_name/1 function explaining the subtle logic: when this clause is reached and Ecto.assoc_loaded?(invoice.category) returns true it means the association was explicitly loaded and is nil (i.e., the category was cleared), so we should return nil; otherwise we query Category by id via Repo.one to fetch the name. Place the comment adjacent to the assoc_loaded? check to make the intent obvious to future readers.
130-143: Simplifywithby removing non-fallible clause.
build_category_attrs/2always returns a map and cannot fail, so using<-is misleading. Move it inside thedoblock.♻️ Suggested simplification
def set_invoice_category(%Invoice{} = invoice, category_id, opts) do - with %Category{} = category <- fetch_company_category(invoice.company_id, category_id), - attrs <- build_category_attrs(category_id, category) do + with %Category{} = category <- fetch_company_category(invoice.company_id, category_id) do + attrs = build_category_attrs(category_id, category) old_name = current_category_name(invoice)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoices/classification.ex` around lines 130 - 143, The with block in set_invoice_category uses a non-fallible clause for build_category_attrs which is misleading; change set_invoice_category to only pattern-match the fallible fetch_company_category call (fetch_company_category(invoice.company_id, category_id)) in the with, then inside the do block call build_category_attrs(category_id, category) to produce attrs, compute merged_meta, and proceed to Invoice.category_changeset(...) |> TrackedRepo.update(...) using the same opts; keep the nil -> {:error, :category_not_in_company} else branch unchanged.
🤖 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/analytics.ex`:
- Around line 148-152: base_aggregation_query/2 currently filters out nil
billing dates but doesn't prevent inverted ranges, so add a sanity predicate to
the query to ensure billing_date_from is less than or equal to billing_date_to;
modify the where clause that references i.billing_date_from and
i.billing_date_to to include i.billing_date_from <= i.billing_date_to (alongside
the existing not is_nil checks) so records with swapped dates are excluded at
query time.
- Around line 159-171: trim_allocations_to_window currently compares
month-bucket dates to raw filter dates and drops buckets when a filter is
mid-month; fix by normalizing billing_date_from and billing_date_to to the first
day of their month before filtering. In trim_allocations_to_window, after
reading from = Map.get(filters, :billing_date_from) and to = Map.get(filters,
:billing_date_to), compute from_month and to_month by converting each date to an
{year, month, _} tuple (Date.to_erl/1) and recreating a Date for day 1
(Date.from_erl!/1 with {year, month, 1}); then use those normalized from_month
and to_month in the Enum.filter calls (comparing &1.billing_date to
from_month/to_month) so month-bucket dates like 2026-04-01 are kept when
billing_date_from is 2026-04-15.
In `@lib/ksef_hub/invoices/classification.ex`:
- Around line 57-66: The tag mutation uses Repo.update_all which bypasses
activity events; replace Repo.update_all with TrackedRepo.update_all (keeping
the same query and set: dynamic(...) arguments) so the change emits activity via
TrackedRepo, and ensure any subsequent reload uses the tracked API if required;
if skipping activity is intentional, add a concise comment above this block
mirroring the style of mark_prediction_manual explaining why we're intentionally
not using TrackedRepo (and reference set_invoice_tags for the standard
behavior).
---
Nitpick comments:
In `@lib/ksef_hub/invoices/classification.ex`:
- Around line 272-278: Add a brief clarifying comment inside the
current_category_name/1 function explaining the subtle logic: when this clause
is reached and Ecto.assoc_loaded?(invoice.category) returns true it means the
association was explicitly loaded and is nil (i.e., the category was cleared),
so we should return nil; otherwise we query Category by id via Repo.one to fetch
the name. Place the comment adjacent to the assoc_loaded? check to make the
intent obvious to future readers.
- Around line 130-143: The with block in set_invoice_category uses a
non-fallible clause for build_category_attrs which is misleading; change
set_invoice_category to only pattern-match the fallible fetch_company_category
call (fetch_company_category(invoice.company_id, category_id)) in the with, then
inside the do block call build_category_attrs(category_id, category) to produce
attrs, compute merged_meta, and proceed to Invoice.category_changeset(...) |>
TrackedRepo.update(...) using the same opts; keep the nil -> {:error,
:category_not_in_company} else branch unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0622984-561a-4aed-872b-adc5f04b1266
📒 Files selected for processing (3)
lib/ksef_hub/invoices.exlib/ksef_hub/invoices/analytics.exlib/ksef_hub/invoices/classification.ex
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ksef_hub/invoices.ex
Comment on lines
+57
to
+66
| Invoice | ||
| |> where([i], i.id == ^invoice.id) | ||
| |> where([i], fragment("NOT ? = ANY(?)", ^trimmed, i.tags)) | ||
| |> where([i], fragment("coalesce(array_length(?, 1), 0) < ?", i.tags, ^Invoice.max_tags())) | ||
| |> Repo.update_all( | ||
| set: [tags: dynamic([i], fragment("array_append(?, ?)", i.tags, ^trimmed))] | ||
| ) | ||
|
|
||
| {:ok, Repo.reload!(invoice)} | ||
| end |
There was a problem hiding this comment.
Tag addition bypasses activity logging.
Using Repo.update_all directly means this mutation won't emit activity events via TrackedRepo, unlike set_invoice_tags which does. This creates inconsistent activity logging between tag operations.
If activity tracking for individual tag additions is intentional to skip (e.g., for performance), consider adding a comment explaining this design decision similar to the one at lines 217-219 for mark_prediction_manual.
As per coding guidelines: "Use TrackedRepo instead of Repo for context mutations to automatically emit activity events."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ksef_hub/invoices/classification.ex` around lines 57 - 66, The tag
mutation uses Repo.update_all which bypasses activity events; replace
Repo.update_all with TrackedRepo.update_all (keeping the same query and set:
dynamic(...) arguments) so the change emits activity via TrackedRepo, and ensure
any subsequent reload uses the tracked API if required; if skipping activity is
intentional, add a concise comment above this block mirroring the style of
mark_prediction_manual explaining why we're intentionally not using TrackedRepo
(and reference set_invoice_tags for the standard behavior).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- analytics.ex: exclude inverted billing date ranges in base query - analytics.ex: normalize filter dates to month starts in trim_allocations_to_window so mid-month filters keep valid buckets - classification.ex: add comment explaining intentional Repo.update_all in add_invoice_tag (atomic array_append, TrackedRepo not applicable) - classification.ex: clarify assoc_loaded? guard in current_category_name - classification.ex: replace misleading with clause in set_invoice_category with case (build_category_attrs is non-fallible) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
types/0,statuses/0,sources/0), madecompany_fields/1private, DRY'd duplicated bank field validationsInvoices.function()andAccounts.function()via facade delegatesNew modules
Invoices.ExtractionInvoices.ClassificationInvoices.QueriesInvoices.AnalyticsInvoices.AccessControlInvoices.ReextractionInvoices.CommentsInvoices.DuplicatesAccounts.ApiTokensTest plan
mix compile --warnings-as-errors— cleanmix test— 1,918 tests, 0 failuresmix credo --strict— no issuesmix test --cover— coverage on extracted modules 67-94%, main facades 86-93%🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests