Added Multi company support #16

Martronic-SA · 2017-06-19T09:30:12Z

Could you check this?
I've added company_ids and currency_id in MIS report instance.
For the moment, the rate calcuation is not implemented (will be soon).

sbidoul

Thanks for this contrib!

sbidoul · 2017-06-19T09:48:40Z

mis_builder/models/aep.py

-    def __init__(self, company):
-        self.company = company
-        self.dp = company.currency_id.decimal_places
+    def __init__(self, company_ids=None, currency_id=None):


Can you name that companies instead of company_ids.

sbidoul · 2017-06-19T09:52:14Z

mis_builder/models/aep.py

        else:
-            aml_model = self.company.env[aml_model]
+            aml_model = self.company_ids[0].env[aml_model]


self.companies.env[aml_model] : [0] is not needed

sbidoul · 2017-06-19T09:54:44Z

mis_builder/models/aep.py

+            [self.env.user.company_id]
+        self.currency_id = currency_id and currency_id or \
+            self.env.user.company_id.currency_id
+        self.dp = self.currency_id.decimal_places


Could you keep the currency stuff in separate commits? So we different commit(s) for multi company and multi currency and a clean commit history.

sbidoul · 2017-06-19T10:01:48Z

mis_builder/models/aep.py

@@ -238,7 +241,7 @@ def get_aml_domain_for_dates(self, date_from, date_to,
        elif mode == self.MODE_UNALLOCATED:
            date_from_date = fields.Date.from_string(date_from)
            fy_date_from = \
-                self.company.\
+                self.company_ids[0].\


Hm. Here it's a special treatment for the first company. This could lead to surprises.

Could you extract the fy_date_from computation in a separate method and add a check (raise UserError) that all companies have the same fy_date_from? Later we can add logic to handle companies with different fiscal years.

I can see some border effects when company is located in Australia with GMT +12, but this can be fixed.
However don't you think that for this particular case (at least) the main company (or user.company_id) is useful?

It could be useful to have a main company but not in AEP. Perhaps in mis_report_instance. But even that I'm not sure and I certainly have not studied the impact of having different fiscal years on the computation of consolidated initial balances... Until that is clarified I prefer to raise an error if the various companies have different fiscal year dates.

sbidoul · 2017-06-19T10:04:20Z

mis_builder/models/mis_report_instance.py

+        help='Select companies for which data will  be searched. \
+            User\'s company if empty.',
+        default=_default_company_ids,
+        required=False)


Could you use this style for new/modified fields?

company_ids = fields.Many2many( comodel_name='res.company', string='Company', help='Select companies for which data will be searched. ' 'User\'s company if empty.', default=_default_company_ids, required=False, )

A migration script is needed.

sbidoul · 2017-06-19T10:04:51Z

mis_builder/models/mis_report_instance.py

+        default=_default_company_ids,
+        required=False)
+    currency_id = fields.Many2one('res.currency', 'Currency',
+        required=False)


Could you please do multi currency in another PR?

sbidoul · 2017-06-19T10:05:49Z

mis_builder/models/mis_report_instance.py

-            _company_default_get('mis.report.instance')
+    def _default_company_ids(self):
+        return [(6,0,[self.env['res.company'].\
+            _company_default_get('mis.report.instance').id])]


flake8 will complain here

I'm wondering if return _company_default_get('mis.report.instance') would work too?

I'm wondering if return _company_default_get('mis.report.instance') would work too?

That's what I was using first, but there were a json complaint about res.company not serialisable.
Why flake8 will complain?

flake8 requires a space after commas

sbidoul · 2017-06-19T10:10:18Z

Can you add a new test for multi-company AEP?

sbidoul · 2017-06-19T10:12:36Z

