[9.0] MIG bi_sql_editor by nicomacr · Pull Request #148 · OCA/reporting-engine

nicomacr · 2017-07-11T17:08:17Z

No description provided.

…ield; [ADD] security

AaronHForgeFlow · 2017-07-14T07:20:25Z

I ❤️ this module. Would you include the changes proposed here #149 ?

JordiBForgeFlow · 2017-07-14T07:22:07Z

@aheficent perhaps you can propose a PR that includes it

legalsylvain · 2017-07-14T14:15:09Z

please wait, i'm doing some improvement on #149.

legalsylvain · 2017-07-14T14:23:28Z

improvment available here : #150

legalsylvain · 2017-07-14T14:33:37Z

Hi @aheficent. Thanks a lot for porting this module. LGTM for me (code / test). please ping me once you integrated #150 commit.
regards.

AaronHForgeFlow · 2017-07-14T16:29:56Z

Sure @legalsylvain Thank you for the improvement

JordiBForgeFlow · 2017-07-16T10:29:35Z

@nicomacr @legalsylvain @aheficent I have improved 9.0 with the cherry pick of #150 here: adhoc-dev#1

[IMP] fr translation [IMP] add last refresh date time on the action name of each sql materialized view

nicomacr · 2017-07-17T14:12:53Z

@jbeficent thanks for the PR

AaronHForgeFlow · 2017-07-17T15:23:20Z

Hi @nicomacr I'm proposing some tests here: https://github.com/adhoc-dev/reporting-engine-1/pull/2 . Also it fixes the unlink method, which is not calling super.

legalsylvain

LGTM to me. one comment on demo data to update.

Otherwise, 👍
Thanks !

PS : could you review, the V8.0 PR ?
#150

legalsylvain · 2017-07-17T15:22:27Z

+SELECT
+    name as x_name,
+    case
+        when author ilike '%OpenERP SA%' THEN 'Odoo SA'


because, odoo renamed authorship in V9.

case when author ilike '%OpenERP SA%' THEN 'Odoo SA' when author ilike '%Odoo SA%' THEN 'Odoo SA' when author ilike '%Odoo Community Association (OCA)%' THEN 'OCA' else 'Undefined Author' END as x_author_type

AaronHForgeFlow · 2017-07-18T11:15:35Z

I closed the previous PR for the test. I created a new one here: https://github.com/adhoc-dev/reporting-engine-1/pull/3 but it's failing because it's creating a lot of connections to the database. I'll update the PR when this issue is solved

legalsylvain · 2017-07-21T07:11:47Z

Any other review to validate the PR ? thanks !

JordiBForgeFlow · 2017-07-21T07:18:35Z

Can we add an option for the person thay creates the query to decide the order of the.views? (Treen graph, pivot)?

AaronHForgeFlow · 2017-07-21T07:18:47Z

@legalsylvain This works ok to me. However there's must be something wrong with the test I did because when I run it on local it's trying to connect to the database multiple times. Getting this:
PoolError: The Connection Pool Is Full

Honestly, don't know the reason

elicoidal

nice one

elicoidal · 2017-07-21T07:26:13Z

+  take a while, if view is materialized)
+
+* If it's a MATERIALIZED view: 
+    * a cron task is created to refresh


add a blank line

elicoidal · 2017-07-21T07:26:43Z

+
+To use this module, you need to:
+
+* Go to 'Reporting' / 'Custom Reports' 


Best would be enumerated list (nice to have)

elicoidal · 2017-07-21T07:28:01Z

