feat: relax NIP uniqueness to per-user and add certificate warning banner#108
Merged
Conversation
…nner Drop the global unique index on companies.nip to prevent NIP squatting — different users can now create companies with the same NIP. Per-user uniqueness is enforced at the application level in create_company_with_owner. Also add a warning banner on the invoice index page when the company has no certificate configured, linking to Settings → Certificates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoved DB-level unique index on company NIP and moved duplicate-NIP prevention for the same user into a pre-check in company creation. Invoice index LiveView now sets a Changes
Sequence Diagram(s)mermaid 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 docstrings
🧪 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: 1
🧹 Nitpick comments (2)
test/ksef_hub_web/live/invoice_live/index_test.exs (1)
47-52: Prefer asserting the certificate-settings link target explicitly.Asserting
"Settings"/"Certificates"is a bit broad; consider asserting the/settings/certificatesanchor to avoid incidental matches.Suggested assertion tweak
- {:ok, _view, html} = live(conn, ~p"/c/#{company.id}/invoices") + {:ok, view, html} = live(conn, ~p"/c/#{company.id}/invoices") assert html =~ "KSeF sync not configured" - assert html =~ "Settings" - assert html =~ "Certificates" + assert has_element?(view, ~s(a[href="/c/#{company.id}/settings/certificates"]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/index_test.exs` around lines 47 - 52, The test "shows warning when company has no certificate" currently asserts generic text "Settings" and "Certificates"; instead assert the specific link target to avoid accidental matches by checking the settings certificates path returned in the rendered HTML (use the same company id used in live(conn, ~p"/c/#{company.id}/invoices") and assert the HTML contains the settings certificates URL, e.g. the /c/#{company.id}/settings/certificates anchor); update the two broad assertions to a single assertion that matches that exact settings certificates link target.lib/ksef_hub_web/live/invoice_live/index.ex (1)
16-33: Add required@docand@specfor the updated public callbacks.
mount/3andrender/1were modified but still don’t include the required docs/typespecs for this module path.As per coding guidelines,
lib/**/*.ex: Every public function must include@docdocumentation in Elixir; Every function must include@spectypespec in Elixir (public and private).Also applies to: 295-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/index.ex` around lines 16 - 33, Add missing `@doc` and `@spec` for the public LiveView callbacks mount/3 and render/1: add a brief `@doc` describing what mount/3 does (initial assigns: page_title, categories, all_tags, has_certificate) and a `@spec` mount(params :: map() | nil, session :: map(), socket :: Phoenix.LiveView.Socket.t()) :: {:ok, Phoenix.LiveView.Socket.t()} and similarly add `@doc` for render/1 and a `@spec` render(assigns :: map()) :: Phoenix.LiveView.Rendered.t(); place these annotations directly above the mount/3 and render/1 function definitions (referenced by their names mount and render) to satisfy the module-wide public function documentation and typespec requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ksef_hub/companies_test.exs`:
- Around line 311-322: The test currently calls
Companies.update_company(company, %{is_active: false}) without asserting its
return value, which can hide failures; change that call to assert the expected
success tuple (for example, assert {:ok, _updated_company} =
Companies.update_company(company, %{is_active: false})) so the deactivation
failure surfaces during the test and references the Companies.update_company/2
call and the company variable used earlier in the test.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/index.ex`:
- Around line 16-33: Add missing `@doc` and `@spec` for the public LiveView
callbacks mount/3 and render/1: add a brief `@doc` describing what mount/3 does
(initial assigns: page_title, categories, all_tags, has_certificate) and a `@spec`
mount(params :: map() | nil, session :: map(), socket ::
Phoenix.LiveView.Socket.t()) :: {:ok, Phoenix.LiveView.Socket.t()} and similarly
add `@doc` for render/1 and a `@spec` render(assigns :: map()) ::
Phoenix.LiveView.Rendered.t(); place these annotations directly above the
mount/3 and render/1 function definitions (referenced by their names mount and
render) to satisfy the module-wide public function documentation and typespec
requirement.
In `@test/ksef_hub_web/live/invoice_live/index_test.exs`:
- Around line 47-52: The test "shows warning when company has no certificate"
currently asserts generic text "Settings" and "Certificates"; instead assert the
specific link target to avoid accidental matches by checking the settings
certificates path returned in the rendered HTML (use the same company id used in
live(conn, ~p"/c/#{company.id}/invoices") and assert the HTML contains the
settings certificates URL, e.g. the /c/#{company.id}/settings/certificates
anchor); update the two broad assertions to a single assertion that matches that
exact settings certificates link target.
🪄 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: aa03d8f6-d6ce-4b34-8d4d-0c971a6971e7
📒 Files selected for processing (6)
lib/ksef_hub/companies.exlib/ksef_hub/companies/company.exlib/ksef_hub_web/live/invoice_live/index.expriv/repo/migrations/20260331212731_relax_nip_unique_constraint.exstest/ksef_hub/companies_test.exstest/ksef_hub_web/live/invoice_live/index_test.exs
💤 Files with no reviewable changes (1)
- lib/ksef_hub/companies/company.ex
- Assert return value of update_company in deactivation test - Add @doc/@SPEC to mount/3 and render/1 in InvoiceLive.Index - Assert exact certificate settings link path instead of generic text Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/ksef_hub/companies_test.exs (2)
311-322: Good fix on deactivation assertion; consider asserting identity and NIP reuse too.Line 316 now correctly asserts
update_company/2success. You can further harden the test by proving a new record is created with the same NIP.Proposed assertion hardening
assert {:ok, %{company: new_company}} = Companies.create_company_with_owner(user, %{name: "New Co", nip: "1111111111"}) assert new_company.name == "New Co" + assert new_company.nip == "1111111111" + assert new_company.id != company.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/companies_test.exs` around lines 311 - 322, The test currently checks deactivation succeeded but doesn't assert that a new DB record was created and that the NIP was reused; after calling Companies.update_company(company, %{is_active: false}) and creating the new company with Companies.create_company_with_owner, add assertions that new_company.id != company.id (to prove a distinct record was created) and that new_company.nip == company.nip (or equals the literal "1111111111") to confirm NIP reuse, keeping the references to the company and new_company variables and the Companies.create_company_with_owner / Companies.update_company functions.
290-297: Strengthen assertions to prove records are distinct per user.Right now both calls only assert success. Adding identity checks would prevent false positives if the implementation ever reuses one company record.
Proposed assertion hardening
- assert {:ok, %{company: _c1}} = Companies.create_company_with_owner(user_a, attrs) - assert {:ok, %{company: _c2}} = Companies.create_company_with_owner(user_b, attrs) + assert {:ok, %{company: c1, membership: m1}} = Companies.create_company_with_owner(user_a, attrs) + assert {:ok, %{company: c2, membership: m2}} = Companies.create_company_with_owner(user_b, attrs) + assert c1.id != c2.id + assert c1.nip == "1111111111" + assert c2.nip == "1111111111" + assert m1.user_id == user_a.id + assert m2.user_id == user_b.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub/companies_test.exs` around lines 290 - 297, The test currently only asserts success for two create calls; strengthen it to verify the two created company records are distinct and owned by the correct users by capturing the returned companies from Companies.create_company_with_owner (e.g., {:ok, %{company: c1}} and {:ok, %{company: c2}}) and then assert c1.id != c2.id and c1.owner_id == user_a.id and c2.owner_id == user_b.id (or equivalent fields) to ensure separate records per user.test/ksef_hub_web/live/invoice_live/index_test.exs (2)
58-58: Consider explicitly settingis_active: truefor clarity.The
get_certificate_for_company/1function filters byis_active == true. While the factory likely sets this by default, making it explicit documents the test's intent and guards against factory changes:insert(:user_certificate, user: user, is_active: true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/index_test.exs` at line 58, The test inserts a user certificate without explicitly marking it active while get_certificate_for_company/1 filters on is_active == true; update the test to call insert(:user_certificate, user: user, is_active: true) so the created certificate clearly satisfies get_certificate_for_company/1’s condition and documents the intent (look for the insert(:user_certificate, user: user) line and the get_certificate_for_company/1 usage).
46-62: Prefer element-based assertions over raw HTML string matching.Per project learnings, LiveView tests should use
has_element?/2orelement/2with selectors rather thanhtml =~string matching. This makes tests more resilient to text or markup changes.Consider adding a
data-testidattribute to the warning banner in the template, then use:assert has_element?(view, "[data-testid=certificate-warning-banner]") assert has_element?(view, ~s{a[href="/c/#{company.id}/settings/certificates"]})This approach aligns with the project guidance to "favor testing for the presence of key elements" and "never test against raw HTML."
Based on learnings: "Instead of relying on testing text content which can change, favor testing for the presence of key elements in LiveView tests" and "Never test against raw HTML - always use
element/2,has_element/2, and similar functions with selectors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ksef_hub_web/live/invoice_live/index_test.exs` around lines 46 - 62, Replace raw HTML string assertions with element-based assertions: capture the LiveView mount as {:ok, view, _html} instead of {:ok, _view, html} in both tests and replace `assert html =~ "KSeF sync not configured"` and the href string check with `assert has_element?(view, "[data-testid=certificate-warning-banner]")` (add that data-testid to the banner in the template) and `assert has_element?(view, ~s{a[href="/c/#{company.id}/settings/certificates"]})`; in the second test use `refute has_element?(view, "[data-testid=certificate-warning-banner]")` after inserting the certificate. Ensure you reference `view` (not `_view` or `html`) and add the data-testid in the banner component/template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/ksef_hub_web/live/invoice_live/index_test.exs`:
- Line 58: The test inserts a user certificate without explicitly marking it
active while get_certificate_for_company/1 filters on is_active == true; update
the test to call insert(:user_certificate, user: user, is_active: true) so the
created certificate clearly satisfies get_certificate_for_company/1’s condition
and documents the intent (look for the insert(:user_certificate, user: user)
line and the get_certificate_for_company/1 usage).
- Around line 46-62: Replace raw HTML string assertions with element-based
assertions: capture the LiveView mount as {:ok, view, _html} instead of {:ok,
_view, html} in both tests and replace `assert html =~ "KSeF sync not
configured"` and the href string check with `assert has_element?(view,
"[data-testid=certificate-warning-banner]")` (add that data-testid to the banner
in the template) and `assert has_element?(view,
~s{a[href="/c/#{company.id}/settings/certificates"]})`; in the second test use
`refute has_element?(view, "[data-testid=certificate-warning-banner]")` after
inserting the certificate. Ensure you reference `view` (not `_view` or `html`)
and add the data-testid in the banner component/template.
In `@test/ksef_hub/companies_test.exs`:
- Around line 311-322: The test currently checks deactivation succeeded but
doesn't assert that a new DB record was created and that the NIP was reused;
after calling Companies.update_company(company, %{is_active: false}) and
creating the new company with Companies.create_company_with_owner, add
assertions that new_company.id != company.id (to prove a distinct record was
created) and that new_company.nip == company.nip (or equals the literal
"1111111111") to confirm NIP reuse, keeping the references to the company and
new_company variables and the Companies.create_company_with_owner /
Companies.update_company functions.
- Around line 290-297: The test currently only asserts success for two create
calls; strengthen it to verify the two created company records are distinct and
owned by the correct users by capturing the returned companies from
Companies.create_company_with_owner (e.g., {:ok, %{company: c1}} and {:ok,
%{company: c2}}) and then assert c1.id != c2.id and c1.owner_id == user_a.id and
c2.owner_id == user_b.id (or equivalent fields) to ensure separate records per
user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2903dc51-4ee7-4f80-b8ba-6a6f0dc33fc4
📒 Files selected for processing (3)
lib/ksef_hub_web/live/invoice_live/index.extest/ksef_hub/companies_test.exstest/ksef_hub_web/live/invoice_live/index_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ksef_hub_web/live/invoice_live/index.ex
- Use has_element?/2 with data-testid for banner tests instead of HTML string matching - Add explicit is_active: true on user_certificate insert to document intent - Assert distinct record IDs and NIP reuse in uniqueness tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 4, 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
create_company_with_owner/2. Same user cannot create two active companies with the same NIP.Test plan
mix ecto.migrate)mix format --check-formattedandmix credo --strictclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores