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

Make weight calculator use line_item.final_weight_volume rather than variant.weight #3895

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@luisramos0
Copy link
Contributor

commented May 31, 2019

This is for cases where the final weight is set manually in the BOM

What? Why?

Closes #3661

Re 3661, the fees are being recalculated, the problem is that the manual weight was not being picked up.
The weight calculator for fees was always using the variant weight, the fixed value. When the final weight is set by the seller on the BOM, that value is stored in the line_item.final_weight_volume field, the weight calculator is now using that value if it is present.

What should we test?

We need to verify all cases where the weight calculator is used.

Release notes

Changelog Category: Fixed
Manual weight values introduced by the seller in the backoffice are now picked up when calculating fees by weight based calculators.

@luisramos0 luisramos0 self-assigned this May 31, 2019

@luisramos0 luisramos0 force-pushed the luisramos0:fees_based_on_final_weight branch 2 times, most recently from eb8a7b3 to 55b7bae May 31, 2019

luisramos0 added some commits May 31, 2019

Make weight calculator use line_item.final_weight_volume rather than …
…variant.weight for cases where the final weight is set manually in the BOM

@luisramos0 luisramos0 force-pushed the luisramos0:fees_based_on_final_weight branch from 55b7bae to 4551149 May 31, 2019

@luisramos0 luisramos0 changed the title Make weight calculator use line_item.unit_value rather than variant.weight Make weight calculator use line_item.final_weight_volume rather than variant.weight May 31, 2019

@mkllnk

mkllnk approved these changes Jun 13, 2019

Copy link
Member

left a comment

Nice code! How does it feel to actually improve the functionality of the software? 😉

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

ahaha, yes! it feels weird I am not just fixing specs in the 2-0-stable branch 🤣

@kristinalim
Copy link
Member

left a comment

@luisramos0 I was wondering about the _volume part of the attribute name and checked... This attribute may be used to indicate volume or custom unit quantity for a variant too. So using this attribute in the weight calculator is a bit more complicated.

actual_kg_per_variant = (final_units_per_variant / total_units_per_variant) * kg_per_variant

In consideration of above, this is another possible gotcha:

  • 123 g (smallest weight unit) unit value is 123
  • 123 mL (smallest volume unit) unit value is 0.123
  • 123 pc unit_type is 123
@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

interesting Kristina.
the original weight calculator uses variant.weight, this field is not updated for volumes and items.
so, the calculator as is will return zero for volumes and items.
I think we can keep that behaviour, so I am adding a clause to the method for that. Please have a look at this new commit.

@luisramos0 luisramos0 force-pushed the luisramos0:fees_based_on_final_weight branch from a527695 to 59ad45a Jun 18, 2019

@luisramos0 luisramos0 force-pushed the luisramos0:fees_based_on_final_weight branch from 59ad45a to 160b535 Jun 18, 2019

@kristinalim

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@luisramos0 I think this line updating Spree::Variant#weight is only to sync the weight of the variant with its unit value when the unit type is weight? When the unit type is not weight, the user can specify the weight at the variant form (not Bulk Edit Product page).

@luisramos0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

ah, nice one Kristina, I didnt see that part of this puzzle 🙄
Following from that, I see that although variant.weight can be set (for products in volume or items), when the order is created, the line_item.final_weight_volume (maybe that's why it's called _volume) is not populated from that weight field but from the unit_value (which is the volume when product is on volume). here

When the product is set for "items" the final_weight_volume is set from the unit_value as well which is the number of items in a variant...

I think it's better to leave this logic above as is with unit_value (volume or items) populating final_weight_volume...
We could change this and populate it from variant.weight if it is present. But the adjustment feature in the BOM page can be used to adjust the final VOLUME of a variant in the order... and we would loose that feature if we change it to be weight.

So, I think the current code in this PR is ok as is. Do you see a better alternative?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.