Configurable account.account model (can use any model that has a code field) #149

ernestotejeda · 2019-01-09T06:28:02Z

The account_id field of the model selected in 'Move lines source'
in the Period form (mis.report.instance.period) can be a Many2one
relationship with any class that has the field code

Cc @Tecnativa
TT14942

pedrobaeza · 2019-01-09T09:43:07Z

@sbidoul this is needed for #148

sbidoul · 2019-01-09T09:48:43Z

Interesting. I'll look into it within a couple of weeks.

sbidoul · 2019-01-13T21:59:21Z

mis_builder/models/mis_report_instance.py

+
+            aep = self.report_id._prepare_aep(self.query_company_ids,
+                                              self.currency_id,
+                                              account_model=account_model)


Looking at this in more details, I think it does not make sense to have a different account_model for different columns, because of account details expansion which would become meaningless if different account objects are used in different columns.

IOW, this may work, but the account model must be configured globally at the level of mis.report, and passed to the AEP constructor, as well as to the KPI matrix constructor.

Or, better, define the AML model used for Actuals at the mis.report level, and make sure Actual (alternative) models used in columns have the same account model in the account_id m2o.

updated: mis.report instead of mis.report.instance

Another way to say it is that one way or another, the same AEP must be used for all columns and the account model must therefore be known before the for period loop.

sbidoul

This require more thinking.

pedrobaeza · 2019-01-14T08:49:23Z

@sbidoul are you saying that each time you want to create an instance, you have to define all of these things? I think all this information should be in the template, not the instance.

sbidoul · 2019-01-14T08:51:29Z

@pedrobaeza yes, correct, I meant the template.

pedrobaeza · 2019-01-14T08:54:31Z

Well, that can an option, but why complicate it when it can be auto-inferred? I don't get the advantage.

sbidoul · 2019-01-14T08:59:08Z

@pedrobaeza I don't understand your last comment.

pedrobaeza · 2019-01-14T09:02:54Z

As we have the model of account_id field, we already know it, so why re-define it on template level?

sbidoul · 2019-01-14T09:10:40Z

That's not what I suggest in #149 (comment)

What I suggest is

add a field named "Actuals move lines model" on mis.report (default = acccount.move.line)
from that, the account_id model can be found
in mis.report.instance.period configuration, enforce the account_id field used for the "Actuals (alternative)" models uses the same account_id model as the main one

pedrobaeza · 2019-01-14T10:57:48Z

But this is what I'm saying that is not really needed, as you already know the m2o model from the model definition. Why do you need to enforce that thing? It would make sense if you enforce all things, like using a template with an specific actual data, but only for the model...

Other thing that I don't understand is that the data source is specified at instance level instead of template. Maybe a template can work with several data sources, but this should be specified by a m2m, but not having to be configured each time for each report...

sbidoul · 2019-01-14T11:13:01Z

The data source is specified at the column level in the instance. There are no columns in the template. This allows having a column for actuals, another for committed, and a third for budget for example.

I'm just saying that we must enforce the same account model is used for the data sources used in all columns. This is necessary because when you expand accounts they are displayed in rows and the rows must make sense for all columns.

pedrobaeza · 2019-01-14T11:17:04Z

@ernestotejeda can you do the requested changes or is there something you don't understand?

ernestotejeda · 2019-01-14T16:58:38Z

Yes @pedrobaeza, I can try to do the requested changes according to #149 (comment). I agree with @sbidoul, thanks.

ernestotejeda · 2019-01-16T17:43:21Z

@sbidoul i have done the changes proposed

rafaelbn · 2019-08-16T14:48:42Z

Hi @ernestotejeda This branch has conflicts that must be resolved, could you rebase?

@sbidoul could you give a feedback of the last changes?

sbidoul

@ernestotejeda @rafaelbn This is going into the right direction. I made a few relatively minor comments.

Could you make this PR to 10.0? I'll then forward port quickly as soon as it is merged.
Also could you add a file named readme/newsfragments/<pr or issue number>.feature that provides a short description of the feature that will be included automatically in the changelog?

Thanks for this work.

sbidoul · 2019-08-16T15:29:07Z

mis_builder/models/aep.py

+        # Account model
+        self._account_model = account_model
+        if account_model is None:
+            self._account_model = self.env['account.account']


