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

7.0 mis builder backport from 8.0 #90

Merged
merged 184 commits into from Dec 9, 2015
Merged

Conversation

lmignon
Copy link
Sponsor

@lmignon lmignon commented Jun 15, 2015

Back port mis_builder from 8.0 (#86)

@gurneyalex
Copy link
Member

My PR fixing the travis and runbot builds on the 8.0 branch were recently merged. Can you rebase your PR?

@lmignon
Copy link
Sponsor Author

lmignon commented Jul 24, 2015

@gurneyalex done.

@gurneyalex
Copy link
Member

I let you backport the fixes asked on #86 when they get applied in there 😸

@JordiBForgeFlow
Copy link
Sponsor Member

I have applied the fixes asked on #86, in acsone#7. Please @lmignon review and merge if you agree.

@sebalix
Copy link

sebalix commented Oct 26, 2015

I implemented this module on a 7.0 instance for some statistics unrelated to accounting, it works like a charm (with SUM and AVG queries). 👍

@sbidoul
Copy link
Member

sbidoul commented Nov 2, 2015

rebased

@pedrobaeza
Copy link
Member

Can you squash the commit history. 179 commit seems a lot of commits.

@sbidoul
Copy link
Member

sbidoul commented Nov 2, 2015

@pedrobaeza the commits are in the 8.0 branch. And there are several authors.

Not sure why travis is complaining. Anyway errors seem unrelated so 👍

Thanks @jbeficent for the last effort.

@sbidoul
Copy link
Member

sbidoul commented Nov 2, 2015

Can you squash the commit history. 179 commit seems a lot of commits.

It's a lot of work too 😉

@pedrobaeza
Copy link
Member

All commits from different authors come together, so not too much, but I'm not going to force you.

@JordiBForgeFlow
Copy link
Sponsor Member

@sbidoul Not sure why Travis fails. It's not related to the changes made.

elif query.aggregate == 'max':
agg = max
elif query.aggregate == 'avg':
agg = lambda l: sum(l) / float(len(l))
Copy link

Choose a reason for hiding this comment

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

I encounter a problem here, a division by 0 could happen if there is no rows to compute.
I fixed it with the following line:

agg = lambda l: l and sum(l) / float(len(l)) or 0

Copy link
Member

Choose a reason for hiding this comment

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

@sebalix this was a backport issue. It should be fixed with @jbeficent's last commit: 30744ab

Copy link

Choose a reason for hiding this comment

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

@sbidoul @jbeficent thanks

@JordiBForgeFlow
Copy link
Sponsor Member

Travis says:
mis_builder/openerp.py:25: [C8102(manifest-required-key), ] Missing required key "images" in manifest file

But in OCA maintainer-tools template: https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py I don't see this parameter being a requirement.

@nmoturi
Copy link

nmoturi commented Dec 3, 2015

We been using MIS Builder V7 successfully and would be glad if its gets pushed to OCA asap.

Thanks,
Naran

lmignon and others added 15 commits December 3, 2015 21:30
Conflicts:
	mis_builder/__openerp__.py

cherry-pick from v8
Make it closer to the evaluation context available for server actions.

Conflicts:
	mis_builder/models/mis_builder.py

cherry-pick from v8
The dependency is indeed declared in __openerp__.py and will be
enforced. Odoo evaluates all python files, even for modules which are
not installed, and crashes if an unneeded dependency is needed.

Conflicts:
	mis_builder/report/__init__.py

cherry picked
 The description has been generated using rst2html5.
The path to the images has been manually fixed by removing 'static/description/' from the path.
The body element has been replaced by a section wrapping the whole content.
Conflicts:
	mis_builder/README.rst

cherry pick to v7
so min(x, y, ...) and min([x, y, ..]) both work as expected.
Conflicts:
	mis_builder/models/mis_builder.py

cherry pick from v8
@sbidoul
Copy link
Member

sbidoul commented Dec 3, 2015

I rebased. No idea why travis fails. Seems unrelated. IMO, this is good for merging.

@lmignon
Copy link
Sponsor Author

lmignon commented Dec 4, 2015

There are 2 pytlint errors

======== Testing test_pylint ========
************* Module account_chart_report.__openerp__
account_chart_report/__openerp__.py:28: [W1401(anomalous-backslash-in-string), ] Anomalous backslash in string: '\ '. String constant might be missing an r prefix.
account_chart_report/__openerp__.py:28: [W1401(anomalous-backslash-in-string), ] Anomalous backslash in string: '\ '. String constant might be missing an r prefix.

And travis need to fixed. The tests fails since the dependencies are no more checked out in the directory exepected by travis_run_tests. We must declare the dependencies in a oca_dependencies.txt file.

@lmignon
Copy link
Sponsor Author

lmignon commented Dec 4, 2015

Pep8 issue is fixed and travis runs but it seems that we have a regression. https://travis-ci.org/OCA/account-financial-reporting/jobs/94815699#L335

2015-12-04 07:35:47,188 22916 TEST openerp_test openerp.modules.module: FAIL: test_datetime_conversion (openerp.addons.mis_builder.tests.test_mis_builder.test_mis_builder)
2015-12-04 07:35:47,188 22916 TEST openerp_test openerp.modules.module: Traceback (most recent call last):
2015-12-04 07:35:47,188 22916 TEST openerp_test openerp.modules.module: `   File "/home/travis/build/OCA/account-financial-reporting/mis_builder/tests/test_mis_builder.py", line 40, in test_datetime_conversion
2015-12-04 07:35:47,188 22916 TEST openerp_test openerp.modules.module: `     'The converted date time convert must contains hour')
2015-12-04 07:35:47,188 22916 TEST openerp_test openerp.modules.module: ` AssertionError: '2014-07-04' != '2014-07-04 22:00:00'
2015-12-04 07:35:47,188 22916 TEST openerp_test openerp.modules.module: ` - 2014-07-04
2015-12-04 07:35:47,189 22916 TEST openerp_test openerp.modules.module: ` + 2014-07-04 22:00:00
2015-12-04 07:35:47,189 22916 TEST openerp_test openerp.modules.module: `  : The converted date time convert must contains hour
2015-12-04 07:35:47,189 22916 TEST openerp_test openerp.modules.module: `
2015-12-04 07:35:47,189 22916 TEST openerp_test openerp.modules.module: Ran 2 tests in 0.029s
2015-12-04 07:35:47,189 22916 TEST openerp_test openerp.modules.module: FAILED
2015-12-04 07:35:47,189 22916 TEST openerp_test openerp.modules.module:  (failures=1)

@lmignon
Copy link
Sponsor Author

lmignon commented Dec 4, 2015

@sbidoul Travis is green. It's ready to be merged

@JordiBForgeFlow
Copy link
Sponsor Member

Thanks @lmignon and @sbidoul! 👍

@sbidoul
Copy link
Member

sbidoul commented Dec 9, 2015

I'm merging now. Many thanks to all who contributed to this backport.

sbidoul added a commit that referenced this pull request Dec 9, 2015
7.0 mis builder backport from 8.0
@sbidoul sbidoul merged commit d7b417a into OCA:7.0 Dec 9, 2015
@sbidoul sbidoul deleted the 7.0-mis_builder-lmi-2 branch December 9, 2015 10:04
rconjour pushed a commit to EssentNovaTeam/account-financial-reporting that referenced this pull request Mar 16, 2020
[8.0] account_default_draft_move uses tuples instead of lists in sql operations
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.

None yet

10 participants