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.

Sign up for GitHub

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

Jump to bottom

[12.0] [MIG] analytic_base_department #209

Merged
merged 12 commits into from
Apr 2, 2019
Merged

[12.0] [MIG] analytic_base_department #209

merged 12 commits into from
Apr 2, 2019

Conversation

anandkansagra
Copy link
Member

Issue Reference

@oca-clabot
Copy link

Hey @anandkansagra, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

This was referenced Dec 4, 2018
Closed
Closed
@pedrobaeza pedrobaeza added this to the 12.0 milestone Dec 4, 2018
@anandkansagra anandkansagra force-pushed the 12.0-mig-analytic_base_department branch from fcc2f40 to f39881b Compare December 4, 2018 11:11
@oca-clabot
Copy link

Hey @anandkansagra, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@anandkansagra anandkansagra force-pushed the 12.0-mig-analytic_base_department branch from f39881b to 5b7ccb1 Compare December 5, 2018 14:38
@oca-clabot
Copy link

Hey @anandkansagra, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@Nikul-Chaudhary Nikul-Chaudhary mentioned this pull request Feb 8, 2019
Closed
emagdalenaC2i
emagdalenaC2i approved these changes Mar 24, 2019
View reviewed changes
Copy link

@emagdalenaC2i emagdalenaC2i left a comment

Choose a reason for hiding this comment

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

Code review

dreispt
dreispt reviewed Mar 25, 2019
View reviewed changes
analytic_base_department/__manifest__.py Outdated
"author": "Camptocamp, Daniel Reis, Odoo Community Association (OCA)",
"license": "AGPL-3",
"category": "Generic Modules/Projects & Services",
"website": "https://github.com/OCA/account-analytic",
"depends": ["account",
"analytic",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed? The module uses account.analytic.account Model, provided by the analytic module.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dreispt ,

If we will install the account module then analytic module will automatically installed due to dependency. So no need to put analytic module in the dependency
Here is the Reference:- https://github.com/odoo/odoo/blob/12.0/addons/account/__manifest__.py#L18

dreispt
dreispt reviewed Mar 25, 2019
View reviewed changes
Copy link
Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

Thanks @anandkansagra, good catch. I would then prefer to keep the closest dependency: this needs analytic but doesn't need account. So I would rather remove the account dependency.

analytic_base_department/views/analytic.xml
<field name="department_id"/>
</field>
</field>
</record>
Copy link
Member

Choose a reason for hiding this comment

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

No point in changing the XML indentation, but discussing that would be bikeshedding.

nikul-serpentcs
nikul-serpentcs approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

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

Code Review LGTM 👍

@anandkansagra
Copy link
Member Author

Thanks @anandkansagra, good catch. I would then prefer to keep the closest dependency: this needs analytic but doesn't need account. So I would rather remove the account dependency.

@dreispt ,
Technically it will not throw an error. I have seen that in the analytic module, there seems no menuitem. All the menuitem related analytic module is defined in the account module. So do we need to change it in this PR?

@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). 🤖

dreispt and others added 12 commits April 2, 2019 09:46
@dreispt @pedrobaeza
Split analytic_department into analytic_base_department to isolate de…
180f380 
…pendency on account
@yvaucher @pedrobaeza
[PORT][9.0] analytic_base_department
bc62fda 
Replace yml test by a python unit test

Move description in README.rst

Replace <openerp> by <odoo>
@thinkwelltwd @pedrobaeza
10.0 mig analytic_base_department
81bc183
@oca-transbot @pedrobaeza
OCA Transbot updated translations from Transifex
0e7d41d
@pedrobaeza
[MIG] analytic_base_department: Migrate to 11.0
cd1896b
@oca-transbot @pedrobaeza
OCA Transbot updated translations from Transifex
d2f199d
@Tonow-c2c @pedrobaeza
Merge analytic_department to analytic_base_department + modif test
67aedc4
@oca-transbot @pedrobaeza
OCA Transbot updated translations from Transifex
c08f2ef
@oca-travis @pedrobaeza
[UPD] Update analytic_base_department.pot
37097a7
@pedrobaeza
Translated using Weblate (Spanish)
126fa07 
Currently translated at 100,0% (6 of 6 strings)

Translation: account-analytic-11.0/account-analytic-11.0-analytic_base_department
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-11-0/account-analytic-11-0-analytic_base_department/es/
@pedrobaeza
Translated using Weblate (Catalan)
9359f75 
Currently translated at 100,0% (6 of 6 strings)

Translation: account-analytic-11.0/account-analytic-11.0-analytic_base_department
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-11-0/account-analytic-11-0-analytic_base_department/ca/
@anandkansagra @pedrobaeza
[MIG] analytic_base_department: Migration to 12.0.
46cfac0
@pedrobaeza pedrobaeza force-pushed the 12.0-mig-analytic_base_department branch from 5b7ccb1 to 46cfac0 Compare April 2, 2019 07:49
@pedrobaeza
Copy link
Member

I have squashed a bit commit history and merging when finished.

@pedrobaeza pedrobaeza merged commit 090235e into OCA:12.0 Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreispt dreispt dreispt left review comments

@emagdalenaC2i emagdalenaC2i emagdalenaC2i approved these changes

@nikul-serpentcs nikul-serpentcs nikul-serpentcs approved these changes

Assignees
No one assigned
Labels
approved ready to merge
Projects
None yet
Milestone
12.0
Development

Successfully merging this pull request may close these issues.