Skip to content
This repository was archived by the owner on Jul 24, 2020. It is now read-only.

#1085 Non-existent cart items #1090

Merged
merged 1 commit into from
Jan 7, 2015
Merged

#1085 Non-existent cart items #1090

merged 1 commit into from
Jan 7, 2015

Conversation

orenyk
Copy link
Contributor

@orenyk orenyk commented Jan 7, 2015

Resolves #1085

# would cause errors when rendering
def fix_cart_items
session[:cart].items.each do |em, _count|
session[:cart].items.delete(em) unless EquipmentModel.find_by_id(em)
Copy link
Contributor

Choose a reason for hiding this comment

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

this queries the db for every item in the cart. I guess you'd only really notice if you had lots of items in it, but it's expensive for an edge case handler. EquipmentModel.where(id: items.keys).count == items.count uses 1 query to see if the # of records returned by looking at the keys matches the original # of keys so you could guard against having to actually do the iteration unless it was necessary.

Actually, then you could just use the returned records to cherry pick the items hash using select

valid_items = EquipmentModel.where(id: items.keys).collect(&:id)
items = items.select{ |em, _count| valid_items.include? em }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hot damn that's really nice. Look at you w/ your fancy db query optimization :-P.

Copy link
Contributor

Choose a reason for hiding this comment

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

honestly i finally got comfortable with collect, inject and select while doing the reports and they are the most amazing things

@mnquintana mnquintana changed the title #1085 Non-existant cart items #1085 Non-existent cart items Jan 7, 2015
@orenyk
Copy link
Contributor Author

orenyk commented Jan 7, 2015

Goddamn spelling :-P. This should now be much leaner and I moved the fix_items method to the Cart model, where it really belongs. I'm actually going to write a test for this and then it's ready for final review.

@orenyk
Copy link
Contributor Author

orenyk commented Jan 7, 2015

Ok, now we're ready.

@squidgetx
Copy link
Contributor

awesome, squash/merge away

better method with fewer db queries

specs ftw
@orenyk
Copy link
Contributor Author

orenyk commented Jan 7, 2015

Woot passing tests! Merging!

orenyk added a commit that referenced this pull request Jan 7, 2015
@orenyk orenyk merged commit 324390f into master Jan 7, 2015
@mnquintana mnquintana deleted the 1085_edge_case branch February 20, 2015 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to render cart if Equipment Models in cart destroyed no longer in DB
2 participants