+        'views/menu.xml',
+    ],
+    'demo': [
+        'demo/res_groups.xml',


demo/res_groups_demo.xml (several)

elicoidal · 2017-07-21T07:28:51Z

+License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+-->
+
+<openerp><data>


odoo
+Improve the format

elicoidal · 2017-07-21T07:31:20Z

+
+    technical_name = fields.Char(
+        string='Technical Name', required=True,
+        help="Suffix of the SQL view. (SQL full name will be computed and"


elicoidal · 2017-07-21T07:32:12Z

+    technical_name = fields.Char(
+        string='Technical Name', required=True,
+        help="Suffix of the SQL view. (SQL full name will be computed and"
+        " prefixed by 'x_bi_sql_view_'. Should have correct"


Syntax should follow: https://www.postgresql.org/"
"docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS

elicoidal · 2017-07-21T07:33:56Z

+
+    ttype = fields.Selection(
+        string='Field Type', selection=_TTYPE_SELECTION, help="Type of the"
+        " Odoo field that will be created. Let empty if you don't want to"


elicoidal · 2017-07-21T07:34:19Z

+    many2one_model_id = fields.Many2one(
+        comodel_name='ir.model', string='Model',
+        help="For 'Many2one' Odoo field.\n"
+        " Co Model of the field.")


legalsylvain · 2017-07-21T07:58:10Z

Can we add an option for the person that creates the query to decide the order of the.views? (Treen graph, pivot)?

We could

add an extra char field in the view, with a default value to 'graph,pivot,tree', (readonly if sql is not in the draft / analyzed state)
add a constraint that check that value.split(',') contains only ['graph', 'pivot', 'tree']
use this value to generate the action. (and use it in the button_open_ui)

No time for this feature, for my side, but ok to review it. (It could be done in the second PR.)

AaronHForgeFlow · 2017-07-21T08:12:42Z

@legalsylvain I'll propose a PR for that

AaronHForgeFlow · 2017-07-21T09:04:26Z

Hi @nicomacr @legalsylvain I added an option for the person that creates the query to decide the first view to show: https://github.com/adhoc-dev/reporting-engine-1/pull/4

[IMP]bi_sql_editor. Select View Order.

legalsylvain

Hi thanks a lot for adding a hook.
Some comments inline however.

thanks.

legalsylvain · 2017-07-27T13:16:27Z

-            sql_view.action_id.unlink()
-            sql_view.menu_id.unlink()
-            sql_view.rule_id.unlink()
+            if sql_view.tree_view_id:


why did you added a test ? if rule_id is False, sql_view.r_le_id.unlink() make nothing.

legalsylvain · 2017-07-27T13:18:47Z

+        rec.button_set_draft()
+        rec.unlink()
+
+    # delete dirty data that could cause problems


I don't understand all the lines below. 15+
for me button_set_draft() and unlink() are enought, to drop all the (materialized or not) views.

I'll check that again without tests. But with them that was not enough, because it creates a lot of dirty data in the database. Thank you @legalsylvain

legalsylvain · 2017-07-27T13:19:09Z

+
+    # Drop table if uninstalling went wrong
+    cr.execute("""
+        delete from ir_model_fields where model like 'bi.sql.view%';


better to do sql_view.model_id.unlink(), don't you think ?

I think the same. This is supposed to be done by this line rec.button_set_draft() But for some reason it does not work properly. At the end it shows this error:

Unable to delete 121@ir.model
and then:
UserError: (u"The field 'bi.sql.view.field.bi_sql_view_id' cannot be removed because the field 'bi.sql.view.bi_sql_view_field_ids' depends on it.", None)

That's why I put all those queries.

legalsylvain · 2017-07-27T13:20:25Z

    def _drop_model_and_fields(self):
        for sql_view in self:
-            sql_view.model_id.unlink()
+            if sql_view.model_id:


AaronHForgeFlow · 2017-07-27T13:56:59Z

BTW, travis compilation was not failing because of the test here but for a test in another module: test_report_qweb_pdf_watermark

MiquelRForgeFlow · 2017-07-28T08:20:18Z

No, Travis now is not failing by the pdf_watermark. It seems it is failing because it cannot install some external libraries. They should be added in Travis or something like this may be.

AaronHForgeFlow · 2017-07-28T08:25:38Z

@mreficent All right. I'll try to fix that

MiquelRForgeFlow · 2017-07-28T08:33:44Z

@aheficent sorry, yes, the problem is with report_qweb_pdf_watermark

AaronHForgeFlow · 2017-07-28T08:48:39Z

BTW among other things, Lint check fails because of the queries in the uninstall hook. But those queries are necessary IMO so I'd keep that

AaronHForgeFlow · 2017-07-28T09:22:30Z

Hi @legalsylvain I created another Pull Request here to attend your comments. Despite my previous comment I realized that those queries in the uninstall hook are not needed. However this error is still shown when uninstalling:

Unable to delete 121@ir.model
and then:
UserError: (u"The field 'bi.sql.view.field.bi_sql_view_id' cannot be removed because the field 'bi.sql.view.bi_sql_view_field_ids' depends on it.", None)

Nevertheless, no bi.sql.view and no bi.sql.view.field remains in the database when uninstalling. That's why I removed the queries from the uninstall hook.

AaronHForgeFlow · 2017-07-28T15:11:03Z

Created Pull Request here to fix flake8 error https://github.com/adhoc-dev/reporting-engine-1/pull/8

However this is still pending:
https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#no-sql-injection

Does anybody knows how to configure travis file to avoid this check? 🙏

legalsylvain · 2017-07-31T09:11:51Z

@aheficent :

self.env.cr.commit() # pylint: disable=invalid-commit

Sample : https://github.com/OCA/server-tools/blob/10.0/database_cleanup/models/purge_columns.py#L51

regards;

AaronHForgeFlow · 2017-07-31T09:15:44Z

Thank you @legalsylvain! I added this to my last Pull Request. (https://github.com/adhoc-dev/reporting-engine-1/pull/8)

legalsylvain · 2017-07-31T13:09:27Z

@jbeficent , @aheficent could you set a little +1 on the PR against V8 you cherry picked ?
#150.

thanks !

AaronHForgeFlow · 2017-08-01T15:40:08Z

Runbot is compiled. It's showing red because of this:
ParseError: "no such savepoint " while parsing /home/odoo/build/OCA/reporting-engine/bi_sql_editor/demo/bi_sql_view_demo.xml:52, near <function model="bi.sql.view" name="button_validate_sql_expression" eval="([ref('module_sql_view')])"/>

Should we create a savepoint before calling the method button_validate_sql_expression?

Edit: Run in local in debug mode. Same error as here travis is getting. In the second try it fails for different reason, the relations created for the demo data in the previous try were still there. Same problem as the one I was having with the tests.

AaronHForgeFlow · 2017-08-10T15:41:11Z

Hi @nicomacr I created a new PR here: https://github.com/adhoc-dev/reporting-engine-1/pull/9

This fixes the error related with the save point that was introduced in the last commit. Besides I added some tests back. However, the tests don't cover the part of creating the models because that would leave unwanted data in the database.
@legalsylvain @jbeficent

[UPT]add test

AaronHForgeFlow · 2017-08-10T16:21:01Z

@nicomacr Sorry for the Lint errors. 🤕 Created new PR for that:https://github.com/adhoc-dev/reporting-engine-1/pull/10 Probably a commit squashing will be needed later

AaronHForgeFlow · 2017-08-10T17:14:02Z

Created PR to prevent runbot warning https://github.com/adhoc-dev/reporting-engine-1/pull/11

Travis will be fixed when this is merged: #154

legalsylvain · 2017-08-11T11:22:23Z

            'relation': self.ttype == 'many2one' and
            self.many2one_model_id.model or False,
-            'tree_visibility': self.field_description and
-            'available' or 'unavailable',


JordiBForgeFlow

Technical and functional review 👍

JordiBForgeFlow · 2017-08-11T11:35:11Z

Travis is KO due to problem found in #154. But tested locally, and all test are passing correctly.

The only problem we found is that it is not possible to include in the tests the parts where the models are created, because every time that you pass the test the tables created by the test are not deleted from the DB -- very strange --, so the next time you run the test, it fails because the table already exists.

legalsylvain · 2017-08-11T11:38:27Z

Well, I propose to merge this PR, because it's OK. (and it has 6 👍, that's a record ! )
let's go improving test in another PR, or for another version. (8.0 version didn't include test at all, so, it's better).

Signed-off-by pedrobaeza

legalsylvain and others added 8 commits July 11, 2017 09:52

new module bi_sql_editor [IMP] function to guess model for many2one f…

73e19e5

…ield; [ADD] security

typo

ad668e7

OCA Transbot updated translations from Transifex

c96cda8

OCA Transbot updated translations from Transifex

a79c475

OCA Transbot updated translations from Transifex

5e3318f

[MIG][9.0]bi_sql_editor

56c7cd6

ADD oca_dependencies

097f097

DEL old files

155d4b7

[ADD] create ui now create tree view

669168d

[IMP] fr translation [IMP] add last refresh date time on the action name of each sql materialized view

legalsylvain approved these changes Jul 17, 2017

View reviewed changes

[UPT]add test. FIx unlink

eb91e2d

elicoidal suggested changes Jul 21, 2017

View reviewed changes

AaronHForgeFlow and others added 2 commits July 21, 2017 13:59

[IMP]add an option to select the first view

b00e6f9

Merge pull request #4 from Eficent/9.0-mig-bi_sql_editor-imp

48978f4

[IMP]bi_sql_editor. Select View Order.

legalsylvain requested changes Jul 27, 2017

View reviewed changes

[FIX]lint check. unnecessary queries in uninstall hook.

c7df8d7

AaronHForgeFlow mentioned this pull request Jul 31, 2017

Uninstall one2many field odoo/odoo#14479

Closed

[FIX]pylint

a47ce80

ValentinoDefelice approved these changes Aug 9, 2017

View reviewed changes

[FIX]remove cr.commit, that removed the savepoint

3c358e6

[UPT]add test

[FIX]lint errors

34014b3

[FIX]ir_model does not have field tree visibility

0317163

legalsylvain reviewed Aug 11, 2017

View reviewed changes

legalsylvain approved these changes Aug 11, 2017

View reviewed changes

JordiBForgeFlow approved these changes Aug 11, 2017

View reviewed changes

JordiBForgeFlow merged commit f94fe24 into OCA:9.0 Aug 11, 2017

jjscarafia deleted the 9.0-mig-bi_sql_editor branch August 11, 2017 11:42

SimoneVagile pushed a commit to SimoneVagile/reporting-engine that referenced this pull request Jan 25, 2021

Merge PR OCA#148 into 12.0

9d93ead

Signed-off-by pedrobaeza


		To use this module, you need to:

		* Go to 'Reporting' / 'Custom Reports'

Uh oh!

Uh oh!

Conversation

nicomacr commented Jul 11, 2017

Uh oh!

AaronHForgeFlow commented Jul 14, 2017

Uh oh!

JordiBForgeFlow commented Jul 14, 2017

Uh oh!

legalsylvain commented Jul 14, 2017

Uh oh!

legalsylvain commented Jul 14, 2017

Uh oh!

legalsylvain commented Jul 14, 2017

Uh oh!

AaronHForgeFlow commented Jul 14, 2017

Uh oh!

JordiBForgeFlow commented Jul 16, 2017

Uh oh!

nicomacr commented Jul 17, 2017

Uh oh!

AaronHForgeFlow commented Jul 17, 2017

Uh oh!

legalsylvain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronHForgeFlow commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legalsylvain commented Jul 21, 2017

Uh oh!

JordiBForgeFlow commented Jul 21, 2017

Uh oh!

AaronHForgeFlow commented Jul 21, 2017

Uh oh!

elicoidal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legalsylvain commented Jul 21, 2017

Uh oh!

AaronHForgeFlow commented Jul 21, 2017

Uh oh!

AaronHForgeFlow commented Jul 21, 2017

Uh oh!

legalsylvain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronHForgeFlow Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronHForgeFlow commented Jul 27, 2017

AaronHForgeFlow commented Jul 18, 2017 •

edited

Loading

AaronHForgeFlow Jul 28, 2017 •

edited

Loading

MiquelRForgeFlow commented Jul 28, 2017 •

edited

Loading

AaronHForgeFlow commented Jul 31, 2017 •

edited

Loading

AaronHForgeFlow commented Aug 1, 2017 •

edited

Loading