Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.0] mis_builder Sorrento #189

Merged
merged 182 commits into from May 24, 2016
Merged

Conversation

@sbidoul
Copy link
Member

sbidoul commented Apr 29, 2016

This is the work done on mis_builder at the Sorrento Sprint.

Thanks to all who contributed code and ideas! It was a great event.

This branch should be usable as is, although not yet perfect.

Sprinters (in particular @adrienpeiffer @sebastienbeau @gfcapalbo), feel free to commit your last small changes here if any.

Then let's review and merge fast so the work in account_financial_report_qweb that depends on this can continue.

sbidoul and others added 30 commits Oct 21, 2015
Code is simpler too but here we really lose functionality.
…h the debit and the credit are zero and balances among which debit and credit nullify
@ThomasBinsfeld
Copy link

ThomasBinsfeld commented May 17, 2016

@sbidoul Launch flake8 with --ignore ("flake8 --ignore E128 " for example) and it will show the under/over-indented lines

@sbidoul
Copy link
Member Author

sbidoul commented May 17, 2016

@ThomasBinsfeld thanks! I would not have thought ignoring an error would raise others.

@coveralls
Copy link

coveralls commented May 17, 2016

Coverage Status

Coverage remained the same at 77.77% when pulling 022b8a8 on oca-sorrento:9.0-mis_builder-sorrento into be32b21 on OCA:9.0.

@sbidoul
Copy link
Member Author

sbidoul commented May 17, 2016

Ok, so now I consider the code is much cleaner and maintainable with a proper separation of concerns:

  • mis_report.py: the core module providing the report templates and computation logic relying on a bunch of lower level features (aep, accounting_none, simple_array, mis_report_style)
  • mis_report_instance.py: providing instanciation of report templates for given time periods
  • the various renderings: widget, qweb-pdf, xslx

There is full feature parity across the 3 renderings.

For more information on new features and fixes compared to the v8 version, see the changelog: https://github.com/oca-sorrento/account-financial-reporting/blob/022b8a8998d7f38fb40f0e82185211b2f044ab62/mis_builder/CHANGES.rst

The test coverage is reasonably complete (78%, with the AccoutingExpressionProcessor fully tested except some error cases).

I still have plenty of ideas for improvements and features, but I'll stop here for now!

Happy reviewing!

@moylop260
Copy link

moylop260 commented May 17, 2016

Should we fix the duplicated csv id?

mis_builder/__init__.py:1: [W7906(duplicate-id-csv), ] Duplicate id "access_mis_report_kpi_expression" in ir.model.access.csv file
mis_builder/__init__.py:1: [W7906(duplicate-id-csv), ] Duplicate id "access_mis_report_subkpi" in ir.model.access.csv file
mis_builder/__init__.py:1: [W7906(duplicate-id-csv), ] Duplicate id "access_mis_report_style" in ir.model.access.csv file
@sbidoul
Copy link
Member Author

sbidoul commented May 17, 2016

@moylop260 perhaps, perhaps. I don't know (yet) what this warning means.

@moylop260
Copy link

moylop260 commented May 17, 2016

Help me to use a clearer message for the following case:
csv

duplicate-id-csv

@moylop260
Copy link

moylop260 commented May 17, 2016

If you have a proposal for this message you could comment here: OCA/pylint-odoo#32

@sbidoul
Copy link
Member Author

sbidoul commented May 17, 2016

@moylop260 the message is just fine. Useful check, thanks! Fixed.

@coveralls
Copy link

coveralls commented May 17, 2016

Coverage Status

Coverage remained the same at 77.77% when pulling 910cd1e on oca-sorrento:9.0-mis_builder-sorrento into be32b21 on OCA:9.0.

@moylop260
Copy link

moylop260 commented May 17, 2016

Thanks @sbidoul
👍

<data noupdate="1">

<record id="ir_cron_crm_action" model="ir.cron">
<field name="name">Vaccum temporary report</field>

This comment has been minimized.

Copy link
@lepistone
<openerp>
<data noupdate="1">

<record id="ir_cron_crm_action" model="ir.cron">

This comment has been minimized.

Copy link
@lepistone

lepistone May 23, 2016

Member

better XMLID?

# as it does not make sense to distinguish 0 from "no data"
if v is not AccountingNone and \
mode in (self.MODE_INITIAL, self.MODE_UNALLOCATED) and \
float_is_zero(v, precision_rounding=2):

This comment has been minimized.

Copy link
@lepistone

lepistone May 23, 2016

Member

Is it correct to hardcode 2 here?

This comment has been minimized.

Copy link
@sbidoul

sbidoul May 23, 2016

Author Member

Well, ideally it should come from the kpi style which itself defaults to the report style. But bringing that value down here is a lot of code for very little benefit (this only serves to distinguish 0 from null in the initial balance).

I think I'll change 2 to 4 so be on the safe side and we'll see if it creates problems in practice (I bet not).

This comment has been minimized.

Copy link
@elicoidal

elicoidal May 24, 2016

Is there any reason not to take the decimal precision set up for accounting?

This comment has been minimized.

Copy link
@sbidoul

sbidoul May 24, 2016

Author Member

@elicoidal you are right! in this area of mis_builder, we rely on accounting precision, not the precision set in the styles. So I changed to use the decimal places of the company currency, and adapted the AEP api to pass the company in the constructor instead of moving it around in each method.

This comment has been minimized.

Copy link
@elicoidal

elicoidal May 24, 2016

my very little piece for a huge job that you have produced so far!

This comment has been minimized.

Copy link
@sbidoul

sbidoul May 24, 2016

Author Member

Thanks. As we say every little bit helps. And I'm glad to have had new contributors in Sorrento.

@classmethod
def get_balances_variation(cls, company, date_from, date_to,
target_move='posted'):
""" A convenience method to obtain the variantion of the

This comment has been minimized.

Copy link
@lepistone

lepistone May 23, 2016

Member

variation

sbidoul added 4 commits May 23, 2016
…sh 0 from null in initial balances

This should be slightly on the safer side. Ideally, this rounding precision
should come from the kpi style (which defaults to the report style), but
that would be a lot of code for little benefits.
…initial balances are null or 0
@coveralls
Copy link

coveralls commented May 24, 2016

Coverage Status

Coverage remained the same at 77.786% when pulling 5319238 on oca-sorrento:9.0-mis_builder-sorrento into be32b21 on OCA:9.0.

# -*- coding: utf-8 -*-
# © 2014-2015 ACSONE SA/NV (<http://acsone.eu>)
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).
""" A trivial immutable array that supports basic arithmetic operations.

This comment has been minimized.

Copy link
@lepistone

lepistone May 24, 2016

Member

This is somewhat like numpy.array without pulling all of numpy, am I right?

This comment has been minimized.

Copy link
@sbidoul

sbidoul May 24, 2016

Author Member

@lepistone it is. I kind of thought pulling numpy was not appropriate 😉
In the end SimpleArray does a bit more wrt exception handling (DataError).

This comment has been minimized.

Copy link
@lepistone
@lepistone
Copy link
Member

lepistone commented May 24, 2016

Great work, thanks! 👍

I think I can merge this!

@lepistone lepistone merged commit 568fbb6 into OCA:9.0 May 24, 2016
3 checks passed
3 checks passed
ci/runbot runbot build 3147595-189-531923 (runtime 64s)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 77.786%
Details
@pedrobaeza pedrobaeza mentioned this pull request Sep 24, 2016
7 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.