openfoodnetwork
openfoodnetwork copied to clipboard
openfoodfoundation
Added HEIGHT, WEIGHT, WIDTH, DEPTH columns to packing reports by customer.
What? Why?
Closes #9487
Adds the Height, Weight, Width, Depth fields to the Line Items table and makes them visible on the packing reports by customer.
As shown here:
What should we test?
- Visit variants edit page of any variant and fill in the WEIGHT, HEIGHT, WIDTH, DEPTH fields.
- Go to an order cycle and place an order with the variant you edited.
- Generate packing report by customer for that order cycle and hub.
- See that the aforementioned fields are visible there.
- Now, go back to the variant you edited and alter any of the dimensions you like.
- Now run the same report and ensure that you get the same results as the first time.
Release notes
Changelog Category: User facing changes
Added HEIGHT, WEIGHT, WIDTH, DEPTH columns to packing reports by customer.
@mkllnk Can you take a look at the rubocop error? Usually you'd have to remove redundant parts or divide the method up to avoid this but is that possible here? Thanks!
Can you take a look at the rubocop error?
I had a look and you are right. We can't divide the method because it's executed in another class. That's a big downside of this design. It would be good if found an architecture that supports smaller methods here. For now, we can just add it to the rubocop todo list. I'll add a commit.
Hi @apricot12, I assume the failing check and the conflict need to be solved before testing, right? Let me know if I am wrong or when it's ready for testing! Thanks!
@drummer83 It's okay to test as the only failing test is a linter check that we cant really help at the moment. There's a merge conflict that I have to resolve though :)
Hi @apricot12, Thanks for this pull request! I have tested it and found an issue with the weight column, but it shouldn't be a blocker for now.
Before this PR
No columns for weight and dimensions:
After this PR
Columns for weight and dimensions are displayed:
I have checked that
- the new columns can be hidden like all the other columns :heavy_check_mark:
- xlsx, pdf and csv are working as before :heavy_check_mark:
- other reports remain unchanged :heavy_check_mark:
- changing the properties of the variant doesn't change the properties of old variants in the report :heavy_check_mark:
Issues
- There are two failing checks now. --> Not sure if this is a problem.
- You can see in the image above that WEIGHT is always '0.0'. I double checked with higher values in case it's a rounding issue, but it's always like that. --> major issue, needs fixing asap
- The input in fields width, height and depth are some kind of validated, but there is no error message if the input is invalid. It is simply set to 0.0 (also if you use comma as a decimal separator). --> minor issue
I will move this to ready to go because it is an improvement with the last bit of functionality missing. @filipefurtad0 Asking your opinion about merging as it is and the failing checks.
@drummer83 Thanks for the in depth testing! And I think the issue with the weight has more to do with the calculator than this PR. I did remember being able to get the weight to display when I changed the value, but I forgot to record what exactly I did there for that to happen. I'll try it out myself tomorrow.
Possibly one more thing to add to the tech debt?
@kirstenalarsen You might want to check if this is solving the original issue completely. Knowing the individual dimensions and weight is only the first step of packing. To select the properly sized box you would at least require the sum of all the item volumes as the estimated minimum. Since we already have a 'total' row in our reports it could be another papercut to calclulate that value and the overall weight of the order. I think that's very interesting and thinking longterm we could even integrate a tool like BoxPacker or similar.
Hey @drummer83, Thanks for the great testing and nudging on this.
There are two failing checks now. --> Not sure if this is a problem.
The first failing check seems to be a rubocop warning, which was added to the to-do list on this commit 6e161bb749b0a394da6ea6dfc7bd766558ef29c9, this is ok for now.
The second failing check relates to #9643, so not introduced here.
You can see in the image above that WEIGHT is always '0.0'.
After this PR the column will be displayed but it will display incorrect values, which feels like a regression. So I'd vote to move this on back to in dev.
It's great that the dimension fields are working correctly though, so maybe it does provide enough value. Adding the feedback label, to get thoughts from @openfoodfoundation/train-drivers-product-owners.
The input in fields width, height and depth are some kind of validated, but there is no error message if the input is invalid.
You mean when editing a variant, right? If so I think this is pre-existing - I see you've opened an issue already here.
PS and perhaps out of the papercut scope: what info should be displayed on the weight column, for products/items with variant_unit = volume or items? Usually both have weight = 0.0 . I mean usually because if a weight product is converted into an items product then it will retain it's weight value on the DB, which would then be displayed on the report (when the aforementioned issue is fixed).
Moving to In Dev as per feedback from delivery circle meeting.
@filipefurtad0
Shall I try to look into why the weight isnt' being displayed properly then?
How stupid of me to not have noticed this. But the reason weight field isnt displayed properly is simply because we've set the default weight to be "0.0" on db, which means the code to copy wont work because the restraint is "if weight.nil?".
We could update the table to make sure there is no default value on weight or we could change the code to copy over the variant weight. I'd prefer to update the db table. Since if the weight was 0.0 on the variant in the first place, it'd just copy that over. So I'd vote to remove default: "0.0", null: false constraint from the weight field on spree_line_items table.
@mkllnk
I'd prefer to update the db table. Since if the weight was 0.0 on the variant in the first place, it'd just copy that over.
I agree. The variant has no default either. It makes sense to use exactly the same data structure in the line item then. Go for it.
Hello @apricot12 do you have time to fix the last failing spec?
sure. should'nt take too long.
Unless the weigth field is well filled (with 1):
the weight column is filled with 0.0:
(and the old line item is empty then)
Ok seems to be a rounded error, when I fill 10g:
Ok found. Even if the variant as 1 filled in its form (ie. /admin/products/tomatoes/variants/[VARIANT_ID]/edit):
When copying the dimensions values from variant to line_items (ie. app/models/spree/line_item.rb#copy_dimensions, it hasn't the right weight:
(rdbg) variant
#<Spree::Variant:0x00007fb57b887628
id: 12,
sku: "",
weight: 0.0,
height: 0.11e2,
width: 0.22e2,
depth: 0.33e2,
deleted_at: nil,
is_master: false,
product_id: 6,
position: 2,
cost_currency: "AUD",
unit_value: 1.0,
unit_description: "",
display_name: "",
display_as: "",
import_date: nil>
Actually, we need to test with product that has different scale/unit
Thanks to commit f54c506c1 now, the weight printed in report in the same than the weight filled in variant form:
Variant unit: "weight" Product Scale : 1 or 1000 (ie. g or kg) Weight filled in the form: 1 Value printed in report: 1
Variant unit is different than weight, we use the weight field directly:
Hey @jibees,
I somehow can't get the Weight cell to display the updated value. While this is fine for order 1 - placed before staging the PR and making the change on the variants - I'd expect this to be the case for order 2. Any ideas?
:rofl: Ok I admit I got carried away taking notes with Inkscape (I still hope they are readable :sweat_smile: )
Actually, I think the definition is quite low, I can't read all the text (at least, the one I want to read). To you have a better definition please? (Ok, I'm almost 40, I think I need to wear glasses, that's all, I'm old now)
If I remember correctly, you need to change the weight of the variant before the checkout. In the checkout, order and then report we do use a line_item that copy the variant but it's not modified if the variant is modified (which is logical, you don't want your order to be modified if a variant has changed). So I'd say:
- Fill a weight to a variant
- Checkout with that variant
- Then, you should see that variant weight in the report (at least that's what I've tested...)
(at least that's what I've tested...)
That makes sense: packing reports are about line items, not products :+1:
Agree, makes sense. I've misinterpreted the detailed testing instructions - my bad :pray:
Below, the report for three orders:
order 1 - before this PR
order 2 - after this PR
order 3 - after this PR and after editing values weight, height, width, depth
After a second edit of the values weight, height, width, depth running the report shows the same result.
Good to go!