Feat/add payroll support #1
Open
+6,798
−327
Conversation
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
|
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:
📝 WalkthroughWalkthroughAdds a centralized date-parsing utility, a new stamp-transaction domain and service, employee/employer subservices, extensive payroll and invoice model expansions (BigDecimal migration and new POJOs), consolidates date handling across download/xml models, adds documentation, and bumps the project version. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant ClientLib as FiscalApiClient
participant StampSvc as StampService
participant HttpClient as IFiscalApiHttpClient
participant API as RemoteAPI
Client->>ClientLib: getStampService().transferStamps(params)
ClientLib->>StampSvc: transferStamps(params)
StampSvc->>HttpClient: POST /api/.../stamps (params)
HttpClient->>API: outbound HTTP request
API-->>HttpClient: HTTP response (ApiResponse<Boolean>)
HttpClient-->>StampSvc: ApiResponse<Boolean>
StampSvc-->>ClientLib: ApiResponse<Boolean>
ClientLib-->>Client: ApiResponse<Boolean>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds payroll (“nómina”) and local taxes complements plus stamp (timbres) transaction support to the Java SDK, while also refactoring several DTOs for BigDecimal/date parsing and extending the client/service surface area.
Changes:
- Introduces payroll/local-taxes complement DTOs and wires them into
Invoice/issuer/recipient models. - Adds stamp transactions models +
StampServiceand exposes it fromFiscalApiClient. - Refactors multiple money/date fields (BigDecimal + shared date parsing helper) and updates examples accordingly.
Reviewed changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/fiscalapi/services/StampService.java | New service for stamps resource, adds transfer/withdraw operations. |
| src/main/java/com/fiscalapi/services/PersonService.java | Exposes employer/employee sub-services off PersonService. |
| src/main/java/com/fiscalapi/services/InvoiceService.java | Simplifies invoice creation endpoint usage. |
| src/main/java/com/fiscalapi/services/FiscalApiClient.java | Registers/exposes new IStampService from the main client. |
| src/main/java/com/fiscalapi/services/EmployerService.java | Adds employer CRUD-like operations under people. |
| src/main/java/com/fiscalapi/services/EmployeeService.java | Adds employee CRUD-like operations under people. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollSeverance.java | New payroll severance DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollRetirement.java | New payroll retirement DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarnings.java | Aggregates payroll earnings/other payments/retirement/severance. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarningOvertime.java | New overtime DTO for payroll earning lines. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarningOtherPayment.java | New “other payment” DTO (incl. balance compensation). |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarning.java | New payroll earning line DTO (taxed/exempt + overtime). |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDisability.java | New payroll disability DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDeduction.java | New payroll deduction DTO. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/Payroll.java | New payroll complement root DTO with SAT date formatting. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/EmployerData.java | Employer payroll data attached to issuer. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/EmployeeData.java | Employee payroll data attached to recipient. |
| src/main/java/com/fiscalapi/models/invoicing/payroll/BalanceCompensation.java | New balance compensation DTO used by other payments. |
| src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoiceTax.java | Moves to paymentComplement package + BigDecimal base. |
| src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoice.java | Moves to paymentComplement package + BigDecimal amounts. |
| src/main/java/com/fiscalapi/models/invoicing/paymentComplement/InvoicePayment.java | Moves to paymentComplement package + BigDecimal amounts, shared date parsing. |
| src/main/java/com/fiscalapi/models/invoicing/localTaxes/LocalTaxes.java | New local taxes complement container. |
| src/main/java/com/fiscalapi/models/invoicing/localTaxes/LocalTax.java | New local tax line DTO. |
| src/main/java/com/fiscalapi/models/invoicing/OnBehalfOf.java | New “on behalf of” DTO for invoice items. |
| src/main/java/com/fiscalapi/models/invoicing/InvoiceRecipient.java | Adds employeeData support. |
| src/main/java/com/fiscalapi/models/invoicing/InvoiceItem.java | Adds unitOfMeasurement/onBehalfOf + BigDecimal quantities/prices. |
| src/main/java/com/fiscalapi/models/invoicing/InvoiceIssuer.java | Adds employerData support. |
| src/main/java/com/fiscalapi/models/invoicing/Invoice.java | Adds complement/paymentConditions + BigDecimal monetary fields + shared date parsing. |
| src/main/java/com/fiscalapi/models/invoicing/Complement.java | New complement wrapper (payroll/payment/local taxes). |
| src/main/java/com/fiscalapi/models/downloading/Xml.java | Refactors SAT date parsing to shared util. |
| src/main/java/com/fiscalapi/models/downloading/MetadataItem.java | Refactors SAT date parsing to shared util. |
| src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java | Adds String-based date setters + LocalDateTime alternatives; uses shared util. |
| src/main/java/com/fiscalapi/models/UserLookupDto.java | New lightweight user reference DTO (for stamp transactions). |
| src/main/java/com/fiscalapi/models/StampTransactionType.java | New enum for stamp transaction types. |
| src/main/java/com/fiscalapi/models/StampTransactionStatus.java | New enum for stamp transaction statuses. |
| src/main/java/com/fiscalapi/models/StampTransactionParams.java | New params DTO for stamp transfer/withdraw calls. |
| src/main/java/com/fiscalapi/models/StampTransaction.java | New stamp transaction DTO. |
| src/main/java/com/fiscalapi/models/ProductTax.java | Changes tax rate type to BigDecimal. |
| src/main/java/com/fiscalapi/models/Product.java | Changes unitPrice type to BigDecimal. |
| src/main/java/com/fiscalapi/models/Person.java | Changes balances to BigDecimal. |
| src/main/java/com/fiscalapi/abstractions/IStampService.java | New service interface for stamp operations. |
| src/main/java/com/fiscalapi/abstractions/IPersonService.java | Adds employer/employee service accessors. |
| src/main/java/com/fiscalapi/abstractions/IFiscalApiClient.java | Adds getStampService(). |
| src/main/java/com/fiscalapi/OptUtil.java | New shared SAT date parsing helper. |
| src/main/java/com/fiscalapi/Examples.java | Adds extensive payroll/local taxes/stamps usage examples. |
| pom.xml | Version bump. |
| CLAUDE.md | Adds repository guidance doc for Claude Code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
10
to
61
| @@ -27,17 +30,17 @@ public String getItemCode() { | |||
| public void setItemCode(String itemCode) { | |||
| this.itemCode = itemCode; | |||
| } | |||
| public Double getQuantity() { | |||
| public BigDecimal getQuantity() { | |||
| return quantity; | |||
| } | |||
| public void setQuantity(Double quantity) { | |||
| this.quantity = quantity; | |||
| public void setQuantity(String quantity) { | |||
| this.quantity = new BigDecimal(quantity); | |||
| } | |||
| public Double getDiscount() { | |||
| public BigDecimal getDiscount() { | |||
| return discount; | |||
| } | |||
| public void setDiscount(Double discount) { | |||
| this.discount = discount; | |||
| public void setDiscount(String discount) { | |||
| this.discount = new BigDecimal(discount); | |||
| } | |||
| public String getUnitOfMeasurementCode() { | |||
| return unitOfMeasurementCode; | |||
| @@ -51,11 +54,11 @@ public String getDescription() { | |||
| public void setDescription(String description) { | |||
| this.description = description; | |||
| } | |||
| public Double getUnitPrice() { | |||
| public BigDecimal getUnitPrice() { | |||
| return unitPrice; | |||
| } | |||
| public void setUnitPrice(Double unitPrice) { | |||
| this.unitPrice = unitPrice; | |||
| public void setUnitPrice(String unitPrice) { | |||
| this.unitPrice = new BigDecimal(unitPrice); | |||
There was a problem hiding this comment.
InvoiceItem quantity/discount/unitPrice setters were changed to accept String and parse BigDecimal. This is a breaking API change and will throw on null/empty inputs. Provide BigDecimal (and/or numeric) overloads to preserve SDK ergonomics, and if this is intended for JSON binding prefer Jackson configuration/annotations rather than changing the public setter signatures.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoiceTax.java (1)
34-36:⚠️ Potential issue | 🟡 MinorSame exception risk in
setTaxRate.Apply the same fix as suggested for
setBaseto handle null/malformed input consistently.
🤖 Fix all issues with AI agents
In `@src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java`:
- Line 7: DownloadRequest's String-based setters (setStartDate, setEndDate,
setLastAttemptDate, setNextAttemptDate) call OptUtil.formatInputDateToSATFormat
and end up storing LocalDateTime.MIN for null/blank input; guard the input by
checking if the incoming String is null or blank and set the corresponding field
to null in that case, otherwise call OptUtil.formatInputDateToSATFormat and
assign the parsed value; update the logic inside the four methods
(setStartDate(String), setEndDate(String), setLastAttemptDate(String),
setNextAttemptDate(String)) to perform this null/blank check before parsing so
SAT-invalid -999999999 dates are not produced.
In `@src/main/java/com/fiscalapi/models/downloading/MetadataItem.java`:
- Around line 155-158: setSatInvoiceDate currently assigns the result of
OptUtil.formatInputDateToSATFormat(satDate) directly, but OptUtil returns
LocalDateTime.MIN for null/empty inputs which then gets serialized as a bogus
date; change setSatInvoiceDate to short‑circuit null/empty inputs or check the
returned LocalDateTime and set this.invoiceDate to null when the input is
null/empty or when the formatted value equals LocalDateTime.MIN, otherwise
assign the formatted value; apply the same pattern to the other MetadataItem
setters that call OptUtil.formatInputDateToSATFormat so any SAT date formatter
result equal to LocalDateTime.MIN is treated as unset (null).
In `@src/main/java/com/fiscalapi/models/invoicing/Invoice.java`:
- Line 8: In class Invoice, the setters setSubtotal, setDiscount, setTotal, and
setExchangeRate should guard against null or blank string input before calling
new BigDecimal(String); update each setter to check if the input string is null
or blank (e.g., null or trim().isEmpty()) and if so set the corresponding
BigDecimal field to null (or leave it unset), otherwise construct new BigDecimal
from the trimmed string and assign it—apply this exact pattern to setSubtotal,
setDiscount, setTotal, and setExchangeRate to avoid
NullPointerException/NumberFormatException during deserialization.
- Around line 216-226: Remove the Jackson setter conflict by annotating the
plain JavaBean setter setDate(String) with `@JsonIgnore` so only
setSatDate(String) (annotated with `@JsonProperty`("date")) is used for JSON
deserialization; additionally, inside setDate(String) and setSatDate(String) add
null/empty guards before calling
com.fiscalapi.OptUtil.formatInputDateToSATFormat(...) and only assign to
this.date when the input is non-null/non-empty (to avoid
formatInputDateToSATFormat returning LocalDateTime.MIN or otherwise unintended
values).
In `@src/main/java/com/fiscalapi/models/invoicing/InvoiceItem.java`:
- Line 3: In InvoiceItem, the String-based setters setQuantity, setDiscount, and
setUnitPrice must guard against null/blank strings before calling new
BigDecimal(...) to avoid NPE/NumberFormatException during deserialization;
update each setter to check if the input is null or input.trim().isEmpty() and
if so assign the corresponding BigDecimal field to null (or a safe default),
otherwise call new BigDecimal(input.trim()) (optionally wrap the construction in
a try/catch to rethrow a clear IllegalArgumentException on invalid number
formats).
In `@src/main/java/com/fiscalapi/models/invoicing/localTaxes/LocalTax.java`:
- Around line 23-25: The setters in LocalTax (setTaxRate and setTaxAmount) take
String and directly new BigDecimal(...) which throws NPE/NumberFormatException;
update these methods in class LocalTax to either accept BigDecimal parameters
(preferred for consistency with PayrollSeverance) or add defensive handling:
check for null/blank, validate/parse safely (e.g., catch NumberFormatException)
and set fields taxRate/taxAmount to null or a default BigDecimal.ZERO as
appropriate, and document behavior; ensure the methods preserve the BigDecimal
field types and mirror the null-safe pattern used in
PayrollDeduction/PayrollSeverance so callers and serialization remain
consistent.
In
`@src/main/java/com/fiscalapi/models/invoicing/paymentComplement/InvoicePayment.java`:
- Around line 69-79: The InvoicePayment class has two setters that map to the
same JSON property—setSatDate(String) is annotated with
`@JsonProperty`("paymentDate") while setPaymentDate(String) is also picked up by
Jackson via naming convention—causing a conflicting-setter error; fix this by
annotating the plain String overload setPaymentDate(String) with `@JsonIgnore` so
Jackson only uses setSatDate(String) for deserialization, leaving the formatting
call to com.fiscalapi.OptUtil.formatInputDateToSATFormat intact.
In
`@src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoice.java`:
- Around line 1-18: The BigDecimal setters in PaidInvoice (setPaymentAmount,
setSubTotal, setPreviousBalance, setRemainingBalance, setEquivalence) call new
BigDecimal(...) directly on String inputs and throw NumberFormatException for
null/blank values; update each setter to first check if the incoming String is
null or blank (use StringUtils.isBlank or trim().isEmpty()) and if so set the
corresponding field to null (or leave it unchanged), otherwise construct new
BigDecimal from the trimmed string—ensuring you reference the paymentAmount,
subTotal, previousBalance, remainingBalance, and equivalence fields and their
respective setter methods when making the change.
In
`@src/main/java/com/fiscalapi/models/invoicing/paymentComplement/PaidInvoiceTax.java`:
- Around line 16-18: The setBase(String base) setter in PaidInvoiceTax currently
calls new BigDecimal(base) which can throw NumberFormatException; either change
the setter signature to setBase(BigDecimal base) (updating all callers) to match
other models like ProductTax/Product, or if you must keep a String overload, add
null and format validation inside setBase(String) (check for null/empty, catch
NumberFormatException) and convert safely to this.base (or delegate to
setBase(BigDecimal)) while preserving existing behavior and consistency with the
class field named base.
In `@src/main/java/com/fiscalapi/models/invoicing/payroll/EmployeeData.java`:
- Around line 84-95: The setter for laborRelationStartDate should avoid storing
the sentinel LocalDateTime.MIN when input is null/blank: in
setLaborRelationStartDate(String) (EmployeeData) check for null or blank input
and set this.laborRelationStartDate = null; otherwise call
OptUtil.formatInputDateToSATFormat and assign; also harden
getLaborRelationStartDate() to treat LocalDateTime.MIN as missing by returning
null if laborRelationStartDate == null ||
laborRelationStartDate.equals(LocalDateTime.MIN) before formatting with
SAT_DATE_FORMAT_OUT.
- Around line 261-270: The setters setBaseSalaryForContributions and
setIntegratedDailySalary should guard against null or blank input before calling
new BigDecimal(...); update each setter to check if the incoming String is null
or blank (e.g., trim and isEmpty/isBlank) and assign the corresponding
BigDecimal field to null in that case, otherwise construct new BigDecimal from
the validated string so Jackson deserialization won't throw
NullPointerException/NumberFormatException.
In `@src/main/java/com/fiscalapi/models/invoicing/payroll/Payroll.java`:
- Around line 38-71: The setters setPaymentDate, setInitialPaymentDate, and
setFinalPaymentDate are assigning LocalDateTime.MIN when
OptUtil.formatInputDateToSATFormat receives null/blank, causing invalid
serialization; change each setter to first check if the input string is null or
blank and set the corresponding field to null in that case, otherwise call
com.fiscalapi.OptUtil.formatInputDateToSATFormat(paymentDate) (or the matching
param) and assign the result; optionally note that a better centralized fix is
to have OptUtil.formatInputDateToSATFormat return null instead of
LocalDateTime.MIN so callers like these setters don't need repeated guards.
In `@src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDeduction.java`:
- Around line 39-41: The setter setAmount in PayrollDeduction currently accepts
a String and constructs new BigDecimal(amount), which breaks JavaBean symmetry
with the BigDecimal-typed field and may throw NPE/NumberFormatException; change
setAmount to accept a BigDecimal parameter (matching PayrollSeverance) or, if
String input is required, add null-check and validation/parsing with clear error
handling (e.g., null => set to null or throw IllegalArgumentException; catch
NumberFormatException and rethrow with contextual message) so that
PayrollDeduction.setAmount and its BigDecimal field/ getter are consistent and
safe.
In `@src/main/java/com/fiscalapi/models/StampTransaction.java`:
- Around line 75-77: The setter method in StampTransaction is misspelled as
seTransactionStatus; rename this method to setTransactionStatus to follow
JavaBean conventions so frameworks can map the transactionStatus field
correctly, update any references/callers/tests to the new method name, and
ensure the method still assigns this.transactionStatus = transactionStatus in
the StampTransaction class.
In `@src/main/java/com/fiscalapi/models/StampTransactionParams.java`:
- Line 8: The field "comments" in class StampTransactionParams is declared
public, breaking encapsulation and differing from other private fields; change
the declaration of the comments field to private and add standard accessor
methods getComments() and setComments(String comments) in StampTransactionParams
so it matches the existing getter/setter pattern used by other fields and
preserves API/serialization expectations.
In `@src/main/java/com/fiscalapi/OptUtil.java`:
- Around line 10-14: In OptUtil.formatInputDateToSATFormat change the sentinel
return value: instead of initializing and returning LocalDateTime.MIN when
stringDate is null or empty, return null so callers can detect missing dates;
update the method's local variable (e.g., parsedDate) to allow null and ensure
callers like Xml.getSatDate() handle a null return appropriately.
In `@src/main/java/com/fiscalapi/services/EmployeeService.java`:
- Around line 26-36: Add null-checks for requestModel and for
requestModel.getEmployeePersonId() at the start of both Create and Update
methods and throw an IllegalArgumentException with a clear message if either is
null; specifically validate inside Create(...) and Update(...) before calling
buildEndpoint(requestModel.getEmployeePersonId()), e.g., ensure requestModel !=
null and requestModel.getEmployeePersonId() != null and throw
IllegalArgumentException("...") when validation fails so buildEndpoint and
downstream httpClient calls never receive "null" as the id.
In `@src/main/java/com/fiscalapi/services/InvoiceService.java`:
- Around line 60-63: The create method in InvoiceService currently always posts
to the base invoices endpoint; restore type-based routing by inspecting
model.getTypeCode() inside create and map "I" -> "income", "E" -> "credit-note",
"P" -> "payment", then call buildEndpoint(mappedPath, null) and pass that
endpoint to httpClient.post(model, Invoice.class); for any other type throw an
IllegalArgumentException("Invalid invoice type") so credit notes and payments
route to their correct endpoints.
In `@src/main/java/com/fiscalapi/services/StampService.java`:
- Around line 21-29: TransferStamps and WithdrawStamps both post to
buildEndpoint("", null) with identical StampTransactionParams so the backend
cannot distinguish actions; update these methods to call distinct endpoints
(e.g., use buildEndpoint("transfer", null) in TransferStamps and
buildEndpoint("withdraw", null) in WithdrawStamps) or alternately add a
transactionType/action field to StampTransactionParams and set it appropriately
before posting, ensuring the chosen approach is implemented consistently in the
TransferStamps and WithdrawStamps methods and any consumers of
StampTransactionParams.
🧹 Nitpick comments (20)
src/main/java/com/fiscalapi/models/Person.java (1)
26-27: Consider importingBigDecimalfor consistency.The file uses import statements for other types (e.g.,
LocalDateTime), butBigDecimalis referenced with its fully qualified name throughout. Adding an import would improve readability and align with the existing code style.✨ Proposed refactor to use import
Add the import at the top of the file:
import java.math.BigDecimal;Then update the field declarations and methods:
- private java.math.BigDecimal availableBalance; - private java.math.BigDecimal committedBalance; + private BigDecimal availableBalance; + private BigDecimal committedBalance;- public java.math.BigDecimal getAvailableBalance() { + public BigDecimal getAvailableBalance() { return availableBalance; } - public void setAvailableBalance(java.math.BigDecimal availableBalance) { + public void setAvailableBalance(BigDecimal availableBalance) { this.availableBalance = availableBalance; } - public java.math.BigDecimal getCommittedBalance() { + public BigDecimal getCommittedBalance() { return committedBalance; } - public void setCommittedBalance(java.math.BigDecimal committedBalance) { + public void setCommittedBalance(BigDecimal committedBalance) { this.committedBalance = committedBalance; }Also applies to: 149-149, 153-153, 157-157, 161-161
src/main/java/com/fiscalapi/models/UserLookupDto.java (1)
5-25: Field declaration should be grouped at the top.The
legalNamefield is declared at line 24, after its getter/setter methods. For consistency and readability, declare all fields together at the top of the class.♻️ Suggested reordering
public class UserLookupDto extends BaseDto { private String tin; + private String legalName; public String getLegalName() { return legalName; } public void setLegalName(String legalName) { this.legalName = legalName; } public String getTin() { return tin; } public void setTin(String tin) { this.tin = tin; } - - private String legalName; }src/main/java/com/fiscalapi/abstractions/IStampService.java (1)
7-10: Method names should use camelCase instead of PascalCase.
TransferStampsandWithdrawStampsshould betransferStampsandwithdrawStampsto follow Java naming conventions and maintain consistency with all other service interfaces in the codebase.♻️ Proposed fix
public interface IStampService extends IFiscalApiService<StampTransaction>{ - ApiResponse<Boolean> TransferStamps(StampTransactionParams requestModel); - ApiResponse<Boolean> WithdrawStamps(StampTransactionParams requestModel); + ApiResponse<Boolean> transferStamps(StampTransactionParams requestModel); + ApiResponse<Boolean> withdrawStamps(StampTransactionParams requestModel); }src/main/java/com/fiscalapi/models/ProductTax.java (1)
34-34: Use the importedBigDecimalclass name for consistency.The fully qualified
java.math.BigDecimalis used here and in the getter/setter (lines 96, 105), but the import already exists at line 5. Consider using the short formBigDecimalfor cleaner code.♻️ Proposed fix
- private java.math.BigDecimal rate; + private BigDecimal rate;And similarly for the getter/setter signatures.
CLAUDE.md (2)
24-32: Add language specifier to fenced code block.Static analysis flagged this code block as missing a language specifier. Since this is a text-based diagram, use
textorplaintext.📝 Proposed fix
-``` +```text IFiscalApiClient (facade) ├── getInvoiceService() → IInvoiceService extends IFiscalApiService<Invoice>
45-47: Add language specifier to fenced code block.Same issue as above - add
textlanguage specifier for the DTO hierarchy diagram.📝 Proposed fix
-``` +```text SerializableDto → AuditableDto (createdAt, updatedAt) → BaseDto (id)src/main/java/com/fiscalapi/models/invoicing/payroll/BalanceCompensation.java (1)
6-9: Consider usingIntegerfor nullable year representation.Using primitive
intforyearmeans it defaults to 0 when not set. In a payroll context, distinguishing between "year not provided" and "year 0" (or any valid year) may be important for validation and serialization.♻️ Proposed change
public class BalanceCompensation extends CatalogDto { private BigDecimal favorableBalance; - private int year; + private Integer year; private BigDecimal remainingFavorableBalance;Update getter/setter accordingly:
- public int getYear() { + public Integer getYear() { return year; } - public void setYear(int year) { + public void setYear(Integer year) { this.year = year; }src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollDisability.java (1)
6-31: Use lowerCamelCase for private fields.
Capitalized field names are non-idiomatic in Java and make the DTO harder to read. Rename the backing fields and update assignments to match.♻️ Proposed refactor
- private int DisabilityDays; - private String DisabilityTypeCode; - private BigDecimal MonetaryAmount; + private int disabilityDays; + private String disabilityTypeCode; + private BigDecimal monetaryAmount; public int getDisabilityDays() { - return DisabilityDays; + return disabilityDays; } public void setDisabilityDays(int disabilityDays) { - DisabilityDays = disabilityDays; + this.disabilityDays = disabilityDays; } public String getDisabilityTypeCode() { - return DisabilityTypeCode; + return disabilityTypeCode; } public void setDisabilityTypeCode(String disabilityTypeCode) { - DisabilityTypeCode = disabilityTypeCode; + this.disabilityTypeCode = disabilityTypeCode; } public BigDecimal getMonetaryAmount() { - return MonetaryAmount; + return monetaryAmount; } public void setMonetaryAmount(BigDecimal monetaryAmount) { - MonetaryAmount = monetaryAmount; + this.monetaryAmount = monetaryAmount; }src/main/java/com/fiscalapi/services/EmployerService.java (1)
20-41: Deduplicate endpoint construction.
This mirrors EmployeeService and avoids repeated String.format blocks across methods.♻️ Proposed refactor
public EmployerService(IFiscalApiHttpClient httpClient) { this.httpClient = httpClient; } + private String buildEndpoint(String personId) { + return String.format("%s/%s/employer", baseEndpoint, personId); + } + public ApiResponse<EmployerData> GetById(String id) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, id); - - return httpClient.get(endpoint, EmployerData.class); + return httpClient.get(buildEndpoint(id), EmployerData.class); } public ApiResponse<EmployerData>Create(EmployerData requestModel) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, requestModel.getPersonId()); - - return httpClient.post(endpoint, requestModel, EmployerData.class); + return httpClient.post(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); } public ApiResponse<EmployerData> Update(EmployerData requestModel) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, requestModel.getPersonId()); - - return httpClient.put(endpoint, requestModel, EmployerData.class); + return httpClient.put(buildEndpoint(requestModel.getPersonId()), requestModel, EmployerData.class); } public ApiResponse<Boolean> Delete(String id) { - String endpoint = String.format("%s/%s/employer", baseEndpoint, id); - - return httpClient.delete(endpoint); + return httpClient.delete(buildEndpoint(id)); }src/main/java/com/fiscalapi/abstractions/IPersonService.java (1)
12-12: Remove the unused interface constant.
Interface fields are public static final; keeping a null constant here is misleading.♻️ Proposed refactor
- EmployerService employer = null;src/main/java/com/fiscalapi/models/downloading/Xml.java (2)
9-11: Unused imports after refactoring.
ZonedDateTimeandDateTimeParseExceptionare no longer used in this file since date parsing logic has been moved toOptUtil. Consider removing these unused imports.♻️ Proposed fix
import java.math.BigDecimal; import java.time.LocalDateTime; -import java.time.ZonedDateTime; -import java.time.format.DateTimeParseException; import java.util.ArrayList;
178-187:setDate(String)duplicatessetSatDate(String)functionality.Both methods do exactly the same thing—parse a string date using
OptUtil.formatInputDateToSATFormatand assign tothis.date. The only difference issetSatDateis annotated with@JsonProperty("date")for JSON deserialization.If
setDate(String)is intended as a programmatic convenience setter, this is acceptable but consider documenting the distinction. Otherwise, this introduces API surface redundancy.src/main/java/com/fiscalapi/models/invoicing/InvoiceRecipient.java (1)
12-22: Field declaration should precede its accessors.The
employeeDatafield is declared at line 21, after its getter and setter (lines 13-19). This breaks Java convention where fields are typically declared at the top of the class before methods. The same pattern is followed elsewhere in this file (e.g.,id,tin, etc. are declared before their accessors).♻️ Proposed fix: move field before accessors
private String cfdiUseCode; + private EmployeeData employeeData; public EmployeeData getEmployeeData() { return employeeData; } public void setEmployeeData(EmployeeData employeeData) { this.employeeData = employeeData; } - private EmployeeData employeeData; private String zipCode;src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarningOvertime.java (1)
32-41: Field placement and import style inconsistencies.Two issues:
- The
amountPaidfield (line 40) is declared after its getter/setter, breaking Java convention.java.math.BigDecimalis used as a fully qualified name instead of an import statement. Other payroll models use an import at the top.♻️ Proposed fix
package com.fiscalapi.models.invoicing.payroll; +import java.math.BigDecimal; + public class PayrollEarningOvertime { private int days; private String hoursTypeCode; private int extraHours; + private BigDecimal amountPaid; public int getDays() { return days; } @@ -32,11 +34,9 @@ this.extraHours = extraHours; } - public java.math.BigDecimal getAmountPaid() { + public BigDecimal getAmountPaid() { return amountPaid; } - public void setAmountPaid(java.math.BigDecimal amountPaid) { + public void setAmountPaid(BigDecimal amountPaid) { this.amountPaid = amountPaid; } - - private java.math.BigDecimal amountPaid; }src/main/java/com/fiscalapi/OptUtil.java (1)
10-10: Method nameformatInputDateToSATFormatis misleading.The method parses a date string into a
LocalDateTimeobject—it doesn't format anything. A name likeparseDateFromSATFormatorparseToLocalDateTimewould better describe the behavior.src/main/java/com/fiscalapi/services/PersonService.java (1)
14-23: Field declarations should precede their accessor methods.Fields
employerServiceandemployeeService(lines 22-23) are declared after their getters (lines 14-20). Move field declarations to precede the methods for consistency with Java conventions.♻️ Proposed fix
public class PersonService extends BaseFiscalApiService<Person> implements IPersonService { + private final EmployerService employerService; + private final EmployeeService employeeService; + public EmployerService getEmployerService() { return employerService; } public EmployeeService getEmployeeService() { return employeeService; } - private final EmployerService employerService; - private final EmployeeService employeeService;src/main/java/com/fiscalapi/services/EmployeeService.java (2)
7-9:httpClientshould beprivate final.The field is injected via constructor and never reassigned, so it should be declared
private finalfor immutability and encapsulation.Suggested fix
- IFiscalApiHttpClient httpClient; + private final IFiscalApiHttpClient httpClient;
20-40: Method names should use camelCase per Java conventions.
GetById,Create,Update, andDeleteuse PascalCase which is unconventional for Java methods. Standard practice is camelCase:getById,create,update,delete.Suggested fix
- public ApiResponse<EmployeeData> GetById(String id) { + public ApiResponse<EmployeeData> getById(String id) { String endpoint = buildEndpoint(id); return httpClient.get(endpoint, EmployeeData.class); } - public ApiResponse<EmployeeData>Create(EmployeeData requestModel) { + public ApiResponse<EmployeeData> create(EmployeeData requestModel) { String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); return httpClient.post(endpoint, requestModel, EmployeeData.class); } - public ApiResponse<EmployeeData> Update(EmployeeData requestModel) { + public ApiResponse<EmployeeData> update(EmployeeData requestModel) { String endpoint = buildEndpoint(requestModel.getEmployeePersonId()); return httpClient.put(endpoint, requestModel, EmployeeData.class); } - public ApiResponse<Boolean> Delete(String id) { + public ApiResponse<Boolean> delete(String id) { return httpClient.delete(buildEndpoint(id)); }src/main/java/com/fiscalapi/models/invoicing/payroll/PayrollEarning.java (1)
6-62: Field declarations are split between top and bottom of the class.Fields
concept,taxedAmount,exemptAmount, andovertimeare declared at lines 58-61, after their accessor methods. Java convention is to declare all instance fields together at the top of the class for readability.Suggested reorganization
public class PayrollEarning { private String earningTypeCode; private String code; + private String concept; + private BigDecimal taxedAmount; + private BigDecimal exemptAmount; + private List<PayrollEarningOvertime> overtime; public String getEarningTypeCode() { return earningTypeCode; @@ -54,9 +58,4 @@ public void setOvertime(List<PayrollEarningOvertime> overtime) { this.overtime = overtime; } - - private String concept; - private BigDecimal taxedAmount; - private BigDecimal exemptAmount; - private List<PayrollEarningOvertime> overtime; }src/main/java/com/fiscalapi/models/downloading/DownloadRequest.java (1)
235-236: Adopt standard JavaBeans setter method names via overloading.
setXFromLocalDateTimeis non-standard. JavaBeans tooling (Spring, BeanUtils, IDEs) recognizes only the patternpublic void setX(T)for propertyXof typeT. Overload existingsetX(String)methods withsetX(LocalDateTime)variants instead, which allows both signatures while maintaining bean introspection compatibility.♻️ Overload example (apply to all four methods)
-public void setStartDateFromLocalDateTime(LocalDateTime startDate) { +public void setStartDate(LocalDateTime startDate) { this.startDate = startDate; }Also applies to: 256–257 (
setEndDateFromLocalDateTime), 442–443 (setLastAttemptDateFromLocalDateTime), 460–462 (setNextAttemptDateFromLocalDateTime)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/fiscalapi/OptUtil.java`:
- Around line 20-26: The catch block that parses into ZonedDateTime currently
throws a new IllegalArgumentException using the wrong cause variable; replace
the cause from 'e' to the actual ZonedDateTime parse exception 'e2' so the
thrown IllegalArgumentException includes the correct underlying
DateTimeParseException (update the throw in the catch for DateTimeParseException
to use e2), keeping the existing message that references stringDate and the
expected format and leaving parsedDate/ZonedDateTime logic unchanged.
🧹 Nitpick comments (1)
src/main/java/com/fiscalapi/OptUtil.java (1)
9-9: Make the utility class non‑instantiable.
This class is a pure static utility; add a private constructor and mark it final to prevent accidental instantiation.♻️ Proposed refactor
-public class OptUtil { +public final class OptUtil { + private OptUtil() { + // Utility class + } public static LocalDateTime formatInputDateToSATFormat(String stringDate) {
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/com/fiscalapi/services/EmployerService.java`:
- Around line 24-26: Validate the id in EmployerService.GetById before calling
httpClient: check for null or blank (trimmed) and fail fast (e.g., throw
IllegalArgumentException or return an ApiResponse representing a bad request)
instead of building the endpoint; apply the same null/blank validation to the
other similar endpoint methods in this class (the method around lines 36-37)
that call buildEndpoint and httpClient.get to avoid constructing invalid URLs
and to surface caller errors immediately.
- Around line 28-34: Guard Create and Update against null requestModel and
missing personId: in EmployerService methods Create(EmployerData) and
Update(EmployerData) check that requestModel is not null and that
requestModel.getPersonId() is non-null/ non-empty before calling buildEndpoint
or httpClient.post/put; if validation fails, return a failed ApiResponse (or
throw a documented IllegalArgumentException) with a clear error message rather
than proceeding to buildEndpoint/getPersonId to avoid NPEs and invalid endpoint
calls.
- Around line 20-22: The buildEndpoint method in EmployerService is using the
wrong path; update EmployerService.buildEndpoint(String personId) to construct
the URL with "/employer" instead of "/employee" (i.e., use baseEndpoint and
personId to format "%s/%s/employer") so the service targets the correct Employer
resource; ensure any tests or usages of EmployerService that rely on
buildEndpoint still work with the corrected path.
🧹 Nitpick comments (1)
src/main/java/com/fiscalapi/services/EmployerService.java (1)
12-18: MakehttpClientimmutable.
It’s only set in the constructor; marking itfinalimproves safety and clarity.♻️ Proposed change
- private IFiscalApiHttpClient httpClient; + private final IFiscalApiHttpClient httpClient;
mendozagit
requested changes
Feb 9, 2026
Contributor
mendozagit
left a comment
mendozagit
left a comment
There was a problem hiding this comment.
Mandé solo 5 comentarios, porque no has resuelto todo las discusiones de code rabit, cuando termines con el me avisas, solo después de que resuelvas esas discusiones avísame hacer un según review, por fa.
Cuida el API publica, hay gente que ya está usando esto y no podemos cambiarla dramáticamente porque no vamos a ganar muchos amigos y nos van a bombardear de soporte. Si cambias el api publica tenemos que deprecar los métodos que quedan en desuso, no eliminarlos y eso implica mantener dos Apis públicas (interfaces/implementaciones) del SDK.
|
|
||
| /** | ||
| * @param startDate Fecha inicial - Fecha de inicio para la solicitud asociada como LocalDateTime | ||
| */ |
Contributor
There was a problem hiding this comment.
No dupliques la api publica del SDK, debe exitir un solo metodo setStartDate / getStartDate
| * @param endDate Fecha final - Fecha de fin para la solicitud asociada como LocalDateTime | ||
| */ | ||
| public void setEndDate(LocalDateTime endDate) { | ||
| public void setEndDateFromLocalDateTime(LocalDateTime endDate) { |
Contributor
There was a problem hiding this comment.
Mismo caso, Tambien considera que modificar el api publica puede romper aplicaciones que ya fueron integradas, si necesitas hacer mejoras asegúrate de evitar los break changes tanto como sea posible.
| * @param lastAttemptDate Fecha del último intento para la solicitud asociada | ||
| */ | ||
| public void setLastAttemptDate(LocalDateTime lastAttemptDate) { | ||
| public void setLastAttemptDate(String lastAttemptDate) { |
| * @param nextAttemptDate Fecha del siguiente intento para la solicitud asociada | ||
| */ | ||
| public void setNextAttemptDate(LocalDateTime nextAttemptDate) { | ||
| public void setNextAttemptDate(String nextAttemptDate) { |
src/main/java/com/fiscalapi/models/invoicing/paymentComplement/InvoicePayment.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 by CodeRabbit
New Features
Refactor
Documentation
Chores