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

11.0 mig product harmonized system #38

Merged
merged 17 commits into from
Aug 20, 2018

Conversation

rvalyi
Copy link
Member

@rvalyi rvalyi commented Jul 30, 2018

supersedes #29
additionally, it

  • removes the Python 2 utf-8 headers
  • fixes the README icon
  • apply the latest v10.0 fixes from Noviat

cc @BT-kaberer @feketemihai @alexis-via @astirpe @luc-demeyer

alexis-via and others added 14 commits March 12, 2018 07:44
…n intrastat

This code has been written both by Luc de Meyer and myself.
Copy incoterms and destination country from SO to invoice when invoicing from SO
We need weight even when supplementary units is used
Small cleanups and enhancements
Add default values for intrastat transaction on company
Code cleanup
Rename variables
Set 2 other modules to uninstallable
Update README.rst: switch to new intrastat project
Special thanks to Pedro and Holger for finding the solution for display_name
Fix demo data
Copy link
Member

@astirpe astirpe left a comment

Choose a reason for hiding this comment

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

Could you also add the readme files? See latest template https://github.com/OCA/maintainer-tools/tree/master/template/module/readme

</record>

<!-- Menu entry for H.S. code -->
<!-- TODO: find a way to put a menu entry without depending on another module ?
Copy link
Member

Choose a reason for hiding this comment

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

An idea could be adding a button or a link in the 'res.config.settings' that triggers the hs_code_action. This way you don't need to depend on another module.

@@ -0,0 +1,30 @@
# © 2018 brain-tec AG (http://www.braintec-group.com)
Copy link
Member

Choose a reason for hiding this comment

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

Could you replace the symbol © with the string Copyright in the headers?

"available from the World Customs Organisation, see "
"http://www.wcoomd.org")
description = fields.Char(
'Description', translate=True,
Copy link
Member

Choose a reason for hiding this comment

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

No need of 'Description'

compute='_compute_display_name_field', string="Display Name",
store=True, readonly=True)
local_code = fields.Char(
string='Local Code', 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.

No need of string='Local Code'

'Description', translate=True,
help="Short text description of the H.S. category")
display_name = fields.Char(
compute='_compute_display_name_field', string="Display Name",
Copy link
Member

Choose a reason for hiding this comment

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

No need of string="Display Name"

Copy link
Contributor

Choose a reason for hiding this comment

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

Code review & tested.
Remarks of @astirpe are valuable hence would be nice to include but imho not blocking for merging this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@astirpe I fully agree, hence if @rvalyi is prepared to adjust the PR accordingly than it's aligned more closely to the OCA guidelines. Imho not blocking though for merge. I prefer to have this one merged asap so that we can focus on the larger work (port of intrastat_product for which I would first like to have a couple of major fixes and enhancements in the 10.0 version merged and use this as the basis for the port).

@rvalyi rvalyi mentioned this pull request Aug 20, 2018
@luc-demeyer luc-demeyer merged commit ebf5f20 into OCA:11.0 Aug 20, 2018
@luc-demeyer
Copy link
Contributor

@rvalyi @astirpe
Merged whereby I also added the suggestions of andrea stirpe.
A remaining point is the addition of a menu entry where andrea suggests to work via a config option.
An alternative could be an auto-install module depending on 'sale' and 'product_harmonized_system'.
What do you think ?

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

Successfully merging this pull request may close these issues.

None yet

5 participants