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

[Spree Upgrade] shipping_category now required for products #3672

Merged

Conversation

Projects
None yet
5 participants
@Matt-Yorkley
Copy link
Contributor

commented Apr 1, 2019

What? Why?

Closes #3663

Updates Product Import with a validation to reflect the fact that shipping_category is now mandatory.

What should we test?

Shipping category is now required in Product Import. Validation messages are clearer when the field is missing.

Documentation updates

Product import user guide will need updating.

@Matt-Yorkley Matt-Yorkley self-assigned this Apr 1, 2019

@Matt-Yorkley Matt-Yorkley force-pushed the Matt-Yorkley:spree2/pi_shipping_category branch from 725409f to b3bab01 Apr 1, 2019

@sigmundpetersen

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Should this target 2-0-stable rather than master?

@Matt-Yorkley Matt-Yorkley force-pushed the Matt-Yorkley:spree2/pi_shipping_category branch from b3bab01 to bcc6b1b Apr 1, 2019

@Matt-Yorkley Matt-Yorkley changed the base branch from master to 2-0-stable Apr 1, 2019

@Matt-Yorkley Matt-Yorkley force-pushed the Matt-Yorkley:spree2/pi_shipping_category branch from bcc6b1b to 75e6b3d Apr 1, 2019

@luisramos0 luisramos0 changed the title shipping_category now required for products [Spree Upgrade] shipping_category now required for products Apr 1, 2019

@luisramos0
Copy link
Contributor

left a comment

nice!
I think this requires a spec.

@sauloperez
Copy link
Contributor

left a comment

Agree with @luisramos0 this needs test coverage.

@Matt-Yorkley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Ready for re-review

@luisramos0
Copy link
Contributor

left a comment

cool, thanks!
I recommend removing some repetition below.

Show resolved Hide resolved spec/models/product_importer_spec.rb Outdated

@luisramos0 luisramos0 requested a review from sauloperez Apr 16, 2019

@Matt-Yorkley Matt-Yorkley force-pushed the Matt-Yorkley:spree2/pi_shipping_category branch from 90a4b0a to 97a3403 Apr 30, 2019

@Matt-Yorkley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@luisramos0 when I rebased this the test changed in relation to an on_demand product. I had to change this to get it green: 41c0b33

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Nice @Matt-Yorkley
From the tests I see users will need to input shipping category name now and not shipping category id. will shipping category id also work?

@Matt-Yorkley

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

From the tests I see users will need to input shipping category name now and not shipping category id. will shipping category id also work?

Shipping id works unofficially, but it's always used the human-readable shipping category name.

@RachL RachL added the pr-staged-fr label May 2, 2019

@RachL RachL self-assigned this May 2, 2019

@RachL

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@Matt-Yorkley it seems that the "can't be blank" mention is still appearing alongside the clear error message, see below for a screenshot of an import with a product with all columns and a product without shipping category.

Can you remove the lonely "can't be blank" easily?

image

@RachL RachL removed the pr-staged-fr label May 5, 2019

@RachL

This comment has been minimized.

@Matt-Yorkley Matt-Yorkley force-pushed the Matt-Yorkley:spree2/pi_shipping_category branch from 41c0b33 to 3323ac7 May 5, 2019

@Matt-Yorkley

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@RachL ready for re-test 👍

@RachL

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Awesome! This is ready to go

image

@RachL RachL removed the pr-staged-uk label May 5, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

⚠️ this needs to be merged into master, not 2-0-stable

@luisramos0 luisramos0 changed the base branch from 2-0-stable to master May 5, 2019

@luisramos0
Copy link
Contributor

left a comment

I have a question about the last change.
Moving to code review just to check that and also to double check we agree this should be merged into master.

@@ -7,5 +7,3 @@
( {{entry.attributes.display_name}} )
%p.error{ng: {repeat: "(attribute, error) in entry.errors", show: "ignore_fields.indexOf(attribute) < 0" }}
&nbsp;-&nbsp; {{error}}
%p.error{ng: {repeat: "(attribute, error) in entry.product_validations"}}
&nbsp;-&nbsp; {{error}}

This comment has been minimized.

Copy link
@luisramos0

luisramos0 May 5, 2019

Contributor

I dont understand this change @Matt-Yorkley
product_validations is still being populated in some cases, why would you remove it from the page?

This comment has been minimized.

Copy link
@Matt-Yorkley

Matt-Yorkley May 6, 2019

Author Contributor

There was some duplication here, as the product_validations are actually being added to entry.errors elsewhere. Also the formatting was not right on the duplicate, as you can see:

This comment has been minimized.

Copy link
@luisramos0

luisramos0 May 6, 2019

Contributor

ok, I dont understand the code... why do we need to keep both entry.errors and entry.product_validations?

def mark_as_invalid(entry, options = {})
  entry.errors.add(options[:attribute], options[:error]) if options[:attribute] && options[:error]
  entry.product_validations = options[:product_validations] if options[:product_validations]
end
@sauloperez

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I don't think it's a critical bug so not worth including it into v2.0. Merge to master 👍

@sauloperez sauloperez merged commit fc38906 into openfoodfoundation:master May 7, 2019

1 check passed

semaphoreci The build passed on Semaphore.
Details
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.