[1114] change item quantities in cart

[connormcl]

Resolved #1114

[squidgetx]

this looks pretty solid. I might just say we should remove the commented code out (save it somewhere, but it doesn't need to be merged to master, especially since the cart UX will go hand in hand with the API-Client rewrite in which we'll scrap all our ERB anyway) @orenyk what do you think'?

[mnquintana]

@connormcl Would you mind merging in master first? The diff is comparing against the version of master you branched from, instead of the one with the latest updates. (note: there will be a merge conflict)

[connormcl]

I'll merge in master now, then resubmit.

[orenyk]

You might have issues rebasing if you perform a merge; I'd almost recommend a non-squashing rebase (git rebase master) over a merge commit if we're looking to ultimately squash commits.

I'm with @squidgetx about the commented code, we'll be rewriting most of this ultimately and may decide not to impose validations on catalog buttons depending on how we rework the cart UX (#1119).

EDIT: Note that you'll have to force push to get your newly rebased branch back up to GitHub, make sure you only push up this branch by specifying it (git push -f origin 1114_change_item_quantities_in_cart). Also note that you'll have to update the views to reflect the Bootstrap upgrade regardless of how you deal with merge conflicts.

[connormcl]

This should be good to merge now that it's been rebased.

[orenyk]

This layout is broken (see below), we need to make sure that it looks right, including potentially cases with double digit item counts.

[mnquintana]

IMO we should probably implement this with HTML5 number inputs instead of custom buttons, which would be less brittle and less likely to break on layout changes without serious layout / styling considerations.

[orenyk]

Fair point, but that would involve refactoring the cart update methods. We could either add a new route for cart amount updates or replace add_to_cart and remove_from_cart with a generalized edit_cart_item method. We can discuss more tonight.

[TyPetrochko]

Going into 2 digits causes it to break for me,

[mnquintana]

@TyPetrochko @connormcl Have you tested it on smaller browser widths? I'd expect it to break even more because the column width will be smaller.

[orenyk]

@connormcl had mostly fixed this as of Sunday night, I think he was just writing some integration tests before pushing (although, to be fair @connormcl, a comment to that effect would have been helpful 😛).

[connormcl]

Like @orenyk said above me, I should have commented earlier to update. Anyways, now the cart uses a number field that should have a consistent layout regardless of window size. Additionally, I wrote some simple integration tests for changing cart item quantities/removing items. I would appreciate code review!

[mnquintana]

Just out of curiosity, is there a reason why xpath would be preferred here over a CSS selector?

[orenyk]

Not sure there's a preference, but implementing my id suggestion above would make this much simpler: fill_in "#quantity_field_#{EquipmentModel.first.id}", with: quantity.to_s (I think, double check with the Capybara docs).

[orenyk]

This seems pretty slick! Nice work 😄. I'm just going to read through the code, but the general functionality seems to work very well from playing with it.

[orenyk]

Do you need to test for items[key] <= 0 or could you just test items[key] == 0? You'd have returned already if the quantity was less than zero.

[orenyk]

Ok, almost there! I think we can replace the old cart model methods (add_to_cart and remove_from_cart) with calls to our new method, which should probably be renamed and modified so that it can add new equipment models to the items hash. Other than that and some minor CSS / spec edits, we should basically be there.

[orenyk]

Looks good!