mis_builder/tests/test_aep.py

            time.strftime('%Y') + '-03-31',
            'posted')
        self.assertEquals(end, {
            self.account_ar.id: (900, 0),
            self.account_in.id: (0, 800),
        })
        unallocated = AEP.get_unallocated_pl(
-            self.company,
+            self.company_ids,


test_aep.py should be unchanged: existing tests in this file should continue to work without change.

So you the the AEP init method shoud accept company and companies as first argument and I shoud add check whether theare are one or many companies passed?

What I mean is that for Odoo, a recordset of 1 is still a recordset, and create returns a recordset of 1. So these tests can remain unmodified and work normally even if AEP.__init__ accept companies as argument.

Martronic-SA · 2017-06-21T09:01:10Z

Even if currency support is in a separate request, we still need to choose a main currency for total amounts calculation as account.move.lines might be in different currencies.
Do you see a way to output the main currency in the report?
What do you think about having a cell of type "Monetary" that would format the number as Monetary field according to the main currency in report? or a new checkbox in the style for "Monetary" ?

EDITED : This PR

sbidoul · 2017-06-21T12:57:19Z

Regarding this PR, a few things remain todo before merging;

a test for multi-company
a few flak8 errors: https://travis-ci.org/acsone/account-financial-reporting/jobs/245282951#L546-L548
a migration script for mis.report.instance.company_id -> company_ids

Regarding multi-currency I would not worry too much with the display issue: we currently do it with a suffix/prefix in the styles, it does the job. Doing the currency conversion in an efficient manner is the key problem to resolve.

sbidoul · 2017-06-21T13:29:41Z

Please please do the currency conversion in another pull request so this one remains reasonably small and easy to review.

Martronic-SA · 2017-06-21T13:34:32Z

I've added a basic currency conversion of amounts to the main currency.
(For now, the main currency is the user's company currency.)
This will at least avoid having amounts in different currencies mixed together.
This does not support choosing currency rates at a particular date..

Martronic-SA · 2017-06-22T08:42:47Z

mis_builder/models/aep.py

                    # in initial mode, ignore accounts with 0 balance
                    continue
-                self._data[key][acc['account_id'][0]] = (debit, credit)
+                if company.currency_id != self.currency_id:
+                    rate = self.currency_id.rate / company.currency_id.rate


The basic implementation is quite simple.
Get the account for the line, get its company. If account company currency is not the same as APE currency, do the conversion (using current rate for now)

sbidoul · 2017-06-22T08:48:54Z

mis_builder/models/aep.py

@@ -281,14 +284,20 @@ def do_queries(self, date_from, date_to,
                                        ['debit', 'credit', 'account_id'],
                                        ['account_id'])
            for acc in accs:
+                company = account_model.browse(acc['account_id'][0]).company_id


This browse inside the loop is bad for performance.

I suggest:

fetching and computing currency rates by company at the beginning of the method, outside of the loops, store that in a dictionary

adding company_id in the read_group

Yes good idea!

What do you think about the precision rounding in float_is_zero method ? should it be company currency rounding or AEP currency rounding?

Use self.dp, which should come from the target currency.

But if target currency is CHF which is 0.5 rounding and we're getting amounts in EUR which rounding is 0.1, won't we miss some amounts? That could be problematic especially if there are many amounts with 0.1 or 0.2 ?

Hm. Indeed this float_compare occurs before currency conversion, so you are probably right, yes.

sbidoul · 2017-06-22T09:29:51Z

mis_builder/models/aep.py

+    def __init__(self, companies, currency_id=None):
+        self.companies = companies
+        self.currency_id = currency_id and currency_id or \
+            self.companies.env.user.company_id.currency_id


here, raise UserError if currency_id is not set and companies don't have all the same currency

Ok, but we might also consider that user wants to see the amounts in his own currency by default. But then we would need the Monetary cell as if not set, the traget currency depends on user

In that case the user will specify the currency explicitly in mis_report_instance.
AEP is a low level API, and I want it to have as little implicit behaviour as possible.

Ok, that's a good reason.

Martronic-SA · 2017-06-23T07:14:52Z

TODO:
Testing
test the tests, I've added an test for multi company. I've not been able to setup a testing environment and I've not been able to test those test files.
There is missing a proper test for currency conversion (set a currency rate and check the final results).
(But I've tested on a dev DB and it's working fine.)

Migration

Write the migration script to convert from company to company_ids in mis.report.instance

There is missing some functionalities to have this 100% usable, at least:

Add currency field in the mis.report.instance (separate PR)
Display a number as a monetary field and format it according to the target currency. Might be done either with a new type of cell "Monetary" or a checkbox in the style "Monetary formatting".
Add a selection field with options to tell the AEP how to choose the right date for currency rate conversion. That could be now, start/end of the report, start/end of each period or an average on the report/period, or..?

Martronic-SA · 2017-06-23T10:02:42Z

I think we are ready to add the currency_id field to mis report instance?
It's not a big change, I've done it in a sapearate branch here and it's working fine:
10.0-mis_builder-3-currency

sbidoul · 2017-06-23T10:06:59Z

Yes, now that all the multi-currency logic in AEP is in this branch, it's a small change to add curency_id in mis_report_instance in this PR.

sbidoul · 2017-06-23T10:08:35Z

mis_builder/migrations/8.0.2.0.0/pre-migration.py

+             SELECT id as mis_report_instance_id,
+                    company_id as res_company_id
+             FROM mis_report_instance;
+        """)


Hm that's a lot of code.

Better use openupgradelib for this: https://github.com/OCA/openupgradelib/blob/master/openupgradelib/openupgrade.py#L789

Also bump version to 10.0.3.1.0 and make a different migration directory for that.

So much simpler, thank you for remind me the openupgrade

Martronic-SA · 2017-06-28T07:06:25Z

I think we should add company_ids to mis.report.instance.period and if not null, add the company filter to
_get_additional_move_line_filter and _get_additional_query_filter methods.
This is useful if you want to make a comparision between companies in a table with a company per column.

We might better add company_ids filter field to sub kpi filter

sbidoul · 2017-06-30T10:06:15Z

Thanks a lot for your work! I've too little time to finalize the review and merge now, but I keep an eye on this and will do as soon as I can.

Martronic-SA · 2017-07-26T09:29:38Z

Please check also this PR
It's the Monetary type of value which takes the currency set in report instance or user's currency to format the values automatically.

sbidoul · 2017-09-03T12:41:36Z

@Martronic-SA Thanks for your contributions and patience.

I'm now back working on this and I'm planning to do a release with as many new features as possible before end of september.

I'll now flesh out the tests a little bit and merge at least some parts of this multi company PR.

Could you rebase it on OCA#275 ?

Martronic-SA · 2017-09-22T09:18:35Z

@sbidoul I'm back from holidays.
Unfortunately I'm having trouble with rebasing. I'm not very used to git in fact.
Any advice?
Thank you

sbidoul · 2017-09-24T11:15:00Z

Here is a rough rebasing procedure:

make sure you have a git remote named acsone: git remote add acsone https://github.com/acsone/account-financial-reporting.git
git fetch --all
git rebase acsone/10.0-mis_builder-3-sbi
in case of conflict, edit the conflicting files to resolve the conflict (look for === in the files), then git add the corrected files, the git rebase --continue
the git push -f

sbidoul · 2017-09-24T11:44:08Z

@Martronic-SA will you be at the OCA code sprint in Louvain-la-Neuve?

Small flake8 corrections

Added multi company test (still missing multi currency test)

Fix Tests complain about absolute import

Added specific method for rates computation

…AEP)

Martronic-SA · 2017-09-27T06:52:41Z

Thank you very much for the procedure.
I think it's ok now with the rebase.
Unfortunately I can't be there in Louvain-La-Neuve next week but I will be there remotely if possible.

sbidoul · 2017-09-27T08:38:01Z

@Martronic-SA could you review OCA#275 ?

We'll merge that first then move on with your other PRs.

Martronic-SA · 2017-09-27T08:52:01Z

I think everything is fine with OCA#275. ok for me!

sbidoul · 2017-09-27T08:54:10Z

Can write your approval there?

sbidoul · 2017-10-02T09:19:17Z

I'm currently moving this PR to OCA/mis-builder.

sbidoul · 2017-10-02T12:04:59Z

Moved to OCA/mis-builder#23

sbidoul requested changes Jun 19, 2017

View reviewed changes

sbidoul reviewed Jun 19, 2017

View reviewed changes

Martronic-SA commented Jun 22, 2017

View reviewed changes

sbidoul reviewed Jun 22, 2017

View reviewed changes

sbidoul reviewed Jun 23, 2017

View reviewed changes

Martronic-SA mentioned this pull request Jun 30, 2017

Added currency rate calculation on date #22

Open

sbidoul force-pushed the 10.0-mis_builder-3-sbi branch 2 times, most recently from 2803bb3 to 14b7f5e Compare September 1, 2017 15:51

sbidoul force-pushed the 10.0-mis_builder-3-sbi branch 2 times, most recently from 0ea81c3 to 6fa52b5 Compare September 23, 2017 22:01

This was referenced Sep 24, 2017

[9.0][10.0] mis_builder: multi-company consolidation OCA/account-financial-reporting#295

Closed

[mis_builder] multi currency support OCA/account-financial-reporting#350

Closed

sbidoul force-pushed the 10.0-mis_builder-3-sbi branch from b77bcb5 to d38052e Compare September 26, 2017 08:30

test using pypi for travis build dependencies

b274991

root added 19 commits September 27, 2017 08:26

compute_fiscalyear_dates is already api.multi so don't do anything here.

b4d4803

Get default currency from user's company currency

10bf472

Added basic currency conversion support.

23a11a7

Small flake8 corrections

flake8 fix

3296c98

flake8

b759cf0

Update pdf and xmlreports for multi company

a30c94d

Added multi company test (still missing multi currency test)

Syntax fix

59f8c4f

Fix Tests complain about absolute import

Removed default company from user in AEP and UserError

77de0ca

Added specific method for rates computation

Code optimisation

a2c6323

Code Optimisation

44b258e

Remove internal test

82df72d

Added multi company test file

0ba76f4

Fixed some syntax

cc174fb

Fix broken AEP + Migration script

8469cc5

Update migration script to 10.0.3.1.0

40938ea

Added currency_id field to mis report instance (and pass currency to …

848bf59

…AEP)

flake8 syntax

aa855c7

flake8 complaints

15c7547

Fix multiple comanies un getting dates in kpi period

cdf6cfc

Martronic-SA force-pushed the 10.0-mis_builder-3-sbi branch from ad33d5d to cdf6cfc Compare September 27, 2017 06:41

This was referenced Sep 30, 2017

multi currency support OCA/mis-builder#3

Closed

multi-company consolidation OCA/mis-builder#7

Closed

sbidoul mentioned this pull request Oct 2, 2017

Multi company support OCA/mis-builder#23

Merged

sbidoul closed this Oct 2, 2017

Added Multi company support #16

Added Multi company support #16

Conversation

Martronic-SA commented Jun 19, 2017

sbidoul 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martronic-SA commented Jun 21, 2017 • edited Loading

sbidoul commented Jun 21, 2017

sbidoul commented Jun 21, 2017

Martronic-SA commented Jun 21, 2017

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

sbidoul Jun 22, 2017 • edited Loading

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

Martronic-SA commented Jun 23, 2017 • edited Loading

Martronic-SA commented Jun 23, 2017

sbidoul commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martronic-SA commented Jun 28, 2017 • edited Loading

sbidoul commented Jun 30, 2017

Martronic-SA commented Jul 26, 2017

sbidoul commented Sep 3, 2017

Martronic-SA commented Sep 22, 2017

sbidoul commented Sep 24, 2017

sbidoul commented Sep 24, 2017

Martronic-SA commented Sep 27, 2017

sbidoul commented Sep 27, 2017

Martronic-SA commented Sep 27, 2017

sbidoul commented Sep 27, 2017

sbidoul commented Oct 2, 2017

sbidoul commented Oct 2, 2017

Martronic-SA commented Jun 21, 2017 •

edited

Loading

sbidoul Jun 22, 2017 •

edited

Loading

Martronic-SA commented Jun 23, 2017 •

edited

Loading

Martronic-SA commented Jun 28, 2017 •

edited

Loading