Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## rules #795 +/- ##
=======================================
Coverage 93.08% 93.08%
=======================================
Files 368 368
Lines 19592 19604 +12
=======================================
+ Hits 18237 18249 +12
Misses 919 919
Partials 436 436 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pmenendz
left a comment
pmenendz
left a comment
There was a problem hiding this comment.
Looks good, but you need to get that test coverage up
There was a problem hiding this comment.
Pull request overview
This PR tightens validation for the Polish FA_VAT (KSeF) addon by requiring invoice notes to include an identifying key or code, preventing submission issues caused by “anonymous” notes.
Changes:
- Added per-note validation on
bill.Invoice.Notesto requirekeyorcode. - Introduced a helper validator (
noteHasKeyOrCode) used by the invoice rules. - Added tests covering valid “code-only” notes and invalid notes missing both key and code.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
addons/pl/favat/bill.go |
Adds invoice-level notes validation and a helper predicate for enforcing key/code presence. |
addons/pl/favat/bill_test.go |
Adds test cases verifying the new notes validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func noteHasKeyOrCode(val any) bool { | ||
| n, ok := val.(*org.Note) | ||
| if !ok || n == nil { | ||
| return true | ||
| } | ||
| return n.Key != "" || n.Code != "" | ||
| } |
There was a problem hiding this comment.
noteHasKeyOrCode currently returns true for a nil *org.Note. That means an invoice unmarshaled with notes: [null] would pass this new per-note check, but will still panic during validation because invoiceExemptionNotePresent / invoiceSingleExemptionNote call isExemptionNote(note) without a nil-guard (dereferencing note.Key). Consider treating nil note elements as invalid here (return false when n == nil) and/or adding nil-checks where notes are iterated so validation never panics on null notes.
| t.Run("note with only code is valid", func(t *testing.T) { | ||
| inv := standardInvoice() | ||
| inv.Notes = []*org.Note{ | ||
| { | ||
| Code: "ABC", | ||
| Text: "Note with code only", | ||
| }, | ||
| } | ||
| require.NoError(t, inv.Calculate()) | ||
| err := rules.Validate(inv) | ||
| assert.NoError(t, err) | ||
| }) | ||
|
|
||
| t.Run("note without key or code is invalid", func(t *testing.T) { | ||
| inv := standardInvoice() | ||
| inv.Notes = []*org.Note{ | ||
| { | ||
| Text: "Note without key or code", | ||
| }, | ||
| } | ||
| require.NoError(t, inv.Calculate()) | ||
| err := rules.Validate(inv) | ||
| assert.ErrorContains(t, err, "note key or code must be set") | ||
| }) |
There was a problem hiding this comment.
Add a regression test for notes containing a nil element (e.g. inv.Notes = []*org.Note{nil} or unmarshaled notes:[null]) to ensure rules.Validate(inv) does not panic and instead returns a validation error. This guards against nil-pointer panics in note-related validators.
Thanks, all covered now! |
samlown
left a comment
samlown
left a comment
There was a problem hiding this comment.
A few changes required.
| func noteHasKeyOrCode(val any) bool { | ||
| n, ok := val.(*org.Note) | ||
| if !ok || n == nil { | ||
| return true |
There was a problem hiding this comment.
This should return false. If the method doesn't know what its dealing with, it should assume false by default.
| t.Run("note with only code is valid", func(t *testing.T) { | ||
| inv := standardInvoice() | ||
| inv.Notes = []*org.Note{ | ||
| { | ||
| Code: "ABC", | ||
| Text: "Note with code only", | ||
| }, | ||
| } | ||
| require.NoError(t, inv.Calculate()) | ||
| err := rules.Validate(inv) | ||
| assert.NoError(t, err) | ||
| }) | ||
|
|
||
| t.Run("note without key or code is invalid", func(t *testing.T) { | ||
| inv := standardInvoice() | ||
| inv.Notes = []*org.Note{ | ||
| { | ||
| Text: "Note without key or code", | ||
| }, | ||
| } | ||
| require.NoError(t, inv.Calculate()) | ||
| err := rules.Validate(inv) | ||
| assert.ErrorContains(t, err, "note key or code must be set") | ||
| }) |
| func noteHasKeyOrCode(val any) bool { | ||
| n, ok := val.(*org.Note) | ||
| if !ok || n == nil { | ||
| return true | ||
| } | ||
| return n.Key != "" || n.Code != "" | ||
| } |
Require key or code in notes for Poland to avoid issues in KSeF
Pre-Review Checklist
go generate .to ensure that the Schemas and Regime data are up to date.And if you are part of the org: