Fix/fix technical debt and add missing payroll classes#2
Open
Anidem1995 wants to merge 11 commits intomasterfrom
Open
Fix/fix technical debt and add missing payroll classes#2Anidem1995 wants to merge 11 commits intomasterfrom
Anidem1995 wants to merge 11 commits intomasterfrom
Conversation
…dated Person Service references for employee and employer service
📝 WalkthroughWalkthroughAdds IEmployeeService and IEmployerService interfaces, updates services and PersonService to use interfaces and dynamic apiVersion endpoints with input validation, extends Payroll model with PayrollStockOptions, updates README, and bumps package version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (6)
Abstractions/IEmployeeService.cs (1)
13-16: Same ambiguousidparameter asIEmployerService— preferpersonId.The endpoint pattern (
api/{version}/people/{personId}/employee) confirmsidrepresents a person's ID, not the employee record's own key. Renaming topersonIdmakes the contract self-documenting and consistent withIEmployerServiceonce that rename is applied.♻️ Proposed rename
-Task<ApiResponse<EmployeeData>> GetByIdAsync(string id); +Task<ApiResponse<EmployeeData>> GetByIdAsync(string personId); Task<ApiResponse<EmployeeData>> CreateAsync(EmployeeData requestModel); Task<ApiResponse<EmployeeData>> UpdateAsync(EmployeeData requestModel); -Task<ApiResponse<bool>> DeleteAsync(string id); +Task<ApiResponse<bool>> DeleteAsync(string personId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Abstractions/IEmployeeService.cs` around lines 13 - 16, The interface IEmployeeService currently uses an ambiguous parameter name "id" in GetByIdAsync(string id) and DeleteAsync(string id) (and any CreateAsync/UpdateAsync signatures if they imply an id); rename those parameters to "personId" to reflect the API route semantics and match IEmployerService (e.g., change GetByIdAsync(string id) -> GetByIdAsync(string personId) and DeleteAsync(string id) -> DeleteAsync(string personId)), update XML docs and any callers to use the new parameter name, and ensure method signatures in implementations of IEmployeeService are updated accordingly.Services/PersonService.cs (1)
12-17: Sub-services are hard-wired in the constructor — consider injection for testability.
EmployerServiceandEmployeeServiceare instantiated directly, so they cannot be independently replaced or mocked when testingPersonService. Accepting the interfaces via the constructor would make the class fully testable and consistent with the interface-based surface already exposed.♻️ Optional refactor — constructor injection
-public PersonService(IFiscalApiHttpClient httpClient, string apiVersion) - : base(httpClient, "people", apiVersion) -{ - Employer = new EmployerService(httpClient, apiVersion); - Employee = new EmployeeService(httpClient, apiVersion); -} +public PersonService( + IFiscalApiHttpClient httpClient, + string apiVersion, + IEmployerService employerService = null, + IEmployeeService employeeService = null) + : base(httpClient, "people", apiVersion) +{ + Employer = employerService ?? new EmployerService(httpClient, apiVersion); + Employee = employeeService ?? new EmployeeService(httpClient, apiVersion); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Services/PersonService.cs` around lines 12 - 17, PersonService currently instantiates EmployerService and EmployeeService directly in its constructor, making Employer and Employee unmockable; change the constructor signature of PersonService to accept interfaces (e.g., IEmployerService employer, IEmployeeService employee) or factories and assign them to the Employer and Employee properties instead of new-ing concrete classes so tests can inject mocks; update any call sites that construct PersonService to pass the concrete implementations (new EmployerService(...), new EmployeeService(...)) or DI container bindings.Abstractions/IEmployerService.cs (1)
13-16: Ambiguousidparameter — considerpersonIdfor clarity.The XML doc comment documents the endpoint as
api/{version}/people/{personId}/employer, making theidparameter inGetByIdAsyncandDeleteAsynca person ID rather than the employer record's own key. The current nameidis ambiguous for callers.♻️ Proposed rename
-Task<ApiResponse<EmployerData>> GetByIdAsync(string id); +Task<ApiResponse<EmployerData>> GetByIdAsync(string personId); Task<ApiResponse<EmployerData>> CreateAsync(EmployerData requestModel); Task<ApiResponse<EmployerData>> UpdateAsync(EmployerData requestModel); -Task<ApiResponse<bool>> DeleteAsync(string id); +Task<ApiResponse<bool>> DeleteAsync(string personId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Abstractions/IEmployerService.cs` around lines 13 - 16, Rename the ambiguous parameter name `id` to `personId` in the IEmployerService interface for the methods GetByIdAsync and DeleteAsync (update their signatures: GetByIdAsync(string personId) and DeleteAsync(string personId)), and update their XML doc comments to match the documented route `api/{version}/people/{personId}/employer`; then propagate the renamed parameter to any implementing classes/methods (implementations of IEmployerService) and callers to keep signatures and docs consistent.Services/EmployerService.cs (1)
15-19: Same missing constructor parameter validation asEmployeeService.Apply the same guard clauses for
fiscalApiHttpClientandapiVersionhere.Proposed fix
public EmployerService(IFiscalApiHttpClient fiscalApiHttpClient, string apiVersion) { - HttpClient = fiscalApiHttpClient; + HttpClient = fiscalApiHttpClient ?? throw new ArgumentNullException(nameof(fiscalApiHttpClient)); + if (string.IsNullOrWhiteSpace(apiVersion)) + throw new ArgumentException("API version is required", nameof(apiVersion)); _baseEndpoint = $"api/{apiVersion}/people"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Services/EmployerService.cs` around lines 15 - 19, The EmployerService constructor currently assigns HttpClient and _baseEndpoint without validating inputs; add the same guard clauses used in EmployeeService to validate parameters by throwing ArgumentNullException for a null IFiscalApiHttpClient (fiscalApiHttpClient) and for a null or empty apiVersion string, before assigning HttpClient and computing _baseEndpoint in the EmployerService(string apiVersion) constructor to ensure inputs are validated early.Services/EmployeeService.cs (2)
27-28: Inconsistent error message language (English vs. Spanish).
GetByIdAsync(Line 28) uses English ("Employee person ID is required to build endpoint"), whileDeleteAsync(Line 50) andValidateRequestModel(Lines 57, 60) use Spanish. The same pattern appears inEmployerService.cs. Pick one language consistently — ideally matching whatever convention the rest of the SDK follows.Also applies to: 49-50, 54-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Services/EmployeeService.cs` around lines 27 - 28, The error messages across methods are inconsistent (mix of English and Spanish); update the message in GetByIdAsync and any mismatched messages in DeleteAsync and ValidateRequestModel to match the SDK's chosen language consistently (use the language already used elsewhere in the SDK), e.g., change the string in the ArgumentException thrown in GetByIdAsync (and analogous messages in DeleteAsync and ValidateRequestModel) so all use the same wording and language; ensure you update EmployerService's equivalent checks as well (match the exact phrasing/style used elsewhere).
15-19: Missing constructor parameter validation.Neither
fiscalApiHttpClientnorapiVersionis validated. Anullor emptyapiVersionsilently produces a malformed endpoint (e.g.,api//people), and anullfiscalApiHttpClientwill cause aNullReferenceExceptionlater at call time instead of at construction time. The same issue exists inEmployerService.cs(Line 15).Proposed fix: validate constructor parameters
public EmployeeService(IFiscalApiHttpClient fiscalApiHttpClient, string apiVersion) { - HttpClient = fiscalApiHttpClient; + HttpClient = fiscalApiHttpClient ?? throw new ArgumentNullException(nameof(fiscalApiHttpClient)); + if (string.IsNullOrWhiteSpace(apiVersion)) + throw new ArgumentException("API version is required", nameof(apiVersion)); _baseEndpoint = $"api/{apiVersion}/people"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Services/EmployeeService.cs` around lines 15 - 19, Validate constructor parameters in EmployeeService by throwing ArgumentNullException for a null fiscalApiHttpClient and ArgumentException (or ArgumentNullException) for a null or empty apiVersion before assigning fields; ensure the constructor for EmployeeService(IFiscalApiHttpClient fiscalApiHttpClient, string apiVersion) checks fiscalApiHttpClient and apiVersion and only then sets HttpClient and _baseEndpoint, and apply the same validation pattern to the EmployerService constructor to prevent malformed _baseEndpoint and delayed NullReferenceExceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 42-43: The README has duplicate top-level headings "## 🎖️ Gestión
de Timbres" and another "## Gestión de Timbres" (lines around the two sections);
merge the content of the second block into the first or rename the second to a
more specific title such as "## 🎫 Operaciones de Timbres" and adjust its body
accordingly, then update the table of contents to reflect the single/renamed
heading so entries are no longer duplicated; ensure any internal links or
references to "Gestión de Timbres" are updated to the chosen consolidated
heading.
- Around line 48-49: Remove the misplaced "Timbres" bullet from the "Gestión de
Productos/Servicios" subsection (the line "- **Timbres** Listar transacciones,
transferir y retirar timbres entre personas.") and move or consolidate that
content into the dedicated "## Gestión de Timbres" section; update the "##
Gestión de Timbres" section to include the bullet text (or a revised entry) and
ensure there are no duplicate references to "Timbres" elsewhere in the README.
In `@Services/StampService.cs`:
- Around line 23-27: WithdrawStamps is incorrectly using ValidateRequest (which
enforces ToPersonId and transfer-specific messages) and the same BuildEndpoint
as TransferStamps; create a withdrawal-specific validation (e.g.,
ValidateWithdrawalRequest or add an operation flag to ValidateRequest) that does
not require ToPersonId and uses withdrawal-appropriate error text, update
WithdrawStamps to call that new validator (instead of ValidateRequest), and call
a distinct endpoint for withdrawals (e.g., BuildEndpoint("withdraw") or a new
BuildWithdrawEndpoint) while leaving TransferStamps to use the transfer
validator/endpoint (TransferStamps, ValidateRequest, BuildEndpoint).
---
Nitpick comments:
In `@Abstractions/IEmployeeService.cs`:
- Around line 13-16: The interface IEmployeeService currently uses an ambiguous
parameter name "id" in GetByIdAsync(string id) and DeleteAsync(string id) (and
any CreateAsync/UpdateAsync signatures if they imply an id); rename those
parameters to "personId" to reflect the API route semantics and match
IEmployerService (e.g., change GetByIdAsync(string id) -> GetByIdAsync(string
personId) and DeleteAsync(string id) -> DeleteAsync(string personId)), update
XML docs and any callers to use the new parameter name, and ensure method
signatures in implementations of IEmployeeService are updated accordingly.
In `@Abstractions/IEmployerService.cs`:
- Around line 13-16: Rename the ambiguous parameter name `id` to `personId` in
the IEmployerService interface for the methods GetByIdAsync and DeleteAsync
(update their signatures: GetByIdAsync(string personId) and DeleteAsync(string
personId)), and update their XML doc comments to match the documented route
`api/{version}/people/{personId}/employer`; then propagate the renamed parameter
to any implementing classes/methods (implementations of IEmployerService) and
callers to keep signatures and docs consistent.
In `@Services/EmployeeService.cs`:
- Around line 27-28: The error messages across methods are inconsistent (mix of
English and Spanish); update the message in GetByIdAsync and any mismatched
messages in DeleteAsync and ValidateRequestModel to match the SDK's chosen
language consistently (use the language already used elsewhere in the SDK),
e.g., change the string in the ArgumentException thrown in GetByIdAsync (and
analogous messages in DeleteAsync and ValidateRequestModel) so all use the same
wording and language; ensure you update EmployerService's equivalent checks as
well (match the exact phrasing/style used elsewhere).
- Around line 15-19: Validate constructor parameters in EmployeeService by
throwing ArgumentNullException for a null fiscalApiHttpClient and
ArgumentException (or ArgumentNullException) for a null or empty apiVersion
before assigning fields; ensure the constructor for
EmployeeService(IFiscalApiHttpClient fiscalApiHttpClient, string apiVersion)
checks fiscalApiHttpClient and apiVersion and only then sets HttpClient and
_baseEndpoint, and apply the same validation pattern to the EmployerService
constructor to prevent malformed _baseEndpoint and delayed
NullReferenceExceptions.
In `@Services/EmployerService.cs`:
- Around line 15-19: The EmployerService constructor currently assigns
HttpClient and _baseEndpoint without validating inputs; add the same guard
clauses used in EmployeeService to validate parameters by throwing
ArgumentNullException for a null IFiscalApiHttpClient (fiscalApiHttpClient) and
for a null or empty apiVersion string, before assigning HttpClient and computing
_baseEndpoint in the EmployerService(string apiVersion) constructor to ensure
inputs are validated early.
In `@Services/PersonService.cs`:
- Around line 12-17: PersonService currently instantiates EmployerService and
EmployeeService directly in its constructor, making Employer and Employee
unmockable; change the constructor signature of PersonService to accept
interfaces (e.g., IEmployerService employer, IEmployeeService employee) or
factories and assign them to the Employer and Employee properties instead of
new-ing concrete classes so tests can inject mocks; update any call sites that
construct PersonService to pass the concrete implementations (new
EmployerService(...), new EmployeeService(...)) or DI container bindings.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Around line 48-49: Remove the misplaced "Timbres" bullet from the "Gestión de
Productos/Servicios" section and consolidate its content into the existing "##
🎖️ Gestión de Timbres" section: delete the line "- **Timbres** Listar
transacciones, transferir y retirar timbres entre personas." under the "Gestión
de Productos/Servicios" header and add (or merge) that exact description into
the "## 🎖️ Gestión de Timbres" section so the functionality is documented only
once.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
43-43: Missing dash separator between bold title and inline description.Line 43 renders as
**Gestión de folios fiscales** Compra timbres…— no separator between the bold label and the explanatory text. Line 17 (- **Timbrado de facturas de nómina** - Soporte para…) and the two new bullets on lines 39-40 both include a dash or use parentheses. Adding a dash keeps the list style consistent.✏️ Suggested wording
- - **Gestión de folios fiscales** Compra timbres a FiscalAPI y transfiere/retira a las personas de tu organización según tus reglas de negocio. + - **Gestión de folios fiscales** - Compra timbres a FiscalAPI y transfiere/retira a las personas de tu organización según tus reglas de negocio.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 43, Update the markdown list item that starts with the bold phrase "**Gestión de folios fiscales**" to include a dash separator before the inline description (e.g., change "**Gestión de folios fiscales** Compra timbres..." to "**Gestión de folios fiscales** - Compra timbres...") so it matches the existing list style like the "**Timbrado de facturas de nómina** - ..." entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 39-40: The parenthetical fragment in the two bullet points for
"Datos de empleado" and "Datos de empleador" should be made grammatically
cohesive by removing the period inside the parentheses and joining the phrase;
update both lines so the parenthetical reads as a modifier (e.g., change "a una
persona. CFDI Nómina)" to "a una persona para CFDI Nómina)" or to "a una persona
(CFDI Nómina)"), ensuring the sentence flows without the detached fragment.
---
Nitpick comments:
In `@README.md`:
- Line 43: Update the markdown list item that starts with the bold phrase
"**Gestión de folios fiscales**" to include a dash separator before the inline
description (e.g., change "**Gestión de folios fiscales** Compra timbres..." to
"**Gestión de folios fiscales** - Compra timbres...") so it matches the existing
list style like the "**Timbrado de facturas de nómina** - ..." entry.
Comment on lines
+39
to
+40
| - **Datos de empleado** (agrega/actualiza/elimina datos de empleado a una persona. CFDI Nómina) | ||
| - **Datos de empleador** (agrega/actualiza/elimina datos de empleador a una persona. CFDI Nómina) |
There was a problem hiding this comment.
Parenthetical label "CFDI Nómina" reads as a detached fragment.
Both lines use the pattern a una persona. CFDI Nómina) — a full stop inside the parentheses followed by a bare noun phrase, which creates an awkward sentence fragment. Replacing the period with a conjunction keeps the parenthetical cohesive:
✏️ Suggested wording
- - **Datos de empleado** (agrega/actualiza/elimina datos de empleado a una persona. CFDI Nómina)
- - **Datos de empleador** (agrega/actualiza/elimina datos de empleador a una persona. CFDI Nómina)
+ - **Datos de empleado** (agrega/actualiza/elimina datos de empleado a una persona para CFDI Nómina)
+ - **Datos de empleador** (agrega/actualiza/elimina datos de empleador a una persona para CFDI Nómina)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Datos de empleado** (agrega/actualiza/elimina datos de empleado a una persona. CFDI Nómina) | |
| - **Datos de empleador** (agrega/actualiza/elimina datos de empleador a una persona. CFDI Nómina) | |
| - **Datos de empleado** (agrega/actualiza/elimina datos de empleado a una persona para CFDI Nómina) | |
| - **Datos de empleador** (agrega/actualiza/elimina datos de empleador a una persona para CFDI Nómina) |
🧰 Tools
🪛 LanguageTool
[grammar] ~39-~39: Aquí puede haber un error.
Context: ... de empleado a una persona. CFDI Nómina) - Datos de empleador (agrega/actualiza/e...
(QB_NEW_ES)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 39 - 40, The parenthetical fragment in the two bullet
points for "Datos de empleado" and "Datos de empleador" should be made
grammatically cohesive by removing the period inside the parentheses and joining
the phrase; update both lines so the parenthetical reads as a modifier (e.g.,
change "a una persona. CFDI Nómina)" to "a una persona para CFDI Nómina)" or to
"a una persona (CFDI Nómina)"), ensuring the sentence flows without the detached
fragment.
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 by CodeRabbit
New Features
Documentation
Improvements