Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[15.0][IMP] l10n_es_facturae: Change facturae to commercial field #3437

Merged
merged 3 commits into from
Oct 30, 2024
Merged

[15.0][IMP] l10n_es_facturae: Change facturae to commercial field #3437

merged 3 commits into from
Oct 30, 2024

Conversation

carolinafernandez-tecnativa
Copy link
Contributor

@carolinafernandez-tecnativa carolinafernandez-tecnativa commented Feb 12, 2024 •
edited
Loading

@OCA-git-bot
Copy link
Contributor

Hi @etobella,
some modules you are maintaining are being modified, check this out!

@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 15.0-imp-l10n_es_facturae-field-facturae branch 2 times, most recently from 408111d to 18d76ae Compare February 12, 2024 12:32
@pedrobaeza pedrobaeza added this to the 15.0 milestone Feb 12, 2024
@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 15.0-imp-l10n_es_facturae-field-facturae branch 3 times, most recently from 96f8d49 to 01db6e8 Compare February 12, 2024 14:05
@carolinafernandez-tecnativa carolinafernandez-tecnativa marked this pull request as ready for review February 12, 2024 14:17
pedrobaeza
pedrobaeza reviewed Feb 12, 2024
View reviewed changes
l10n_es_facturae/data/Facturaev3_2.xsd Outdated
@@ -1670,8 +1670,8 @@
</xs:complexType>
<xs:complexType name="ExtensionsType">
<xs:sequence>
<xs:any namespace="##other" processContents="strict" minOccurs="0" maxOccurs="unbounded"/>
</xs:sequence>
<xs:any namespace="##other" processContents="lax" minOccurs="0" maxOccurs="unbounded"/>
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @etobella

Copy link
Member

Choose a reason for hiding this comment

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

Esto estaba en otro commit para la 14. La idea de este cambio es permitir añadir extensiones. Seria requerido, por ejemplo, en el caso de enviar facturas a FaceB2B

Copy link
Member

Choose a reason for hiding this comment

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

Vale, pero esto no va en el mismo commit y debe llevar su propia explicación y que el título del PR y comentario principal se adapte también.

etobella
etobella reviewed Feb 13, 2024
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Por mi parte lo veo bien, pero viendo que has añadido otros commits, estaria bien separarlo con los commits originales para ver la historia correcta

l10n_es_facturae/data/Facturaev3_2.xsd Outdated
@@ -1670,8 +1670,8 @@
</xs:complexType>
<xs:complexType name="ExtensionsType">
<xs:sequence>
<xs:any namespace="##other" processContents="strict" minOccurs="0" maxOccurs="unbounded"/>
</xs:sequence>
<xs:any namespace="##other" processContents="lax" minOccurs="0" maxOccurs="unbounded"/>
Copy link
Member

Choose a reason for hiding this comment

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

Esto estaba en otro commit para la 14. La idea de este cambio es permitir añadir extensiones. Seria requerido, por ejemplo, en el caso de enviar facturas a FaceB2B

@carolinafernandez-tecnativa carolinafernandez-tecnativa marked this pull request as draft February 13, 2024 16:38
@carolinafernandez-tecnativa carolinafernandez-tecnativa marked this pull request as ready for review February 13, 2024 16:54
@carolinafernandez-tecnativa
Copy link
Contributor Author

Por mi parte lo veo bien, pero viendo que has añadido otros commits, estaria bien separarlo con los commits originales para ver la historia correcta

Ya lo he separado en distintos commits para ver la historia FYI @pedrobaeza

@etobella
Copy link
Member

Aplican los mismos comentarios puestos en #3439

@pedrobaeza
Copy link
Member

Tiene conflicto. ¿Esto aún aplica?

@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 15.0-imp-l10n_es_facturae-field-facturae branch 2 times, most recently from 6b79bd7 to 1b262bb Compare March 27, 2024 14:16
@carolinafernandez-tecnativa
Copy link
Contributor Author

carolinafernandez-tecnativa commented Mar 27, 2024 •
edited
Loading

Aplican los mismos comentarios puestos en #3439

Se aplicaron los cambios solicitados

@pedrobaeza pedrobaeza requested review from etobella and removed request for etobella March 27, 2024 16:16
@carolinafernandez-tecnativa
Copy link
Contributor Author

ping @etobella

etobella
etobella reviewed May 15, 2024
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Has añadido un código que solo tiene sentido en envíos a FACe Puedes mirarlo?

l10n_es_facturae/models/account_move.py Outdated
@@ -147,6 +148,10 @@ def validate_facturae_fields(self):
raise ValidationError(_("Partner vat is too small"))
if not self.partner_id.state_id:
raise ValidationError(_("Partner state not provided"))
if not self.partner_id.unidad_tramitadora:
Copy link
Member

Choose a reason for hiding this comment

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

No recuperes este código. Solo tiene sentido para envíos a FACe, en el que tendremos a futuro B2B no tiene sentido

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vale entonces dices que debo de cambiarlo de lugar para el FACe? Así como valido que esté completo el Organo gestor, aplicaría la validación de la Unidad Tramitadora no?

Copy link
Member

Choose a reason for hiding this comment

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

Los campos organo gestor, unidad tramitadora y oficina contable solo deberían ser obligatorios si envío a FACe. Por ejemplo, en FACeB2B no es así. Cuando lo hicimos en su dia, no lo contemplamos, aunque a dia de hoy, al conocerlo, no hace ningún daño irlo modificando

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya quedaron los cambios aplicados

l10n_es_facturae/tests/common.py Outdated Show resolved Hide resolved
l10n_es_facturae/tests/common.py Outdated Show resolved Hide resolved
@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 15.0-imp-l10n_es_facturae-field-facturae branch from 1b262bb to cca55f7 Compare May 16, 2024 14:43
@pedrobaeza
Copy link
Member

ping @carolinafernandez-tecnativa

@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 15.0-imp-l10n_es_facturae-field-facturae branch from cca55f7 to af362d9 Compare October 30, 2024 10:09
@pilarvargas-tecnativa pilarvargas-tecnativa force-pushed the 15.0-imp-l10n_es_facturae-field-facturae branch from af362d9 to 6dd54ad Compare October 30, 2024 10:27
@pilarvargas-tecnativa
Copy link

ping @pedrobaeza

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 15.0-ocabot-merge-pr-3437-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit ea207cf into OCA:15.0 Oct 30, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 14c82eb. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 15.0-imp-l10n_es_facturae-field-facturae branch October 30, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pilarvargas-tecnativa pilarvargas-tecnativa pilarvargas-tecnativa left review comments

@etobella etobella etobella approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@carolinafernandez-tecnativa @OCA-git-bot @etobella @pedrobaeza @pilarvargas-tecnativa