Skip to content

Conversation

svillegas-cdd
Copy link
Contributor

  • Added RcvCompraCsvRowSchema with extended fields support.
  • Updated RCV compra row schemas to inherit from the base schema.
  • Updated models to include new fields in RcDetalleEntry and its variants.
  • Refactored field removal logic, retaining only Nro for exclusion.

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

@svillegas-cdd svillegas-cdd requested a review from Copilot August 28, 2025 13:14
@svillegas-cdd svillegas-cdd self-assigned this Aug 28, 2025
@svillegas-cdd svillegas-cdd added enhancement New feature or request component: rcv labels Aug 28, 2025
Copilot

This comment was marked as outdated.

@svillegas-cdd svillegas-cdd force-pushed the task/sc-16110-parse-rcvdata-to-create-entrada-registro-compras branch from 36a297b to adb421a Compare August 28, 2025 14:34
@svillegas-cdd svillegas-cdd marked this pull request as ready for review August 28, 2025 14:35
@svillegas-cdd svillegas-cdd requested a review from a team as a code owner August 28, 2025 14:35
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 88.20513% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.54%. Comparing base (17ed246) to head (8e8419a).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/cl_sii/rcv/parse_csv.py 84.78% 11 Missing and 10 partials ⚠️
src/cl_sii/rcv/data_models.py 96.49% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #864      +/-   ##
===========================================
- Coverage    88.65%   88.54%   -0.12%     
===========================================
  Files           38       38              
  Lines         3359     3483     +124     
  Branches       332      340       +8     
===========================================
+ Hits          2978     3084     +106     
- Misses         238      248      +10     
- Partials       143      151       +8     

☔ 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.

Copy link

@Copilot 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 expands the RCV Compras CSV schemas to support extended field mappings and refactors the schema hierarchy to reduce code duplication. The main purpose is to add a base RcvCompraCsvRowSchema with common fields that all compra schemas inherit from, and update the corresponding data models to include all the new fields.

Key changes include:

  • Created a new base RcvCompraCsvRowSchema with extended field definitions
  • Refactored existing compra schemas to inherit from the base schema
  • Added RcDetalleEntry as a base class for all RC detail entries
  • Simplified field removal logic to only exclude the Nro field

Reviewed Changes

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

File Description
src/cl_sii/rcv/parse_csv.py Added base RcvCompraCsvRowSchema with extended fields and refactored existing schemas to inherit from it
src/cl_sii/rcv/data_models.py Created RcDetalleEntry base class with common fields and updated existing entry classes to inherit from it
src/tests/test_rcv_parse_csv.py Added comprehensive test implementations for all compra CSV row schemas
src/tests/test_rcv_data_models.py Updated test data to include the new required fields

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-16110-parse-rcvdata-to-create-entrada-registro-compras branch 3 times, most recently from e5dc546 to 2843446 Compare August 28, 2025 14:51
- Added `RcvCompraCsvRowSchema` with extended fields support.
- Updated RCV compra row schemas to inherit from the base schema.
- Updated models to include new fields in `RcDetalleEntry` and its variants.
- Refactored field removal logic, retaining only `Nro` for exclusion.

Ref: https://app.shortcut.com/cordada/story/16110/
@svillegas-cdd svillegas-cdd force-pushed the task/sc-16110-parse-rcvdata-to-create-entrada-registro-compras branch from 2843446 to 8e8419a Compare August 28, 2025 16:29
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

@svillegas-cdd svillegas-cdd merged commit d08463a into develop Aug 28, 2025
19 of 21 checks passed
@svillegas-cdd svillegas-cdd deleted the task/sc-16110-parse-rcvdata-to-create-entrada-registro-compras branch August 28, 2025 16:52
@svillegas-cdd svillegas-cdd mentioned this pull request Sep 2, 2025
Copy link
Member

@jtrobles-cdd jtrobles-cdd left a comment

Choose a reason for hiding this comment

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

pass
def test_parse_rcv_compra_registro_row(self) -> None:
schema_context = dict(
emisor_rut=Rut('1-9'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
emisor_rut=Rut('1-9'),
receptor_rut=Rut('1-9'),

'fecha_emision_date': datetime.date(2019, 6, 21),
'monto_total': 285801,
'emisor_razon_social': 'Fake Company S.A.',
'receptor_rut': Rut('1-9'),
Copy link
Member

Choose a reason for hiding this comment

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

  • The RcvCompraRegistroCsvRowSchema did not attempt to obtain the receptor_rut from the context. If it did, we would have detected that the context contains the emisor_rut instead of the receptor_rut.
  • Using RcvCompraRegistroCsvRowSchema.to_detalle_entry() to test parsing is not enough because it skips all the parsing work done by Marshmallow, such as reading data from the context. To really test parsing, you need to pass the raw data to RcvCompraRegistroCsvRowSchema.load(), and then pass the output of load() to to_detalle_entry().
Suggested change
'receptor_rut': Rut('1-9'),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants