Skip to content

[10.0][MIG] report_xml migration - #143

Merged
JordiBForgeFlow merged 17 commits into
OCA:10.0from
etobella:10.0
Jul 12, 2017
Merged

JordiBForgeFlow merged 17 commits into
OCA:10.0from
etobella:10.0

Conversation

@etobella

Copy link
Copy Markdown
Member

Migration of report_xml module.

@etobella

Copy link
Copy Markdown
Member Author

Module migrated to 10.0

@JordiBForgeFlow JordiBForgeFlow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review

Comment thread report_xml/models/report_action.py Outdated
@api.model
def render_report(self, res_ids, name, data):
"""Special handling for ``qweb-xml`` reports."""
if data.get("report_type") == u"qweb-xml":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sentence is error-prone. I would suggest to search for the report in the first, place, like in https://github.com/OCA/reporting-engine/blob/10.0/report_py3o/models/ir_actions_report_xml.py#L100
, based on the name, or self._cr.execute("SELECT * FROM ir_act_report_xml WHERE report_name=%s", (name,))

And once it has been found, and the type is 'qweb-xml', you can continue with the next steps.
You would even not need to use _loookup_report at all, in case that the type is 'qweb-xml'.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified

Comment thread report_xml/tests/test_report_xml.py Outdated
report_name = 'report_xml.demo_report_xml_view'
docs = self.env['res.company'].search([], limit=1)
rep = report_object.render_report(
docs.ids, report_name, {'report_type': u"qweb-xml"}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that the usual practice is to call render_report not specifying the report_type, as it is looked up inside the method.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified

@JordiBForgeFlow

Copy link
Copy Markdown
Member

@mreficent can you review?

@JordiBForgeFlow JordiBForgeFlow changed the title [MIG] report_xml migration [10.0][MIG] report_xml migration Jul 7, 2017

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

but just minor comments

Comment thread report_xml/README.rst
@@ -0,0 +1,102 @@
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
:alt: License: AGPL-3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add :target: http://www.gnu.org/licenses/agpl-3.0-standalone.html

Comment thread report_xml/README.rst Outdated
.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
:alt: License: AGPL-3

Qweb XML Reports

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put ================ on top of the title
the proper name of the module is Report XML

Comment thread report_xml/README.rst Outdated
Configuration
=============

No manual configuration is needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this configuration section

Comment thread report_xml/README.rst Outdated
Known issues / Roadmap
======================

None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove issues section as there are none

Comment thread report_xml/__manifest__.py Outdated
"name": "Qweb XML Reports",
"version": "10.0.1.0.0",
"category": "Reporting",
"website": "https://odoo-community.org",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/OCA/reporting-engine

Comment thread report_xml/demo/report.xml Outdated
report_type="qweb-xml"
model="res.company"/>


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove a blank line

Comment thread report_xml/models/__init__.py Outdated

from . import report_action
from . import report_generator
# from . import xsd_checked_report

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this comment because no file is provided

@@ -0,0 +1,48 @@
# -*- coding: utf-8 -*-
# Copyright (C) 2014-2015 Grupo ESOC <www.grupoesoc.es>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add license

@@ -0,0 +1,24 @@
# -*- coding: utf-8 -*-
# Copyright (C) 2014-2015 Grupo ESOC <www.grupoesoc.es>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add license

@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
<data>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove data tags and change openerp by odoo

@etobella

etobella commented Jul 7, 2017

Copy link
Copy Markdown
Member Author

Fixes applied

@MiquelRForgeFlow

Copy link
Copy Markdown
Contributor

cc: @rgarnau

@JordiBForgeFlow
JordiBForgeFlow merged commit c69a47d into OCA:10.0 Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants