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

[12.0][product_dimension] Update volume when dimension are updated by file import #11

Open
wants to merge 2 commits into
base: 12.0-mig-product_dimension
Choose a base branch
from

Conversation

florian-dacosta
Copy link
Member

Hi @rvalyi
Could you give me a feedback on this PR?
First commit complete the migration (split readme + remove readonly on dimension, it seems you forgot this one)

Second commit : the goal is to update the volume even when dimensions are not updated from the UI.
I only manage file importing, I don't see another way to create product (in native Odoo modules) and if modules create/write on products, I guess they should play the onchanges themselves...

What do you think? I made a mixin to avoid duplicating code, but maybe it is not a good practice to make a so small mixin?
Also, I assume that when importing file, Odoo make one write by record, but I am not totally sure, I'll try to make some tests to be sure.

Anyway, if you are not sure about the second commit, we could make the first commit in your branch, merge it into OCA and I'll do a second PR with second commit...It could even go into a separated module but I don't think it should!

@codecov
Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #11 into 12.0-mig-product_dimension will increase coverage by 1.54%.
The diff coverage is 100%.

Impacted file tree graph

@@                      Coverage Diff                       @@
##           12.0-mig-product_dimension      #11      +/-   ##
==============================================================
+ Coverage                       76.94%   78.49%   +1.54%     
==============================================================
  Files                              19       19              
  Lines                             347      372      +25     
==============================================================
+ Hits                              267      292      +25     
  Misses                             80       80
Impacted Files Coverage Δ
product_dimension/models/product.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 689d07e...7d10d5d. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 78.495% when pulling 7d10d5d on 12-improve-product_dimension-migration into 689d07e on 12.0-mig-product_dimension.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 78.495% when pulling 7d10d5d on 12-improve-product_dimension-migration into 689d07e on 12.0-mig-product_dimension.

@florian-dacosta
Copy link
Member Author

@bguillot
It would be nice to have your opinion also.

This work well and I guess is not bad for performance since we can include the volume in vals before the write. But the thing is that it work only for import.
With module like mass_editting, we still face the problem where volume does not update.
If we extend the logic to every right (instead of making it specific for import), I guess we would catch all cases, but then we would like to to a lot of write.

For instance, if we got 1 write on 100 products, we would end writing 100 time the volume...

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

Successfully merging this pull request may close these issues.

2 participants