[11.0] migration #67

sbidoul · 2018-01-05T16:52:12Z

TODO:

This PR requires [FIX] web: issue when id field is present in x2m form odoo/odoo#21944. It's merged in saas-16, wait until it's forward ported to 11.0.
investigate some changes in mis_builder_template.xml wrt v10
drilldown is broken because the arguments is sent as string to the backend, probably due to a change in the js framework, the conversion to json must take place here
investigate changes in the definition of fields seq1, seq2 in mis_kpi_data.py: these should remain related as in v10, or is there an ORM bug?

sbidoul · 2018-01-06T14:13:00Z

Fixed the drilldown test and rebased.

I updated the review TODO list in the PR description.

sbidoul · 2018-01-08T08:49:05Z

mis_builder/models/mis_report_instance.py

+    @api.multi
+    def get_mis_report_view_from_id(self):
+        for rec in self:
+            return rec.get_mis_report_view_html()


This return inside the loop is not correct.

sbidoul · 2018-01-08T08:50:21Z

mis_builder/models/mis_report_instance.py

+        to render the actual mis report instance in the html format using the
+        widget"""
+        for rec in self:
+            rec.mis_report_view = ""


I'm not comfortable with this dummy field. Surely there must be a better way. Why is it computed btw?

You need to use a dummy field in order to be able to render the form view into this field.

I tested the various methods to render the dynamic view. The method used in v10, using the declaration of , https://github.com/OCA/mis-builder/blob/10.0/mis_builder/views/mis_report_instance.xml#L17, does not work in v11 anymore.
I tried using client actions as used in https://github.com/odoo/odoo/blob/11.0/addons/stock/views/stock_picking_views.xml#L235, but this would not let me add the view to the dashboard.

Perhaps there's another way, but I could not find it.

Perhaps we could bind the widget to the id field? So we could use the same widget on any m2o field that points to a mis.report.instance?

The problem is that with this approach we need to define . And it is "mis_report_view" the field that renders the full representation. I would not know how to bind the widget to the id field.

But where is that mis_report_view field computed with a real value? I can't find it in the code.

The content is defined in the _render method in the JS side https://github.com/OCA/mis-builder/blob/11.0-dev/mis_builder/static/src/js/mis_builder_widget.js#L44

So, the field has a widget, and the widget renders the content of this field. It really does happen to need any content from the field itself.

Yes but that does not use the mis_report_view field at all. So the widget could be bound to any field and the result would be identical?

Do you suggest ?

fd26f81 is a first simplification.

sbidoul · 2018-01-08T08:51:47Z

mis_builder/static/src/js/mis_builder_widget.js

+                method: 'drilldown',
+                args: [context.active_id, drilldown],
+                context: context,
+            }).then(function(result){


I my understanding of the js framework, there are better, higher level, ways to invoke backend methods than _rpc. These higher level methods were used in v10. This is probably the cause of drilldown not working.

This seems to be the standard method to call to backend methods in v11.

Ah ok then. So we need to convert the drilldown arg to json here.

sbidoul · 2018-01-08T10:00:30Z

mis_builder/views/mis_report_template.xml

+
+                                    <td t-foreach="row['cells']" t-as="cell"
+                                        t-att="{'style': cell['style'], 'title': cell['val_c']}" class="mis_builder_amount">
+                                        <t t-if="row['cells']">


why this t-if row['cells'] ?

I think it can be removed.

sbidoul · 2018-01-08T10:01:04Z

mis_builder/models/kpimatrix.py

+                        'val_c': '',
+                        'style': None,
+                    }
+                    row_data['cells'].append(col_data)


Is this change necessary? If yes why?

Yes. This fix prevented errors raising when you attempted to create lines.

sbidoul · 2018-01-08T10:04:04Z

@jbeficent the issues with the complex domains in the view, as well as the MisKpiData tests are resolved.

I still need fully dive into the js part. I've some doubts I wrote in the review comments. To be further investigated.

sbidoul · 2018-01-08T13:22:24Z

mis_builder/static/src/js/mis_builder_widget.js

+    _render: function () {
+        var self = this;
+        var context = self.getParent().state.context;
+        var rec_id = this.recordData.id || self.getParent().state.context.active_id;


Here we must use the field value. Not sure active_id makes sense (TBC).

The problem I had is when you add to the dashboard. It is in those cases where I needed. Because in the dashboard the record is not loaded. This has changed in the definition of the dashboard from v10, as far as I can tell.

JordiBForgeFlow · 2018-01-10T23:02:00Z

I will look at this proposal from @lasley to define classes during testing OCA/maintainer-tools#242. Perhaps it is better than what we used, or vice-versa?

lasley · 2018-01-10T23:39:30Z

It was adapted from an Acsone strategy (from Alfodoo), which originally looked very similar to the one you have here. It received a bunch of fixes from @obulkin though as the abstract models began to be implemented in descendent modules. Off the top of my head, I remember there were also some database mutations being missed that caused major headaches.

sbidoul · 2018-01-11T07:37:05Z

@jbeficent @lasley I believe the solution I reached in this PR for the test model is simple and clean and leaves the database clean. It works because auto_init does not commit in v11 (whereas it did in 10).

JordiBForgeFlow · 2018-01-11T11:21:30Z

@lasley @obulkin I also implemented @sbidoul approach here: https://github.com/OCA/server-tools/blob/11.0/base_exception/tests/common.py with success. Perhaps you can review if this proposal works better also for you and we can propose an improved template in maintainer-tools.

sbidoul · 2018-01-11T11:24:48Z

@jbeficent in my tests the teardown part is useful too to avoid warnings. Note this approach is very close to what Odoo does when creating/removing custom models through the UI: that's where I found inspiration in the end.

JordiBForgeFlow · 2018-01-11T11:26:06Z

@sbidoul I had to remove the reardown in the other module in https://github.com/OCA/server-tools/blob/11.0/base_exception/tests/common.py, though, because it would not allow me to uninstall the module. Gave me a nasty error indicating that the model was not found in the registry.

obulkin · 2018-01-11T20:19:07Z

At a glance, it does seem like all the problematic commits have been removed from v11, so there's probably no need to put the registry into test mode in this version. However, you may still want to do something along the lines of the following:

cls.registry[cls.test_model._name]._abstract = True
cls.registry[cls.test_model._name]._auto = False

This should prevent warnings about the model not having a table, while avoiding any errors caused by descendent code that expects it to be in the registry. At the very least, that was my experience in v10, where my first solution was also to just delete the model from the registry.

Following odoo/odoo#21944.

- we use the widget qweb template mechanism as in v10 - so the widget template does not change (except a tiny improvement) - rename widget and widget template files (mis_report_widget.js, .xml) - call the asynchronous method where they belong ie in the willStart method of the widget

sbidoul · 2018-01-13T15:37:11Z

Rebased. @jbeficent with fb3654e I'm now satisfied with the widget.

I rebased a last time, and removed the WIP tag.

I think we are good now!

JordiBForgeFlow · 2018-01-13T16:06:59Z

@sbidoul Good! Thanks for your help! I will do a last review on monday.

simahawk

Code LGTM. Just some questions...

simahawk · 2018-01-19T08:40:40Z

mis_builder/__manifest__.py

@@ -1,10 +1,9 @@
-# -*- coding: utf-8 -*-
 # Copyright 2014-2017 ACSONE SA/NV (<http://acsone.eu>)


you might want to raise year up to 2018 ;)

simahawk · 2018-01-19T08:42:24Z

mis_builder/__manifest__.py

 # Copyright 2014-2017 ACSONE SA/NV (<http://acsone.eu>)
 # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).

 {
    'name': 'MIS Builder',
-    'version': '10.0.3.1.1',
+    'version': '11.0.3.1.1',


by convention should be "11.0.0.0" but I understand if you want to keep internal versions in line :)

Yes, I do the effort of maintaining feature parity across 9 10 11 so this version number expresses that.

simahawk · 2018-01-19T08:43:59Z

mis_builder/datas/ir_cron.xml

-            <field eval="'_vacuum_report'" name="function"/>
-            <field eval="'(24,)'" name="args"/>
+            <field ref="model_mis_report_instance" name="model_id"/>
+            <field name="code">model._vacuum_report(24, 0)</field>


hours=24 (default, BTW). The second number is? I don't see it here

mis-builder/mis_builder/models/mis_report_instance.py

Line 466 in fb3654e

def _vacuum_report(self, hours=24):

@simahawk fixed

simahawk · 2018-01-19T08:47:37Z

mis_builder/report/mis_report_instance_qweb.xml

            <t t-set="matrix" t-value="o._compute_matrix()"/>
            <t t-set="style_obj" t-value="o.env['mis.report.style']"/>
            <div class="page">
+              <link href="/mis_builder/static/src/css/report.css" rel="stylesheet"/>


any good reason to keep the css here instead of assets?

good question. @jbeficent do you know why this change was made?

@simahawk is it better now?

@sbidoul should I see any difference? 🤔

@simahawk yes (0237f59). Apparently github has difficulties showing the diff inside the conversation. But the full diff is correct.

ahh now I see it. LGTM 😄

astirpe · 2018-01-24T09:33:31Z

I just noticed this line:
https://github.com/OCA/mis-builder/blob/11.0-dev/mis_builder/views/mis_report_instance.xml#L93
and I would ask you to replace the old colors= syntax with the new decoration-*= way to define the colors of rows.

Replaced by decoration-*. To be backported to 10 and 9.

sbidoul · 2018-01-24T22:46:27Z

@astirpe color="red:... replaced with decoration-danger. Thanks for pointing this out.

JordiBForgeFlow · 2018-01-25T13:42:58Z

I am good with the proposed changes!

astirpe · 2018-02-05T09:26:15Z

Thank you for this!
I think the default branch can be set to 11.0 ;)

JordiBForgeFlow · 2018-02-05T09:34:01Z

Thank you @sbidoul !!

sbidoul added 11.0 work in progress labels Jan 5, 2018

This was referenced Jan 5, 2018

restore report and instance views as in v10 ForgeFlow/mis-builder#3

Closed

11.0 mig mis builder budget #66

Closed

sbidoul changed the title ~~[11.0] 11.0 migration~~ [11.0] migration Jan 5, 2018

sbidoul mentioned this pull request Jan 5, 2018

Migration to version 11.0 #12

Closed

7 tasks

sbidoul changed the base branch from 10.0 to 11.0 January 6, 2018 13:14

sbidoul closed this Jan 6, 2018

sbidoul force-pushed the 11.0-dev branch from 9e7998e to edc61e9 Compare January 6, 2018 13:47

sbidoul reopened this Jan 6, 2018

sbidoul force-pushed the 11.0-dev branch 3 times, most recently from cff8425 to 99fa7b9 Compare January 7, 2018 16:12

sbidoul commented Jan 8, 2018

View reviewed changes

JordiBForgeFlow mentioned this pull request Jan 8, 2018

[11.0][base_exception] refactor tests OCA/server-tools#1111

Merged

sbidoul commented Jan 8, 2018

View reviewed changes

JordiBForgeFlow mentioned this pull request Jan 11, 2018

[11.0][mig] l10n_es_mis_reports OCA/l10n-spain#692

Closed

sbidoul added needs review and removed work in progress labels Jan 13, 2018

JordiBForgeFlow added 2 commits January 13, 2018 16:29

[MIG][11.0] mis_builder

42beb86

[MIG] fixes

3243d67

sbidoul and others added 15 commits January 13, 2018 16:29

[MIG] remove remnant of previous experiment

a8ea6a0

[MIG][FIX] setup.py

d512263

[MIG] bump version in mis_builder_budget and mis_builder_demo

13b1f98

[MIG] restore report and instance views as in v10

4d263fd

Following odoo/odoo#21944.

[MIG] mis_builder_budget: Migrate to Odoo 11

0450d5f

[MIG] python headers (copyright and encoding statement)

6d3e87d

[MIG] revert super() changes to minimize diff with 10.0

67db48d

[MIG] restore drilldown test as it was in 10

a5e7cbc

[MIG][11.0] mis_builder_demo

30c734c

[MIG] restore KpiData as it was in 10

b6db016

[MIG] a clean way to declare test models

3ed8d76

[MIG] remove unnecessary dummy field

752bdaf

[MIG] fix drilldown issue

867c6a0

[MIG] fix template view

525c9f4

sbidoul force-pushed the 11.0-dev branch from 5516236 to fb3654e Compare January 13, 2018 15:33

simahawk approved these changes Jan 19, 2018

View reviewed changes

[MIG] fix cron declaration

43f3ef2

sbidoul added 2 commits January 24, 2018 23:25

[IMP] color attribute deprecated in views

f029d78

Replaced by decoration-*. To be backported to 10 and 9.

[MIG] better way to declare report assets

0237f59

JordiBForgeFlow approved these changes Jan 25, 2018

View reviewed changes

sbidoul merged commit 12b58be into 11.0 Feb 4, 2018

sbidoul deleted the 11.0-dev branch February 4, 2018 15:20

		@@ -1,10 +1,9 @@
		# -- coding: utf-8 --
		# Copyright 2014-2017 ACSONE SA/NV (<http://acsone.eu>)

[11.0] migration #67

[11.0] migration #67

Conversation

sbidoul commented Jan 5, 2018 • edited Loading

sbidoul commented Jan 6, 2018

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

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 Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordiBForgeFlow commented Jan 10, 2018

lasley commented Jan 10, 2018

sbidoul commented Jan 11, 2018

JordiBForgeFlow commented Jan 11, 2018

sbidoul commented Jan 11, 2018

JordiBForgeFlow commented Jan 11, 2018

obulkin commented Jan 11, 2018

sbidoul commented Jan 13, 2018

JordiBForgeFlow commented Jan 13, 2018

simahawk 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

astirpe commented Jan 24, 2018

sbidoul commented Jan 24, 2018

JordiBForgeFlow commented Jan 25, 2018

astirpe commented Feb 5, 2018

JordiBForgeFlow commented Feb 5, 2018

sbidoul commented Jan 5, 2018 •

edited

Loading