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

[13.0] [MIG] date_range #99

Merged
merged 37 commits into from
Oct 25, 2019
Merged

Conversation

lmignon
Copy link
Sponsor Contributor

@lmignon lmignon commented Oct 18, 2019

fixes and supersedes #95

lmignon and others added 30 commits October 18, 2019 12:05
* [ADD] Basic structure for the new date range module

* [IMP] Add a basic description into the README

* [IMP] Basic implementation

* [IMP] First working implementation

* [IMP] Improve datamodel

* [ADD] Add basic tests for date.range

* [PEP8]

* [PYLINT]

* [DEL] Remove unused code

* [IMP] Remove unsused dependencies into the JS

* [IMP] Better operator label for date range

* [DEL] Remove unused file

* [IMP] Better user experience by showing the select input only once empty

* [FIX]Try to fix tests that fails only on travis by adding an explicit cast on the daterange methods parameters

* [FIX]Try to fix tests that fails only on travis by adding an explicit cast on the daterange methods parameters

* [FIX]Try to fix tests that fails only on travis by using postgresql 9.4

* [FIX]Try with postgresql 9.2 since the daterange method has appeared in 9.2

* [IMP] Add a limitation into the module description to warm about the minimal version of postgresql to use

* [IMP]Add multi-company rules

* [IMP]Remove unused files

* [FIX] Add missing brackets into JS

* [FIX] Overlap detection when company_id is False

* [IMP] Add default order for date.range

* [IMP] Add date range generator

* [FIX] OE compatibility

* [FIX] Travis

* [IMP] Code cleanup and improves test coverage

* [FIX] Add missing dependency on 'web'

* [PYLINT] remove unused import

* [FIX] Add missing copyright

* [FIX] Limits are included into the range

* [IMP][date_range] Security

* [IMP] Improve module description

* [IMP] Spelling
* Improve 'name' for generator wizard

  ir.rule should be active by default
* Don't auto-add '-' after prefix when generating date ranges via wizard
* code fine tuning suggested by Sylvain Garancher
Currently translated at 89.1% (41 of 46 strings)

Translation: server-ux-11.0/server-ux-11.0-date_range
Translate-URL: https://translation.odoo-community.org/projects/server-ux-11-0/server-ux-11-0-date_range/ar/
If any module adds a required field on company, module fails. Executing it on
post_install, there's no problem.
Currently translated at 82.6% (38 of 46 strings)

Translation: server-ux-11.0/server-ux-11.0-date_range
Translate-URL: https://translation.odoo-community.org/projects/server-ux-11-0/server-ux-11-0-date_range/da/
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
Currently translated at 100.0% (46 of 46 strings)

Translation: server-ux-12.0/server-ux-12.0-date_range
Translate-URL: https://translation.odoo-community.org/projects/server-ux-12-0/server-ux-12-0-date_range/it/
Currently translated at 100.0% (46 of 46 strings)

Translation: server-ux-12.0/server-ux-12.0-date_range
Translate-URL: https://translation.odoo-community.org/projects/server-ux-12-0/server-ux-12-0-date_range/pt/
@pedrobaeza
Copy link
Member

Please check runbot warning:

2019-10-18 10:25:50,201 160 WARNING openerp_test odoo.addons.base.models.ir_module: python external dependency dateutil should be replaced by it's PyPI package name

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Oct 18, 2019

@pedrobaeza If I use de Pypi package name, pylint fails

Missing external dependency "dateutil.relativedelta" from manifest. More info: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#external-dependencies

😒

a pylint issue I suppose?

@pedrobaeza
Copy link
Member

Well, I'm only saying about the thing that makes runbot CI red. But isn't that an Odoo requirement? Do we need to declare explicitly in this module? I don't think so.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Oct 18, 2019

@pedrobaeza Yes it's an odoo requirement. I don't know why we must now declare this dependency compared to 12.0 but I'm not choked to have to do it since it's always better to not rely on transitively acquired dependencies when explicitly using one of them. I don't know how to solve the issue in this specfic case...

@pedrobaeza
Copy link
Member

@sbidoul @guewen you were dealing with this specific case, isn't it?

Copy link
Member

@astirpe astirpe left a comment

Choose a reason for hiding this comment

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

In the meantime: Tested OK, code review: OK

@sbidoul
Copy link
Member

sbidoul commented Oct 18, 2019

See OCA/pylint-odoo#263

It needs to evolve to avoid conflict with the local isort config, and handle the new python dependency mechanism of Odoo 13 (referencing PyPI distribution names instead of import name).

In the meantime it does no harm to add the dependency explicitly.

@pedrobaeza
Copy link
Member

In the meantime it does no harm to add the dependency explicitly.

Except for the runbot warning...

@sbidoul
Copy link
Member

sbidoul commented Oct 18, 2019

@lmignon The manifest must say python-dateutil, not dateutil. https://pypi.org/project/python-dateutil/. That should silence runbot.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Oct 18, 2019

@sbidoul but in this case, pylint fails...

@sbidoul
Copy link
Member

sbidoul commented Oct 18, 2019

Ah yes, sorry. Then the pylint check must be temporarily disabled in .pylintrc-mandatory. In any case when pylint will be updated and released we'll need to push at minimum a new .pre-commit-config.yaml.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Oct 18, 2019

@sbidoul OK I'll do like proposed

Pylint-odoo is not able to detect properly whitelisted odoo's dependencies if an isort configuration file is present into the directory. Therefore the check fails by complaining that the dateutil dependecy is not declared into the manifest. A fix could have been to declare the dependency into the manifest . Unfortunately, in this case, a warning will be issued by runbot complaining that the python external dependency dateutil should be replaced by it's PyPI package name. Unfortunalely, if we put the Pypi package name into the external dependencies, pylint fails again since it's not able to play with PyPI distribution names.
The workaround is to temporarily disable this check into pylint. I every case, all the pre-commit files will be reset once pylint will be fixed
@pedrobaeza
Copy link
Member

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-99-by-pedrobaeza-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Oct 25, 2019
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot OCA-git-bot merged commit 6dc3479 into OCA:13.0 Oct 25, 2019
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7e4c455. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet