Skip to content

[11.0] [MIG] report_xlsx - #171

Merged
JordiBForgeFlow merged 13 commits into
OCA:11.0from
etobella:11.0-mig-report_xlsx
Nov 9, 2017
Merged

JordiBForgeFlow merged 13 commits into
OCA:11.0from
etobella:11.0-mig-report_xlsx

Conversation

@etobella

Copy link
Copy Markdown
Member

Migration to 11.0.
Creation of reports has changed a little because report_swx has disappeared on v11. However, changes are not complicated.

@gabrielcardoso21

Copy link
Copy Markdown

@etobella, very nice job! I was having really hard times migrating this module. I shure would get there, but it would take at least until the end of the week.

@pedrobaeza pedrobaeza added this to the 11.0 milestone Oct 26, 2017
@pedrobaeza pedrobaeza mentioned this pull request Oct 26, 2017
12 tasks
@pedrobaeza

Copy link
Copy Markdown
Member

@gabrielcardoso21 please review then the PR.

@etobella any chance of increasing test coverage?

@etobella
etobella force-pushed the 11.0-mig-report_xlsx branch 4 times, most recently from bc13867 to 068e6fb Compare October 27, 2017 10:15
@etobella

Copy link
Copy Markdown
Member Author

@pedrobaeza Test coverage improved, but controllers are not being tested right now.

@rgarnau rgarnau left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM 👍

@elicoidal elicoidal left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

some nitpicking.

Comment thread report_xlsx/README.rst
Contributors
------------

* Adrien Peiffer <adrien.peiffer@acsone.eu>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would suggest that you add your name as contributor

Comment thread report_xlsx/__init__.py Outdated
@@ -0,0 +1,7 @@
# -*- coding: utf-8 -*-
# Copyright 2015 ACSONE SA/NV (<http://acsone.eu>)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

be consistent and remove the copyright notice in all init files

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.

You can remove also coding line

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.

@elicoidal Suggested changes have been included.

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.

@pedrobaeza coding could be removed from all files, isn't it?

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.

@etobella
etobella force-pushed the 11.0-mig-report_xlsx branch 3 times, most recently from 9cac5c7 to 23f252b Compare October 27, 2017 12:14

@elicoidal elicoidal left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

js and xml can/should contain copyright notices too.
Not blocking for that though

Comment thread report_xlsx/tests/__init__.py Outdated
@@ -0,0 +1,3 @@
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some remaining.

@etobella
etobella force-pushed the 11.0-mig-report_xlsx branch from 23f252b to 692da64 Compare October 30, 2017 10:56
@JordiBForgeFlow
JordiBForgeFlow merged commit 8437da8 into OCA:11.0 Nov 9, 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.

10 participants