-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
12570 - Fix Variant Unit field is Out of Sync with the Pop-out #12602
12570 - Fix Variant Unit field is Out of Sync with the Pop-out #12602
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this solves it by ensuring there's value in the underlying field, but I don't see an answer as to why it was out of sync with what's displayed on the screen.
Looking at _variant_row.html.haml
, I can see that unit_value_with_description
introduces a default of 1
, when unit_value
is blank.
Your solution ensures unit_value is never blank, with a different default from the last variant. I think 1 is a better default (at least that's the current behaviour).
So I think that your helper should provide a default of 1, and we can remove the 1
from _variant_row
.
Oh, it might be event more complicated than that. We want Sorry, this system is really complicated, it took me ages to understand it, and I was too tired to try and document it. I've created a screenshot to help: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Product-amounts-units#current-state Also I have a diagram I drew to show the relationship between fields: (edit: moved diagram to wiki also ) |
Sure, thanks, David. I'll dig into this a bit more. |
Hi @chahmedejaz , just letting you know there are more conflicts on the way 😞 |
Thanks for the heads up @dacook. I'll wrap this ASAP. |
- New variant unit_value is empty, so +VariantUnits::OptionValueNamer.new(variant).name+ returns "" - Now we are making sure that new variant unit_value won't be empty
2553945
to
d835429
Compare
Sorry I just noticed the description of PR is kind of misleading, I've updated it now. |
@dacook - PR is ready for review now. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good, thanks @chahmedejaz !
It's going to clash with the product refactor but I'll address that in due time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay, thanks for fixing this longrunning issue!
I think this is good, but I have a few questions where I think the code could be simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Hi @chahmedejaz, Before stagingI could not reproduce the exact original issue:
After staging
ConclusionI found no issues with your solution. |
What? Why?
unit_value
present in it, due to whichdisplay_as
field is also empty and variant is unable to be saved.unit_value
of 1 xvariant_unit_scale
,unit_value
is present, then it sets the placeholder value for thedisplay_as
field, which was emptyWhat should we test?
Changelog Category (reviewers may add a label for the release notes):