Suggested change

self._account_model = self.env['account.account']

self._account_model = self.env[account_model or 'account.account'].with_context(active_test=False)

sbidoul · 2019-08-16T15:29:47Z

mis_builder/models/aep.py

        for key, acc_domains in self._map_account_ids.items():
            all_account_ids = set()
            for acc_domain in acc_domains:
                acc_domain_with_company = expression.AND([
                    acc_domain,
                    [('company_id', 'in', self.companies.ids)],
                ])
-                account_ids = account_model.\
+                account_ids = self._account_model.\
+                    with_context(active_test=False).\


Suggested change

with_context(active_test=False).\

sbidoul · 2019-08-16T15:30:24Z

mis_builder/models/kpimatrix.py

-        self._account_model = env['account.account']
+        self._account_model = account_model
+        if account_model is None:
+            self._account_model = env['account.account']


Suggested change

self._account_model = env['account.account']

self._account_model = env[account_model or 'account.account']

sbidoul · 2019-08-16T15:32:05Z

mis_builder/models/mis_report.py

+        compute='_compute_account_model',
+        comodel_name='ir.model',
+        string='Account model',
+    )


make this field a Char

sbidoul · 2019-08-16T15:32:23Z

mis_builder/models/mis_report.py

@@ -520,15 +551,15 @@ def copy(self, default=None):
    @api.multi
    def prepare_kpi_matrix(self):
        self.ensure_one()
-        kpi_matrix = KpiMatrix(self.env)
+        kpi_matrix = KpiMatrix(self.env, self.env[self.account_model.model])


Suggested change

kpi_matrix = KpiMatrix(self.env, self.env[self.account_model.model])

kpi_matrix = KpiMatrix(self.env, self.account_model)

sbidoul · 2019-08-16T15:32:39Z

mis_builder/models/mis_report.py

        for kpi in self.kpi_ids:
            kpi_matrix.declare_kpi(kpi)
        return kpi_matrix

    @api.multi
    def _prepare_aep(self, companies, currency=None):
        self.ensure_one()
-        aep = AEP(companies, currency)
+        aep = AEP(companies, currency, self.env[self.account_model.model])


Suggested change

aep = AEP(companies, currency, self.env[self.account_model.model])

aep = AEP(companies, currency, self.account_model)

sbidoul · 2019-08-16T15:33:43Z

mis_builder/models/mis_report_instance.py

+                record_model = record.source_aml_model_id.field_id.filtered(
+                    lambda r: r.name == 'account_id').relation
+                period_model = record.report_id.account_model.model
+                if record_model != period_model:


s/period_model/report_account_model/
s/record_model/period_account_model/

sbidoul · 2019-08-16T15:35:05Z

mis_builder/models/mis_report_instance.py

@@ -703,7 +718,8 @@ def drilldown(self, arg):
        account_id = arg.get('account_id')
        if period_id and expr and AEP.has_account_var(expr):
            period = self.env['mis.report.instance.period'].browse(period_id)
-            aep = AEP(self.query_company_ids, self.currency_id)
+            aep = AEP(self.query_company_ids, self.currency_id,
+                      self.env[self.report_id.account_model.model])


Suggested change

self.env[self.report_id.account_model.model])

self.report_id.account_model)

sbidoul · 2019-08-16T15:36:04Z

mis_builder/tests/test_data_sources.py

        ))
+        self.report._compute_account_model()


It should not be necessary to change the test since move_lines_source has a default value.

sbidoul · 2019-08-16T15:37:15Z

mis_builder_demo/data/mis_report.xml

@@ -6,6 +6,8 @@
  <record id="mis_report_expenses" model="mis.report">
    <field name="name">Demo Expenses</field>
    <field name="style_id" ref="mis_report_expenses_style1"/>
+    <field name="move_lines_source" search="[('model', '=', 'account.move.line')]"/>
+    <field name="account_model" search="[('model', '=', 'account.account')]"/>


This should not be necessary since there is a default value.

rafaelbn · 2019-09-30T09:20:13Z

Hi @ernestotejeda please rebase, @pedrobaeza @sbidoul could we discuss this?

#148 depends on this

Thanks

The account_id field of the model selected in 'Move lines source' in the Period form (mis.report.instance.period) can be a Many2one relationship with any class that has the field code

