Skip to content

Conversation

@svillegas-cdd
Copy link
Contributor

  • This change addresses cases where multiple CSV rows with empty fields appeared, representing additional impuestos linked to the same folio as the main row.
  • Replaced individual fields (codigo_otro_impuesto, valor_otro_impuesto, tasa_otro_impuesto) with unified otros_impuestos structure.
  • Updated schemas, data models, and deserializers to handle "Otros Impuestos".
  • Adjusted tests and test data to validate new structure and edge cases.

Ref: https://app.shortcut.com/cordada/story/16788/

@svillegas-cdd svillegas-cdd self-assigned this Oct 10, 2025
@svillegas-cdd svillegas-cdd added bug Something isn't working enhancement New feature or request component: rcv labels Oct 10, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the handling of "Otros Impuestos" fields in RCV CSV parsing by consolidating three separate fields into a unified structure. The change addresses cases where multiple CSV rows with empty fields represent additional tax information linked to the same invoice folio.

  • Replaced individual codigo_otro_impuesto, valor_otro_impuesto, and tasa_otro_impuesto fields with a unified otros_impuestos list structure
  • Updated CSV parsing logic to group rows by folio and aggregate "Otros Impuestos" data from additional rows
  • Modified data models and schemas to support the new structure with proper type definitions

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/tests/test_rcv_parse_csv.py Updated test cases to use new otros_impuestos structure and added test for handling empty impuestos rows
src/tests/test_data_models.py Updated test fixtures to use new otros_impuestos field instead of individual fields
src/tests/test_data/sii-rcv/RCV-venta-extra-empty-impuestos-rows.csv New test data file with multiple rows for testing folio grouping functionality
src/tests/test_data/sii-rcv/RCV-compra-reclamado.csv Updated test data to include "Otros Impuestos" values in the first row
src/cl_sii/rcv/parse_csv.py Refactored schema definitions and parsing logic to handle grouped "Otros Impuestos" data
src/cl_sii/rcv/data_models.py Updated data models with new OtrosImpuestos TypedDict and replaced individual fields
src/cl_sii/libs/rows_processing.py Added folio grouping logic to aggregate "Otros Impuestos" data from multiple CSV rows
Comments suppressed due to low confidence (2)

src/cl_sii/rcv/parse_csv.py:1

  • The Decimal import is removed but still appears to be used in the code. Verify that all usages have been replaced or that the import is still needed.
"""

src/cl_sii/rcv/parse_csv.py:1003

  • The receptor_rut field definition appears to be removed but this may break the schema functionality. Ensure this removal is intentional and doesn't affect CSV parsing.
    receptor_rut = mm_fields.RutField(
        required=True,

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 92.68293% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.90%. Comparing base (cc7ea5d) to head (839a2d4).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/cl_sii/rcv/parse_csv.py 91.54% 2 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #913      +/-   ##
===========================================
- Coverage    89.40%   88.90%   -0.50%     
===========================================
  Files           40       40              
  Lines         3728     3759      +31     
  Branches       378      389      +11     
===========================================
+ Hits          3333     3342       +9     
- Misses         243      265      +22     
  Partials       152      152              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@svillegas-cdd svillegas-cdd marked this pull request as ready for review October 10, 2025 17:28
@svillegas-cdd svillegas-cdd requested a review from a team as a code owner October 10, 2025 17:28
@svillegas-cdd svillegas-cdd force-pushed the task/sc-16788-rcv-processing-errors-otros-impuestos branch from 4c80325 to 19f5854 Compare October 10, 2025 17:28
@svillegas-cdd svillegas-cdd force-pushed the task/sc-16788-rcv-processing-errors-otros-impuestos branch from 19f5854 to 724f5df Compare October 14, 2025 21:19
@svillegas-cdd svillegas-cdd requested a review from Copilot October 14, 2025 21:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/cl_sii/rcv/parse_csv.py:1005

  • The receptor_rut field definition appears to be removed from RcvCompraCsvRowSchema but this field is likely still needed for the schema to function properly. This could cause validation errors when processing CSV files.
    receptor_rut = mm_fields.RutField(
        required=True,
    )

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@svillegas-cdd svillegas-cdd force-pushed the task/sc-16788-rcv-processing-errors-otros-impuestos branch from 724f5df to 1de3daf Compare October 14, 2025 21:33
- This change addresses cases where multiple CSV rows with empty fields appeared,
representing additional impuestos linked to the same folio as the main row.
- Replaced individual fields (`codigo_otro_impuesto`, `valor_otro_impuesto`, `tasa_otro_impuesto`) with unified `otros_impuestos` structure.
- Updated schemas, data models, and deserializers to handle "Otros Impuestos".
- Adjusted tests and test data to validate new structure and edge cases.

Ref: https://app.shortcut.com/cordada/story/16788/
@svillegas-cdd svillegas-cdd force-pushed the task/sc-16788-rcv-processing-errors-otros-impuestos branch from 1de3daf to 839a2d4 Compare October 15, 2025 13:57
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
14.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@svillegas-cdd
Copy link
Contributor Author

@jtrobles-cdd The PR is been blocked by coverage, could we merge it?

@jtrobles-cdd jtrobles-cdd merged commit 5268b6e into develop Oct 15, 2025
19 of 21 checks passed
@jtrobles-cdd jtrobles-cdd deleted the task/sc-16788-rcv-processing-errors-otros-impuestos branch October 15, 2025 17:12
@svillegas-cdd svillegas-cdd mentioned this pull request Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working component: rcv enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants