[MIG] 8.0 l10n_nl_intrastat #58

thomaspaulb · 2016-12-02T17:05:11Z

Imported from Launchpad, ported to v8, added tests.

hbrunn · 2016-12-02T18:20:48Z

did you consider using this https://github.com/OCA/maintainer-tools/wiki/Migration-Launchpad-%E2%86%92-GitHub method to import from launchpad, this way keeping authors and commits?

thomaspaulb · 2016-12-02T18:34:38Z

I saw it but I somehow thought it was only for migrating full repos, not single modules to add to an existing OCA repo. Is it useful to keep history for this one?

hbrunn · 2016-12-02T18:54:19Z

Sometimes it's helpful for stuff, but it's also a lot of extra work. You'd have to filter the commits for the directory in question as outlined in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-8.0#how

Let's see what other reviewers think and do nothing for now

hbrunn · 2017-02-23T15:01:27Z

@thomaspaulb can you rebase?

astirpe · 2017-03-02T14:19:50Z

oca_dependencies.txt

@@ -1 +1,2 @@
 partner-contact https://github.com/OCA/partner-contact 8.0
+intrastat https://github.com/sunflowerit/intrastat 8.0-intrastat_base-tests


Module intrastat_base is available in OCA for V8, so I think you can set the OCA repository intrastat here, instead of the sunflowerit branch. Or is it a temporary solution for some reason?

it was probably not merged at the time of the PR, so this was necessary to get the tests right. But by now, this should indeed just be 'intrastat'

(took the liberty to do exactly that in order to continue with this)

astirpe · 2017-03-02T14:31:18Z

About keeping authors and commits when importing from launchpad, I think the decision is up to the Authors (@Therp, @alexis-via, etc..).

hbrunn · 2017-03-02T14:49:08Z

We don't really mind attribution wise as the license headers are kept intact.

For git migrations from older branches I'd enforce this, because it makes cherry picking stuff back and forth between the different versions a lot simpler. But this is not going to happen with launchpad, so technically, it wouldn't be useful.

@StefanRijnhart what do you think? It's your commits that are mutilated here

astirpe

@thomaspaulb thanks for porting this module.

Here is just a code review, I didn't test the module.

astirpe · 2017-03-02T15:42:26Z

l10n_nl_intrastat/models/l10n_nl_intrastat.py

+        """
+        Do not allow unlinking of confirmed reports
+        """
+        if self.search([('state', '!=', 'draft')]):


This porting of unlink() method would not work as expected: it actually checks "all" the records that are not in draft.
It should be instead something like:

for report in self: if report.state != 'draft': raise ...

astirpe · 2017-03-02T15:49:49Z

l10n_nl_intrastat/__openerp__.py

