Pi/updating variants bug #2604
Pi/updating variants bug #2604
Conversation
There was a problem hiding this comment.
Nice testing. From your commit history I can't actually see which spec is covering the fix you did and which specs are just additional coverage. I think the commit messages could explain the intent better or some of the commits could have been squashed together to ease reviewing. :-)
| mark_as_invalid(entry, attribute: 'name', error: I18n.t('admin.product_import.model.no_product')) | ||
| return | ||
| end | ||
|
|
||
| match.variants.each do |existing_variant| | ||
| unit_scale = match.variant_unit_scale | ||
| products.map(&:variants).flatten.each do |existing_variant| |
There was a problem hiding this comment.
I feel this can be simpler with #flat_map
There was a problem hiding this comment.
Thanks, I didn't know about #flat_map
| mark_as_new_product(entry) | ||
| return | ||
| end | ||
|
|
||
| # Otherwise, if a variant exists with matching display_name and unit_value, update it | ||
| match.variants.each do |existing_variant| | ||
| products.map(&:variants).flatten.each do |existing_variant| |
| expect(page).to_not have_selector '.invalid-count' | ||
| expect(page).to have_selector '.create-count', text: "2" | ||
| expect(page).to_not have_selector '.update-count' | ||
| expect(page).to_not have_selector '.update-count' |
Matt-Yorkley
force-pushed
the
pi/updating_variants_bug
branch
from
c1ca1cb to
4dfbbd6
Compare
|
@sauloperez It'd be great for the UK if we could get these little bug fixes into the next release as we will need this on production... 🙏 |
|
Sorry, I forgot about this one. |
|
Please folks let us know with a comment when you stage something so we receive a notification ;-) I'm not sure how we should test this one as it was for a specific case bugging in the UK. @Matt-Yorkley @lin-d-hop do we know what caused the bug more specifically so we can do a proper testing? I'm happy to test but I don't know what to test exactly... in the issue I don't know what was in the file that was imported... |
|
Sorry @myriamboure, I thought you go check the test ready column in ZenHub |
|
@sauloperez @myriamboure Actually I do go through the zenhub column and I think @sstead is doing that too. So I really just need the label and the assigned tester if there s already one. As we said Myriam that I would ask you when you need to test, I think there is no need for comments. Do you agree ? |
|
Hum... @RachL I think we discussed and agreed to do both to avoid confusion, that's what we wrote in the process. So I would suggest we try the process we decided and if it doesn't work then we adapt? Cause some devs only put message and no label, some do the other side and some do both so let's try to let people do the same thing and see how it works? |
|
@myriamboure as you wish. But so far I've never had an issue with the label. So that's why if it's easier I'm proposing this. |
|
from slack: @lin-d-hop ;-) |
One thing that makes finding staged things easier @sauloperez @luisramos0 @Matt-Yorkley @mkllnk is to make sure they're at the top of the test ready column. Yesterday I moved the pr-staged-es up because it was lost further down the list. Just spoke to @sstead and she doesn't mind with/without a comment. She comes in on her day and looks for staged things on AU and then throughout the day she and Maikel communicate on slack when things are staged/tested. |
Given this ☝️ I'm moving PR to the Ready to Go column. @lin-d-hop move it back if this isn't the case. |
|
|
|
actually I didn't test it...I was waiting for a confirmation in the slack product import channel to know what to test :( @daniellemoorhead |
|
Uh oh... looks like I read the comment stream incorrectly 😱 Not sure what to do now that it's merged... @RachL @Matt-Yorkley suggestions? |
|
We were testing this on staging with large and complex inputs to get it right, and we quietly pushed this branch to UK production about a week ago so we could use it live (once we were satisfied). It's been used since and is working correctly without any problems. |
|
|
What? Why?
Closes #2602
Fixes some old logic on finding/matching products when updating existing variants that was leading to strange user feedback in the validation step, and improved some related specs.
What should we test?
Issue outlined in Slack channel with example large CSV file. Validation should now be working correctly during import.
Release notes
Fixed validation issue when updating existing products during product import.
Changelog Category: Fixed