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

[Feature] Assign new colors for the products in the admin area #1569

Merged
merged 23 commits into from Apr 25, 2016

Conversation

dmitrychekalin
Copy link

@dmitrychekalin dmitrychekalin commented Apr 12, 2016

https://fameandpartners.atlassian.net/browse/WEBSITE-204

Problem:
Ability to create ProductColorValues on admin ui

  • associate product
  • associate option_value (somehow? Create it?)

Implemented as a new form for the product colors page http://prnt.sc/att0yz. Colors (except already assigned colors for this product) are loading dynamically.

@tiagoamaro, is there a requirement to delete colors from a product?

  • YES ,Tania wanted to delete colors in fame_admin too .

@namproctin
Copy link

This works for creating a new color , but we need to associate a newly created color to a dress in admin as well

@tiagoamaro
Copy link
Contributor

tiagoamaro commented Apr 12, 2016

This error was caused by a rack bug, introduced on versions prior to 1.6.1. Its opened file verification was considering parameters that weren't a file as a file, and it raised this error. The explanation for this issue can be found here rack/rack#814.

Updating Rack and QAing preprod is the better option for the bug introduced by this issue.

Update: updating rack will be impossible, since modern ActionPack depends on older Rack versions. I would vote on just changing the multipart_part_limit to a specified number, where this error won't happen.

Example: we now have 230 colors on the dress-color option value (Spree::OptionType.color.count). If we limit that Rack file limit to 300, that should be fine until Rails is updated to version 4.

Suggestion (though it's stupid using puts instead of logger, but it's default and it'll work on dev fine)

if Gem::Version.new(Rails::VERSION::STRING) < Gem::Version.new('4')
  Rack::Utils.multipart_part_limit = 300
else
  puts '------------------------------------------------------------------------------------------------------'
  puts 'Rails is on its 4 version! Upgrade Rack to its latest version to solve rack/rack#814, "Too many open files"'
  puts '------------------------------------------------------------------------------------------------------'
end

@tiagoamaro tiagoamaro added the bug label Apr 14, 2016
@dmitrychekalin dmitrychekalin changed the title [Fix] remove rack multipart limit [Feature] Assign new colors for the products in the admin area Apr 18, 2016
.row
.col-md-3
p
b SKU
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slim tip: when you have small nested tags like this you can use two points to "inline nest" them

.col-md-3 : p : b
  SKU

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool tip, thanks a lot!

@namproctin
Copy link

some screenshots and test cases are needed

@tiagoamaro
Copy link
Contributor

That's a very good point @namproctin. The Admin UI has been lacking tests from developer confidence that'll "simply work". As it grows, that assertion could be false.

uber (~> 0.0.7)
representable (3.0.0)
declarative (~> 0.0.5)
uber (~> 0.0.15)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, careful when upgrading major versions of other gems you're not actually using. Those could break behavior from other areas/specs. Was this upgrade intended?

@tiagoamaro
Copy link
Contributor

tiagoamaro commented Apr 21, 2016

👍 LGTM! 👍

There's a small comment on the Gemfile.lock on updating unwanted gems.

Just CI complaining, probably because of fixture database. Going to prepare one and commit here.

@namproctin any comments on this?

@namproctin
Copy link

making color deletion feature on fame_admin is wanted

= render 'admin_ui/grid/grid_collection_filters', collection_url: product_colors_path
= link_to 'Create a Color', new_product_color_path, class: 'btn btn-primary'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 'Create a new product-color association' OR 'Add new color to product'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@namproctin's right. This isn't a "New Color Creation" action. IMO, "Add new color to product" is fine.

@tiagoamaro
Copy link
Contributor

@dmitrychekalin , freel free to :shipit:

This now has a full CRUD on ProductColorValues =D

@tiagoamaro tiagoamaro merged commit bb8a3ee into master Apr 25, 2016
@tiagoamaro tiagoamaro deleted the fix/spree-options-types-form branch April 25, 2016 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants