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

[10.0][IMP] New module contract_line_extended. #129

Closed
wants to merge 19 commits into from

Conversation

NL66278
Copy link

@NL66278 NL66278 commented Nov 30, 2017

Restores link between contract line (account.analytic.invoice.line) and contract
(account.analytic.account).
Also adds reference fields partner_id, date_start and date_end to contract line.

@pedrobaeza
Copy link
Member

Please rebase on current 10.0 that is green

@NL66278
Copy link
Author

NL66278 commented Dec 11, 2017

@pedrobaeza Rebase has been done. As well as commit squash. @lasley Everything is now green on travis and runbot.

# We already had analytic_account_id to refer to the contract to which
# this line belongs. Unfortunately this is overwritten by
# account.analytic.contract.line which (ab)uses the same field to refer
# to the contract template.
Copy link
Contributor

@lasley lasley Dec 11, 2017

Choose a reason for hiding this comment

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

Hmmm this feels to me like I broke something with the new implementation while being lazy. Am I understanding correctly that after turning a contract template (account.analytic.contract) into an active contract (account.analytic.account), the account.analytic.account.lines that are created maintain a reference ID corresponding to the account.analytic.contract.line instead of being rewritten correctly? If so this, was not my intent and seems like an extension of #80

@lasley
Copy link
Contributor

lasley commented Dec 12, 2017

@NL66278 - I looked deeper into this after reading #124, and I get it now. I didn't consider a related field with this implementation, but this makes sense in hindsight.

IMO it would be better to fix this on the base module though- Otherwise we're just designing around my stupidity.

@NL66278
Copy link
Author

NL66278 commented Dec 12, 2017

@lasley Hi, something definitely broke. Well mistakes happen sometime, I have been guilty of a lot of them. I agree it would be best to solve it in the main module. The question is what the best approach would be. Personally I would prefer to merge contracts and templates. Then a template (which could have a boolean field marking it as a template), would have start date, partner and things like that optional. If it is preferred to keep contracts and templates separate, I think the inheritance should go from templates and template lines to contract and contract lines. And finally we might do something about the fact that templates are called contracts in the model name, which is mighty confusing.

@lasley
Copy link
Contributor

lasley commented Dec 12, 2017

I agree it makes sense to combine the two. I think we could actually skip the boolean field and just consider a contract as a template if it does not have a partner defined on it. We don't want a partner in the templates for the most part, because it's the one getting sold to multiple partners. This would work out too, because then we'd gain the reporting of having the templates as a parent analytic account.

I think it makes sense to keep the explicit contract model, and try to pull as much of the contract logic out of the analytic account as possible. The mixing of that logic has always been kind of dirty in my mind anyways, and is what I kind of half designed around with the template. Hindsight is 20/20 🚀

@NL66278
Copy link
Author

NL66278 commented Jul 19, 2018

I close this PR in favour of this one, that is a direct backport of what has already been merged in 11.0: #172. Any extra things done in this PR might be part of an extra module.

@NL66278 NL66278 closed this Jul 19, 2018
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.

3 participants