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.

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

[13.0][IMP] intrastat*: brexit support #179

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Jun 14, 2022

Backport from 14.0: #163

Please @pedrobaeza can you review it?

@Tecnativa TT37486

@victoralmau victoralmau force-pushed the 13.0-intrastat-brexit-support branch from 329803a to 2c4e69f Compare June 14, 2022 08:26
@victoralmau victoralmau changed the title [13.0] intrastat*: brexit support [13.0][IMP] intrastat*: brexit support Jun 14, 2022
@luc-demeyer
Copy link
Contributor

@victoralmau
I had placed this action on my todo list but now it's done already, top !.

But there is something wrong with this PR:
Stack trace when I test it;:

File "/opt/odoo13/OCA/intrastat-extrastat/intrastat_product/models/intrastat_product_declaration.py", line 762, in action_gather
lines = self._gather_invoices()
File "/opt/odoo13/OCA/intrastat-extrastat/intrastat_product/models/intrastat_product_declaration.py", line 625, in _gather_invoices
partner_country = self._get_partner_country(inv_line, eu_countries)
File "/opt/odoo13/OCA/intrastat-extrastat/intrastat_product/models/intrastat_product_declaration.py", line 288, in _get_partner_country
self._format_line_note(inv_line, line_notes)
TypeError: _format_line_note() missing 1 required positional argument: 'line_notes'

The signature of the _format_line_note in 13.0 is:
def _format_line_note(self, line, line_nbr, line_notes):
In 14.0 this signature has changed to:
def _format_line_note(self, line, notedict, line_notes):

Backporting also the unit tests that we have in 14.0 would be great so that we detect such issues automatically.

@victoralmau victoralmau force-pushed the 13.0-intrastat-brexit-support branch from bb4ec2c to 1977ab8 Compare June 15, 2022 06:51
@victoralmau
Copy link
Member Author

Changes done and tests solved.

@pedrobaeza pedrobaeza added this to the 13.0 milestone Jun 15, 2022
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Squash commits

product_origin_country_code = fields.Char(
string="Country of Origin of the Product",
size=2,
required=True,
Copy link
Member

Choose a reason for hiding this comment

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

What happen with existing lines not having this filled?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is a strange behavior, but it is what was added in v14. In my opinion, this should be a compute store field. What do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

We should keep the changes at minimum to comply with the Brexit requirement

Copy link
Member Author

Choose a reason for hiding this comment

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

The product_origin_country_code field was added by the v14 PR to add the Brexit requirement (it is not country-related because it depends on the state).

You can change that field and make it a compute store, although it would be necessary to add a product_origin_state_id field as well (this would avoid overwriting the create() and write() of the account.move).
In any case, migration scripts will be necessary. Which way do you think is better?

return super().write(vals)

def _format_vals(self, vals):
if "product_origin_country_code" in vals:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

string="Country Code",
compute="_compute_src_dest_country_code",
store=True,
required=True,
Copy link
Member

Choose a reason for hiding this comment

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

The same for this.

product_origin_country_code = fields.Char(
string="Country of Origin of the Product",
size=2,
required=True,
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 continue saying, but there's a lot of new required fields with the same problem.

intrastat_product/tests/common.py Show resolved Hide resolved
luc-demeyer and others added 2 commits June 16, 2022 09:29
[14.0]intrastat_product - Intrastat Brexit support

fix unit tests - keep product_origin_country_id on top of new product_origin_country_code to avoid breaking the localisation modules

[14.0]intrastat - improved brexit support

[14.0]improved brexit support - keep backwards compatibility with localisation modules

[14.0]brexit support - increase test coverage

[14.0]brexit support - DeprecationWarning

[14.0]brexit support - size=2 on product_origin_country_code
@victoralmau victoralmau force-pushed the 13.0-intrastat-brexit-support branch from 1977ab8 to 446dcb3 Compare June 16, 2022 07:31
@luc-demeyer
Copy link
Contributor

Product Origin country code is XI, XU for brexit support.
This is a required field for the declaration.

@victoralmau
Copy link
Member Author

Product Origin country code is XI, XU for brexit support. This is a required field for the declaration.

Ping @pedrobaeza

@pedrobaeza
Copy link
Member

I don't remember what I have to check here, but please go on in the best way you think. There is also a "Requested changes" from @luc-demeyer. Is it attended?

@victoralmau
Copy link
Member Author

I don't remember what I have to check here, but please go on in the best way you think. There is also a "Requested changes" from @luc-demeyer. Is it attended?

There was really nothing to validate (you asked if everything added since v14 was necessary). The changes proposed by @luc-demeyer have already been made so it would be ready to merge.

@pedrobaeza
Copy link
Member

OK, let's merge it then:

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-179-by-pedrobaeza-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0efb503 into OCA:13.0 Aug 9, 2022
@OCA-git-bot
Copy link
Contributor

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

@JordiBForgeFlow
Copy link
Sponsor Member

JordiBForgeFlow commented Aug 9, 2022

@JordiMForgeFlow
Copy link

JordiMForgeFlow commented Aug 9, 2022

@victoralmau @pedrobaeza buddies, where is the method '_compute_src_dest_country_code' https://github.com/OCA/intrastat-extrastat/pull/179/files#diff-74e1197466e3c9a2c08b006d373e86cca69ac389f34c9e4ed38dc24b84018d80R1000 defined??

In V14 I don't see it being a computed field

I think we need to BP this fix: 3d4e311

@JordiBForgeFlow
Copy link
Sponsor Member

@victoralmau please backport #174 ASAP

@JordiBForgeFlow
Copy link
Sponsor Member

JordiBForgeFlow commented Aug 9, 2022

@victoralmau please make sure that you review and backport all commits from 14.0

@victoralmau
Copy link
Member Author

The missing change is now merged #188

@JordiBForgeFlow
Copy link
Sponsor Member

@victoralmau thanks!!

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.

None yet

6 participants