Feat/reviewer expense only#43
Conversation
📝 WalkthroughWalkthroughImplements role-based access restricting reviewers to expense invoices by propagating a role value through authentication, controllers, LiveView, and the invoices context; the context layer enforces filtering (expense-only) for reviewer role and tests/UI updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Controller
participant Auth as API Auth Plug
participant Live as LiveView/Controller
participant Context as KsefHub.Invoices
participant DB as Database
rect rgba(200,200,255,0.5)
Client->>API: Request (with token / session)
API->>Auth: validate_token()
Auth->>Auth: resolve_role(user_id, company_id)
Auth-->>API: assigns :current_role (e.g., "reviewer")
end
rect rgba(200,255,200,0.5)
API->>Context: list/get invoices (company_id, filters, role: "reviewer")
Context->>Context: maybe_scope_type_by_role(role)
Context->>DB: query invoices WHERE company_id AND (type = "expense" if reviewer)
DB-->>Context: rows
Context-->>API: invoices
API-->>Client: response (filtered)
end
rect rgba(255,230,200,0.5)
Client->>Live: LiveView request
Live->>Auth: resolve_role(user_id, company_id)
Live->>Context: list_invoices_paginated(..., role: "reviewer")
Context->>DB: query (scoped)
DB-->>Context: rows
Context-->>Live: paginated result
Live-->>Client: UI (type filter disabled, income hidden)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 3
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/controllers/invoice_pdf_controller.ex`:
- Around line 50-62: The resolve_role/2 function in invoice_pdf_controller.ex
duplicates logic found elsewhere; extract this into a shared helper module
(e.g., AuthHelpers.resolve_role/2) that takes the Plug.Conn and company_id,
calls conn.assigns[:current_user] and Companies.get_membership(user_id,
company_id) and returns the role or nil, then replace the local resolve_role/2
implementation in InvoicePdfController (and other modules) with a call to the
new shared function to centralize the logic and avoid duplication.
In `@lib/ksef_hub_web/live/invoice_live/index.ex`:
- Around line 153-163: Replace the raw <form> and direct assigns with the
Phoenix HTML form helper pattern: in the live view assign function (e.g.,
filter_assigns/3) build a form with form = to_form(filters, as: :filters) and
include it in assigns (form: form) instead of raw filter fields; then update the
template to use the component <.form for={`@form`} phx-change="filter" ...> and
replace direct name/selected usage with `@form`[:type].name, `@form`[:type].id and
`@form`[:type].value for each select/input (handle the reviewer-disabled select by
using the same `@form`[:type] attributes plus disabled). Ensure all other filter
fields follow the same `@form`[:field] attribute access.
In `@test/support/api_test_helpers.ex`:
- Around line 41-70: The create_reviewer_with_token function should use a with
chain to safely sequence Accounts.create_api_token and the membership role
downgrade instead of calling update!; replace the current direct pattern-match
and update! with a with that calls Accounts.create_api_token(user.id,
company.id, ...) and then Ecto.Changeset.change(membership, role: "reviewer") |>
KsefHub.Repo.update, propagating {:ok, ...} and {:error, reason} cases; update
the function spec/signature to return {:ok, %{user: ..., company: ..., token:
..., api_token: ...}} | {:error, term()} so callers can handle failures.
🧹 Nitpick comments (2)
lib/ksef_hub_web/plugs/api_auth.ex (1)
49-55: Extract role resolution into a shared helper.
resolve_role/2duplicates logic already present inKsefHubWeb.LiveAuthandInvoicePdfController. Please centralize this in a shared module (e.g.,KsefHub.Companiesor aKsefHubWeb.Authhelper) and call it from each place to avoid drift.
As per coding guidelines: “Extract shared logic into dedicated modules instead of copy-pasting.”test/ksef_hub_web/live/invoice_live/show_test.exs (1)
128-160: Move default stub to the describe setup.These tests use
stub/3for a default return; per guideline, place that in thesetupblock for the reviewer describe to keep the test bodies focused.♻️ Suggested refactor
describe "reviewer role" do setup %{conn: _conn} do @@ conn = build_conn() |> log_in_user(reviewer, %{current_company_id: company.id}) - %{conn: conn, company: company} + stub(KsefHub.Pdf.Mock, :generate_html, fn _xml, _meta -> {:error, :no_xml} end) + %{conn: conn, company: company} end @@ - stub(KsefHub.Pdf.Mock, :generate_html, fn _xml, _meta -> {:error, :no_xml} end) - {:ok, _view, html} = live(conn, ~p"/invoices/#{invoice.id}") @@ - stub(KsefHub.Pdf.Mock, :generate_html, fn _xml, _meta -> {:error, :no_xml} end) - assert {:error, {:redirect, %{to: "/invoices"}}} = live(conn, ~p"/invoices/#{invoice.id}")As per coding guidelines: “Use expect/3 for specific call expectations in tests; use stub/3 for default returns in setup blocks.”
Reviewers should not see income invoices. Defines enforcement at the context layer with role passed from web layer, covering LiveView, API, and download endpoints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add role-aware filtering to the Invoices context layer so all access paths (LiveView, API, browser downloads) consistently restrict reviewers to expense invoices. The context accepts an optional `role:` keyword; when "reviewer", it forces `type: "expense"` on list queries and adds a type constraint on single-invoice lookups. - Invoices context: add opts keyword with :role to list/get functions - API auth plug: resolve current_role from token creator's membership - LiveView index/show: pass current_role to context calls - API controller: pass current_role to all context calls - PDF controller: resolve role from session user's membership - Dashboard: hide income stats for reviewers Implements ADR 0016. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover all access paths for the reviewer-only-expense constraint: - Context: list, count, get, get! with role: "reviewer" - LiveView index: reviewer sees only expense, type=income param ignored - LiveView show: reviewer redirected on income invoice - API controller: reviewer token scoped to expense invoices - API auth plug: current_role resolved from token creator's membership Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add default xml_content to invoice factory to satisfy NOT NULL constraint - Fix create_reviewer_with_token to create as owner first then downgrade, since token creation requires owner role Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The xml_content field was defined twice — a stale "<Faktura/>" stub at the top and the correct File.read! fixture further down. Since Elixir maps take the last key, the stub was silently ignored. Remove it to avoid confusion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract resolve_role/2 into KsefHubWeb.AuthHelpers shared module, removing duplicate private functions from InvoicePdfController, Plugs.ApiAuth, and LiveAuth - Replace raw HTML form with Phoenix to_form in InvoiceLive.Index filter form, removing individual filter assigns in favor of @Form - Refactor create_reviewer_with_token to use with chain and Repo.update instead of Repo.update!, returning {:ok, map} tuples - Move Pdf.Mock stub into reviewer describe setup block in show_test - Resolve merge conflict in invoice_controller_test.exs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
emilwojtaszek
force-pushed
the
feat/reviewer-expense-only
branch
from
455ddd2 to
d36daa5
Compare
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_web/controllers/api/invoice_controller.ex (1)
262-267:⚠️ Potential issue | 🟠 MajorMissing role parameter in
xmlaction creates authorization bypass.The
xmlaction does not passroletoget_invoice!/3, unlike all other actions (show,approve,reject,html,🔒 Proposed fix to add role parameter
def xml(conn, %{"id" => id}) do company_id = conn.assigns.current_company.id - invoice = Invoices.get_invoice!(company_id, id) + invoice = Invoices.get_invoice!(company_id, id, role: conn.assigns[:current_role]) send_attachment(conn, "application/xml", "#{invoice.invoice_number}.xml", invoice.xml_content) end
🧹 Nitpick comments (3)
docs/adr/0016-reviewer-expense-only-visibility.md (1)
35-35: Update function arities in documentation.The ADR references
list_invoices_paginated/2andget_invoice/2, but the implementation uses 3-arity versions with anoptskeyword list parameter. Consider updating to reflect the actual API:-1. **Context layer (`KsefHub.Invoices`)** — `list_invoices_paginated/2` and `get_invoice/2` accept a `role` parameter. +1. **Context layer (`KsefHub.Invoices`)** — `list_invoices_paginated/3` and `get_invoice/3` accept a `role` option via the `opts` keyword list.lib/ksef_hub_web/live/invoice_live/index.ex (1)
171-178: Disabled select won't submit value — verify filter behavior.A disabled
<select>element doesn't submit its value with the form. When a reviewer triggers thephx-change="filter"event,params["type"]will benilor missing. This should be fine sincescope_by_role/2in the context layer enforcestype: "expense"regardless, but the filter URL won't includetype=expense.If you want the URL to explicitly show
?type=expensefor reviewers (for clarity/bookmarking), consider usingreadonlystyling with a hidden input instead, or simply accept the current behavior where the backend enforces the constraint silently.lib/ksef_hub/invoices.ex (1)
73-88: Consider passing opts to internal count_invoices call for consistency.On line 78,
count_invoices(company_id, filters)is called without theopts(includingrole). This works becausefiltersis already scoped byscope_by_role/2on line 74, and callingcount_invoices/3with default opts appliesscope_by_role(filters, nil)which is a no-op.However, for clarity and to avoid subtle bugs if
scope_by_rolebehavior changes in the future, consider explicitly passing the same opts:- total_count = count_invoices(company_id, filters) + total_count = count_invoices(company_id, filters, opts)This would technically apply
scope_by_roletwice (idempotent), but makes the intent clearer.
Reviewers could download XML of income invoices via the xml endpoint because it was the only action not passing the role option to get_invoice!/3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Update ADR 0016 to reference /3 arities with opts keyword list - Pass opts through to count_invoices in list_invoices_paginated for consistency, ensuring role scoping is explicit at every call site Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores