search

spree-contrib / spree_digital

A Spree extension to enable downloadable products
http://spreecommerce.org
MIT License
117 stars 97 forks source link

Redirect appropriately on validation error #80

Closed jspizziri closed 9 years ago

jspizziri commented 9 years ago

If for some reason there is an error on the object validation, its best to redirect them appropriately. Otherwise you'll get a nasty error as there is no "new" template for digitals.

I was experiencing a validation error because of a bug in paperclip that I described here: https://github.com/spree-contrib/spree_digital/issues/79

JDutil commented 9 years ago

Would be good to add a spec.

halo commented 9 years ago

Just a less obtrusive thought: wouldn't it be easier to simply provide a new template?

jspizziri commented 9 years ago

I thought about adding a new template. My only concern would be that I feel it would unnecessarily complicate the UX. If there's going to be a new template then I think that it should be enforced across the entire UX, so that there isn't a one off situation.

halo commented 9 years ago

OK, I buy that. It would need specs because overriding methods is always a little brittle :)

jspizziri commented 9 years ago

I cleaned up the controller a bit an submitted the spec. Incidentally, during the process of writing the spec I found another little bug where the user could upload a nil attachment to the digitals model. I fixed that by adding a validation.

halo commented 9 years ago

So, in hindsight I think things like this should go in master and from master they propagate back to 3-0-stable etc. Now 3-0-stable is a little bit ahead of master :) But I cannot merge in 3-0-stable into master because once master would be merged back into 3-0-stable, anything 3-0 specific would be overriden.

Oh well, I guess that's how to learn this kind of stuff :)

The solution for now would be to simply pull in the new 3-0-stable files manually into master with git checkout 3-0-stable -- app/controllers/spree/admin/digitals_controller.rb etc...

jspizziri commented 9 years ago

I would suggest using a cherry-pick, which allows you to select specific commits and apply them to a branch. In this specific case you can do the following:

git checkout master
git cherry-pick 3cd169ca33a2e0a229c02b10ddefc088cc0e3ec9
git cherry-pick 2b135f68cd7403e47cc67cc9804a7aeb233d4ae7

If you would like I could supply you with a PR for this.

jspizziri commented 9 years ago

Here you go: https://github.com/spree-contrib/spree_digital/pull/87

However if you want to do it yourself you can just close that PR.