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][IMP] account_financial_risk sale_financial_risk: Performance. Not store fields. Links to values records. Multi-company. Multi-currency. #65

Merged
merged 1 commit into from
Jun 5, 2020
Merged

[12.0][IMP] account_financial_risk sale_financial_risk: Performance. Not store fields. Links to values records. Multi-company. Multi-currency. #65

merged 1 commit into from
Jun 5, 2020

Conversation

carlosdauden
Copy link
Contributor

@carlosdauden carlosdauden commented May 28, 2020 •
edited
Loading

*_financial_risk: (account_financial_risk, account_payment_return_financial_risk sale_financial_risk)

  • Optimize code and change stored computed fields to not store to improve multi-company support
  • Improve multi-currency support
  • Convert risk amount fields to clickable link that shows traceability of amount origin
  • New pivot views to risk amount traceability
  • Simplify class style applied on risk fields
  • Migration script to remove old stored computed fields
  • Improve tests to cover new functionallity
  • Update translation files

account_financial_risk:

  • Don't block refund invoice validation when partner has risk exception
  • Allow search partners by risk exception field
  • Remove obsolete cron

sale_financial_risk

  • Create related store commercial_partner_id field in sale order line to simplify computation
  • Rename amt_to_invoice field to risk_amount in sale order line
  • Hook and migration scripts to reduce new fields computing time

@Tecnativa TT23765

financial_risk_click

@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch from 9807ff1 to 5ab1490 Compare May 28, 2020 01:52
@carlosdauden carlosdauden mentioned this pull request May 28, 2020
Closed
@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch 2 times, most recently from f2d39f4 to 7857d65 Compare May 28, 2020 02:46
chienandalu
chienandalu reviewed May 28, 2020
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

¿No readme updates?

sale_financial_risk/hooks.py Outdated Show resolved Hide resolved
sale_financial_risk/views/sale_financial_risk_view.xml Outdated Show resolved Hide resolved
@pedrobaeza pedrobaeza added this to the 12.0 milestone May 28, 2020
pedrobaeza
pedrobaeza previously requested changes May 28, 2020
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Is Odoo removing automatically SQL columns of the previously stored computed fields? If not, you need to add that in the migration scripts.

I also find that there's no commit separation between each feature, and it even seems to include fixes that in the computation that are independent from non stored or link. Please try to split things for better review. If not, it's very difficult to hunt any possible problem

account_financial_risk/data/account_financial_risk_data.xml Show resolved Hide resolved
sale_financial_risk/models/sale.py Show resolved Hide resolved
sale_financial_risk/models/sale.py Outdated Show resolved Hide resolved
@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch 6 times, most recently from cca9a4a to 402d4d4 Compare June 2, 2020 00:58
chienandalu
chienandalu reviewed Jun 3, 2020
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Very cool :) Tested on runbot 👍 Missing a little update in the readme with the new feature.

rafaelbn
rafaelbn approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Functionally tested in runbot 👍 thanks

@rafaelbn
Copy link
Member

rafaelbn commented Jun 3, 2020

2020-06-03_14-13-54

@pedrobaeza
Copy link
Member

Are you going to perform the commit separation?

@carlosdauden
Copy link
Contributor Author

No, I'm working in other comments

@pedrobaeza pedrobaeza dismissed their stale review June 3, 2020 14:39

OK, I refrain from reviewing then. I consider that important for later references, or that at least the commit message reflects all you have done.

@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch from 402d4d4 to 1144dde Compare June 3, 2020 15:06
@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). 🤖

@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch 4 times, most recently from 28720dd to 8d5979b Compare June 5, 2020 01:26
@carlosdauden carlosdauden changed the title [IMP] *_financial_risk: Performance. Not store fields. Links to values records [12.0][IMP] account_financial_risk sale_financial_risk: Performance. Not store fields. Links to values records. Multi-company. Multi-currency. Jun 5, 2020
@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch from 8d5979b to 5be0862 Compare June 5, 2020 01:32
@carlosdauden
Copy link
Contributor Author

Changes done

@carlosdauden
[IMP] account_financial_risk account_payment_return_financial_risk sa…
0f1ad80 
…le_financial_risk:

 - Optimize code and change stored computed fields to not store to improve multi-company support
 - Improve multi-currency support
 - Convert risk amount fields to clickable link that shows traceability of amount origin
 - New pivot views to risk amount traceability
 - Simplify class style applied on risk fields
 - Migration script to remove old stored computed fields
 - Improve tests to cover new functionallity
 - Update translation files

account_financial_risk:
 - Don't block refund invoice validation when partner has risk exception
 - Allow search partners by risk exception field
 - Remove obsolete cron

sale_financial_risk
 - Create related store commercial_partner_id field in sale order line to simplify computation
 - Rename amt_to_invoice field to risk_amount in sale order line
 - Hook and migration scripts to reduce new fields computing time

TT23765
@carlosdauden carlosdauden force-pushed the 12.0-IMP-account_financial_risk-not_store_and_links branch from 5be0862 to 0f1ad80 Compare June 5, 2020 02:05
@carlosdauden carlosdauden mentioned this pull request Jun 5, 2020
Merged
@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-65-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 1fea59d into OCA:12.0 Jun 5, 2020
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza mentioned this pull request Jun 8, 2020
Merged
@carlosdauden carlosdauden mentioned this pull request Jul 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chienandalu chienandalu chienandalu approved these changes

@sergio-teruel sergio-teruel sergio-teruel approved these changes

@rafaelbn rafaelbn rafaelbn approved these changes

@pedrobaeza pedrobaeza pedrobaeza left review comments

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@carlosdauden @rafaelbn @pedrobaeza @OCA-git-bot @chienandalu @sergio-teruel