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

[ADD][8.0] stock_analytic #53

Merged
merged 23 commits into from
Dec 15, 2016
Merged

Conversation

andhit-r
Copy link
Member

@andhit-r andhit-r commented Feb 4, 2016

Move stock_analytic into OCA/account-analytic. The module retrieve from https://github.com/julius-network-solutions/julius-openobject-addons and https://github.com/ClearCorp-dev/stock-logistics-workflow.

Commit history preserved from both branch. Several merge conflict had been resolved.

Contributors
------------

* Fabio Vílchez <fabio.vilchez@clearcorp.co.cr>
Copy link
Member

Choose a reason for hiding this comment

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

Add here Julius people and you

@pedrobaeza
Copy link
Member

Same comments as in #52

@andhit-r
Copy link
Member Author

andhit-r commented Feb 4, 2016

@pedrobaeza i'm not confident regarding my retrieving technique. Is it safe to proceed?

@pedrobaeza
Copy link
Member

Yeah, I think so. It seems totally correct.

@andhit-r
Copy link
Member Author

andhit-r commented Feb 4, 2016

@pedrobaeza I noticed that this module depend on purchase and sale module. Is it wise to depend on those modules since we only need stock_account to give analytic account capability to stock.move?

Maybe I could create stock_sale_analytic, stock_purchase_analytic, and maybe stock_mrp_analytic to achieve modularity.

@pedrobaeza
Copy link
Member

For what is that modules needed? If there's no other choice, to keep modularity it would be good to split it.

@andhit-r
Copy link
Member Author

andhit-r commented Feb 4, 2016

I mean this original module will force users to install sale and purchase. I think the main functionality for this module only to give analytic account on stock.move. So when stock.move create account.move.line it will automatically fill analytic account field on account.move.line. So no need to depend on sale and purchase.

stock_sale_analytic
depend: stock_analytic, sale_stock
functionality: automatically give analytic account on stock.move according to project_id field on sale.order

stock_purchase_analytic
depend: purchase, stock_analytic
functionality: automatically give analytic account on stock.move according to account_analytic_id field on purchase.order

I don't like to force users install modules that they don't need. But if my suggestion is somehow overkill I will continue as it is.

@pedrobaeza
Copy link
Member

No, of course is a good proposition, but I was asking for the reason to need sale or purchase modules. Now I understand that is to propagate the corresponding analytic accounts, but I agree that that should be done in the modules you propose. Maybe a better name should be sale_stock_analytic y purchase_analytic to follow Odoo's naming convention.

@andhit-r
Copy link
Member Author

andhit-r commented Feb 5, 2016

Ok @pedrobaeza. Thanks for your input. I will finish this module first.

@andhit-r andhit-r changed the title [ADD][WIP][8.0] stock_analytic [ADD][8.0] stock_analytic Feb 5, 2016
@andhit-r
Copy link
Member Author

andhit-r commented Feb 5, 2016

Travis didn't run again

@pedrobaeza
Copy link
Member

Seeing https://www.traviscistatus.com/, they experimented a problem and now they have some troubles to get up to date with all the checks, so wait a bit or try again to resubmit.

@andhit-r
Copy link
Member Author

andhit-r commented Feb 5, 2016

close to restart travis

@andhit-r andhit-r closed this Feb 5, 2016
@andhit-r andhit-r reopened this Feb 5, 2016
@andhit-r
Copy link
Member Author

@pedrobaeza would you kindly give need review label on this PR. Thanks before.

@pedrobaeza
Copy link
Member

Done

@andhit-r
Copy link
Member Author

Thanks @pedrobaeza.

@andhit-r
Copy link
Member Author

Hi @OCA/accounting-maintainers would you kindly review this PR. Many thanks

@pedrobaeza
Copy link
Member

👍

@andhit-r
Copy link
Member Author

Thanks @pedrobaeza

@andhit-r
Copy link
Member Author

Hi @OCA/accounting-maintainers would you kindly review this PR. Many thanks

@JordiBForgeFlow
Copy link
Sponsor Member

@aheficent and me will review.

@andhit-r
Copy link
Member Author

andhit-r commented Aug 5, 2016

Thank you @jbeficent

res[1][2].update({
'analytic_account_id': move.account_analytic_id.id,
})
return res
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO I think It should create the analytic entries for income or expenses account only. This creates two analytic entries, one for the debit and one for the credit, even for assets accounts. Do you agree with me @andhit-r ?
cc @jbeficent

Copy link
Member Author

Choose a reason for hiding this comment

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

@aheficent agree. I push a commit to correct this. Analytic account will only be assigned if account != valuation account

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for really-really late reply. Kindly review this again. Many thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @andhit-r I like the proposed changes. There's a problem with runbot, so i will on local. The code looks correct to me.

@AaronHForgeFlow
Copy link
Contributor

What else is needed to get this merged?

@pedrobaeza
Copy link
Member

Get more reviewers. You haven't approved the PR yet and there's a question unanswered as I see.

@andhit-r
Copy link
Member Author

andhit-r commented Dec 3, 2016

Try to restart travis

@andhit-r andhit-r closed this Dec 3, 2016
@andhit-r
Copy link
Member Author

andhit-r commented Dec 4, 2016

@jbeficent @aheficent all checks green. Would you kindly review it again. Many thanks.

@mikevhe18 @azmimr67 please help review

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

I tested on local and reviewed the code and it works correctly. The only issue is related with the name of the field. What do you think about this?

class StockMove(models.Model):
_inherit = "stock.move"

account_analytic_id = fields.Many2one(
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO i think that the name of the field should be analytic_account_id. I think the name of the field should match the name of other modules as for example the account module https://github.com/OCA/OCB/blob/8.0/addons/account/account.py#L2398 or the project module https://github.com/OCA/OCB/blob/8.0/addons/project/project.py#L231

Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Tested on runbot + code review 👍

Thank you @andhit-r and @mikevhe18 Great job!

@mikevhe18
Copy link

👍 tested on runbot

@JordiBForgeFlow JordiBForgeFlow merged commit 24cf4ba into OCA:8.0 Dec 15, 2016
This was referenced Mar 31, 2017
sbejaoui pushed a commit to acsone/account-analytic that referenced this pull request Nov 18, 2020
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.

9 participants