ernestotejeda · 2019-10-10T19:43:00Z

@rafaelbn, @sbidoul , @pedrobaeza I have done the changes proposed by @sbidoul

sbidoul · 2019-10-11T17:40:34Z

Thanks, I'll review soon.

sbidoul · 2019-10-17T17:27:42Z

@ernestotejeda I ported your commit to 10.0 and slightly amended it to resolve the conflict that was due to #242. It is in #240 (ce73613). Would you like to have quick look at it?

I removed the changes to the test, because the change should be fully backward compatible.

I also changed the way you added contributors, to conserve the style of adding contributors chronologically, I hope you don't mind.

sbidoul · 2019-10-18T15:31:14Z

Closing as it is included in 10.0-staging and will be soon merged in 11.0. Shout if you see any issue.

pedrobaeza mentioned this pull request Jan 9, 2019

mis_builder_analytic: New module #148

Closed

1 task

sbidoul changed the title ~~11.0 imp account_id mis_builder~~ Configurable account.account model (can use any model that has a code field) Jan 13, 2019

sbidoul added enhancement 11.0 needs review labels Jan 13, 2019

sbidoul reviewed Jan 13, 2019

View reviewed changes

sbidoul requested changes Jan 13, 2019

View reviewed changes

ernestotejeda force-pushed the 11.0-imp-account_id-mis_builder branch 2 times, most recently from f1aa2ab to 4688386 Compare January 16, 2019 15:37

sbidoul mentioned this pull request May 20, 2019

KPI description from value #197

Closed

sbidoul reviewed Aug 16, 2019

View reviewed changes

ernestotejeda force-pushed the 11.0-imp-account_id-mis_builder branch from 4688386 to eb16300 Compare October 10, 2019 18:43

mis_builder: Make the account_id field flexible

344c4cc

The account_id field of the model selected in 'Move lines source' in the Period form (mis.report.instance.period) can be a Many2one relationship with any class that has the field code

ernestotejeda force-pushed the 11.0-imp-account_id-mis_builder branch from eb16300 to 344c4cc Compare October 10, 2019 19:21

sbidoul closed this Oct 18, 2019

pedrobaeza deleted the 11.0-imp-account_id-mis_builder branch October 27, 2019 10:25

	self._account_model = self.env['account.account']
	self._account_model = self.env[account_model or 'account.account'].with_context(active_test=False)

	self._account_model = env['account.account']
	self._account_model = env[account_model or 'account.account']

	kpi_matrix = KpiMatrix(self.env, self.env[self.account_model.model])
	kpi_matrix = KpiMatrix(self.env, self.account_model)

	aep = AEP(companies, currency, self.env[self.account_model.model])
	aep = AEP(companies, currency, self.account_model)

	self.env[self.report_id.account_model.model])
	self.report_id.account_model)

Configurable account.account model (can use any model that has a code field) #149

Configurable account.account model (can use any model that has a code field) #149

Conversation

ernestotejeda commented Jan 9, 2019 • edited Loading

pedrobaeza commented Jan 9, 2019

sbidoul commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul Jan 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbidoul left a comment

Choose a reason for hiding this comment

pedrobaeza commented Jan 14, 2019

sbidoul commented Jan 14, 2019

pedrobaeza commented Jan 14, 2019

sbidoul commented Jan 14, 2019

pedrobaeza commented Jan 14, 2019

sbidoul commented Jan 14, 2019

pedrobaeza commented Jan 14, 2019

sbidoul commented Jan 14, 2019

pedrobaeza commented Jan 14, 2019

ernestotejeda commented Jan 14, 2019

ernestotejeda commented Jan 16, 2019

rafaelbn commented Aug 16, 2019

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

sbidoul Aug 16, 2019 • 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

rafaelbn commented Sep 30, 2019

ernestotejeda commented Oct 10, 2019

sbidoul commented Oct 11, 2019

sbidoul commented Oct 17, 2019 • edited Loading

sbidoul commented Oct 18, 2019

ernestotejeda commented Jan 9, 2019 •

edited

Loading

sbidoul Jan 13, 2019 •

edited

Loading

sbidoul Aug 16, 2019 •

edited

Loading

sbidoul commented Oct 17, 2019 •

edited

Loading