+    'depends': ['intrastat_base'],
+    'data': [
+        'views/l10n_nl_intrastat.xml',
+        'security/ir.model.access.csv',


security/* files are commonly declared before the views/* files

astirpe · 2017-03-02T15:49:53Z

l10n_nl_intrastat/__openerp__.py

+    'license': 'AGPL-3',
+    'summary': """Intrastat report for the Netherlands""",
+    'author': 'Therp BV, Odoo Community Association (OCA)',
+    'website': 'https://launchpad.net/new-report-intrastat',


Could we think of an updated website to propose?
Maybe https://github.com/OCA/intrastat ?

astirpe · 2017-03-02T15:50:59Z

l10n_nl_intrastat/i18n/l10n_nl_intrastat.pot

+#. module: l10n_nl_intrastat
+#: field:l10n_nl.report.intrastat,date_done:0
+msgid "Date done"
+msgstr ""


File .pot could be safely removed, is not required anymore

astirpe · 2017-03-02T15:52:24Z

l10n_nl_intrastat/README.rst

+===========
+
+Bugs are tracked on `GitHub Issues
+<https://github.com/OCA/{project_repo}/issues>`_. In case of trouble, please


The link here should be https://github.com/OCA/l10n-netherlands/issues

astirpe · 2017-03-03T10:08:36Z

l10n_nl_intrastat/models/l10n_nl_intrastat.py

+        string='Period',
+        comodel_name='account.period',
+        states={'done': [('readonly', True)]},
+        default=_default_period_id,


Here:
default=lambda self: self._default_period_id(),

astirpe · 2017-03-03T10:11:32Z

l10n_nl_intrastat/models/l10n_nl_intrastat.py

+        required=True,
+        states={'done': [('readonly', True)]},
+        help="Related company.",
+        default=_default_company_id,


Here:
default=lambda self: self.env.user.company_id

and method _default_company_id() can be removed.

astirpe · 2017-03-03T10:22:38Z

l10n_nl_intrastat/models/l10n_nl_intrastat.py

+        for line_obj in self.line_ids:
+            line_obj.unlink()
+        # Define search for invoices for period and company:
+        company_obj = self.company_id  # simplify access


Here if you replace the name of the variable from company_obj to company (and do the same for its usage in the subsequent lines), then you are compliant with the OCA guidelines

astirpe · 2017-03-03T10:22:41Z

l10n_nl_intrastat/models/l10n_nl_intrastat.py

+        self._check_generate_lines()
+
+        # Remove existing lines
+        for line_obj in self.line_ids:


You can simply do this:
self.line_ids.unlink()

astirpe · 2017-03-03T10:36:10Z

l10n_nl_intrastat/models/l10n_nl_intrastat.py

+            ('company_id', '=', company_obj.id),
+        ]
+        # Signal invoices without a country
+        # (Should not happen, as country_id on parter is set to required)


Typo: parter -> partner
But real question is: why this comment line? country_id on partner is not required by default.
I think this comment line should be removed. I would prefer this way, instead of setting country_id as required on partner.

andreschenkels · 2017-08-30T07:51:30Z

Do you guys need some help here?

thomaspaulb · 2017-08-30T07:55:37Z

Hey @aschenkels-ictstudio, of course help is always appreciated :-) If I remember correctly, after the code sprint in Barcelona me and @astirpe managed to get the 9.0 and 10.0 versions accepted, but this one slipped my mind. @astirpe what did we suggest for this module.. backporting or resuming with this one?

hbrunn · 2017-08-30T07:57:45Z

@thomaspaulb you already made https://github.com/OCA/l10n-netherlands/tree/9.0/l10n_nl_intrastat, don't you think it's simpler to backport that? Originally the 9 version cam from this I think to remember, so that might just work with cherry picking and changing some xmlids?

thomaspaulb · 2017-08-30T08:35:05Z

@hbrunn I think so too. The 9.0 version was what we finished, reviewed and polished in the end, also the unit tests there are more developed. So it's best to just backport that, run the unit tests and do some fixing here and there. @aschenkels-ictstudio do I hear you volunteering for the job?

astirpe · 2017-08-30T09:20:37Z

@thomaspaulb I think doing a backport from 9.0 is faster, but be careful there are some aspects to consider:

The version 9.0 of this module makes use of date_range and gets rid of V8.0 fiscal periods. It means that, if you go for the backporting way, you will have to ensure that you continue to use periods instead of date_range (module date_range is not available in V8.0 and I don't think it will be ever backported).
The Monetary field type used in V9.0 is not existing in V8.0

... and surely I'm missing something else. So at the end you will have to resume part of the code V8.0.

Whatever will be your choice, I will be happy to review it :)

hbrunn · 2017-10-26T09:13:29Z

as of the discussion above I close this one

thomaspaulb added 9 commits November 29, 2016 13:53

import code from launchpad

91065dd

bump version

40cd297

Repair broken check_generate_lines call

96cd2db

encoding, folder structure

6a5a0d5

tests: generation of report and checking of total amount

2faf739

linting fixes

0509415

readme

2592b22

flake8

c2a9a59

flake

09aec5f

hbrunn added the needs review label Dec 2, 2016

hbrunn added this to the 8.0 milestone Dec 2, 2016

thomaspaulb added 2 commits January 11, 2017 16:55

short license headers and flake errors

53554dd

oca dependencies

9871f31

astirpe reviewed Mar 2, 2017

View reviewed changes

hbrunn added the needs fixing label Mar 2, 2017

Merge branch '8.0' into 8.0-mig-l10n_nl_intrastat

0157b1d

hbrunn removed the needs fixing label Mar 2, 2017

astirpe suggested changes Mar 3, 2017

View reviewed changes

hbrunn closed this Oct 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG] 8.0 l10n_nl_intrastat #58

[MIG] 8.0 l10n_nl_intrastat #58

thomaspaulb commented Dec 2, 2016

hbrunn commented Dec 2, 2016

thomaspaulb commented Dec 2, 2016

hbrunn commented Dec 2, 2016

hbrunn commented Feb 23, 2017

astirpe Mar 2, 2017

hbrunn Mar 2, 2017

hbrunn Mar 2, 2017

astirpe commented Mar 2, 2017

hbrunn commented Mar 2, 2017

astirpe left a comment

astirpe Mar 2, 2017

astirpe Mar 2, 2017

astirpe Mar 2, 2017

astirpe Mar 2, 2017

astirpe Mar 2, 2017

astirpe Mar 3, 2017

astirpe Mar 3, 2017

astirpe Mar 3, 2017

astirpe Mar 3, 2017

astirpe Mar 3, 2017

andreschenkels commented Aug 30, 2017

thomaspaulb commented Aug 30, 2017

hbrunn commented Aug 30, 2017

thomaspaulb commented Aug 30, 2017

astirpe commented Aug 30, 2017

hbrunn commented Oct 26, 2017

		@@ -1 +1,2 @@
		partner-contact https://github.com/OCA/partner-contact 8.0
		intrastat https://github.com/sunflowerit/intrastat 8.0-intrastat_base-tests

[MIG] 8.0 l10n_nl_intrastat #58

[MIG] 8.0 l10n_nl_intrastat #58

Conversation

thomaspaulb commented Dec 2, 2016

hbrunn commented Dec 2, 2016

thomaspaulb commented Dec 2, 2016

hbrunn commented Dec 2, 2016

hbrunn commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astirpe commented Mar 2, 2017

hbrunn commented Mar 2, 2017

astirpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreschenkels commented Aug 30, 2017

thomaspaulb commented Aug 30, 2017

hbrunn commented Aug 30, 2017

thomaspaulb commented Aug 30, 2017

astirpe commented Aug 30, 2017

hbrunn commented Oct 26, 2017