[237] change cart reservation #1221

AlanLiu96 · 2015-04-06T17:45:38Z

Resolves #237

Redesign & Add form to confirm reservation page
Allow user to edit cart & dates on confirm reservation page
Lock cart when changing quantities
Add submit_cart_updates_form in catalog controller to update cart with changes on confirm reservation page & redirect
Add change_reservation_dates in catalog controller to update cart dates
Separate update_cart in application controller into two different methods: performing cart updates and appropriate redirections/js calls
Add reservation_form.js to dynamically switch between reservations and reservation requests
Add controller tests & feature tests

(edited to reflect full changes)

mnquintana · 2015-04-06T17:46:31Z

app/assets/stylesheets/reservations/_new.css.scss

@@ -1,3 +1,7 @@
 .warning-icon {
  color: $btn-warning-bg;
 }
+
+#empty_cart_btn {
+	margin-bottom: 0px;


Too much indentation

orenyk · 2015-04-07T19:27:58Z

I've started working through this now, here are some thoughts from playing around with it:

Overall, editing the cart from the page is a nice addition, but from a UX perspective it's a little confusing to me (as a user) what's going on. For example, we're running all of the update actions and the flash is being set accordingly (Cart Updated.), but the workflow should really be that you make your changes locally, then hit "Update Reservation" to try again. At the same time, I'm not sure that we want to be setting up an entirely separate mechanism for these updates vs cart updates since they're effectively the same.
This would benefit greatly from @connormcl's work in Allow changes to item quantity in cart #1114; I'm almost tempted to block this PR on that issue so you can incorporate a quantity field for equipment items and the associated method vs the current implementation that only has the "x" button to remove from cart.
The "Edit Reservation" button is entirely too inconspicuous and hard to see; it took me a few seconds of clicking around to figure out how to get the form out.
Similarly, the "Update Reservation" button doesn't exactly pop out to me; this is basically a rehash of my first point, but the workflow for this page wasn't entirely intuitive to me at first glance, and I know this app pretty well at this point. We might want to have a discussion about how exactly to implement this and if we can make things smoother from a UX perspective.

You can address @mnquintana's comments regardless, and I'm going to give the code a first read through, but think about the UX here and how we can improve it.

orenyk · 2015-04-07T19:30:55Z

app/assets/javascripts/reservation_form.js

+function toggle_form(){
+  if ($('#form').is(':visible')){
+    pause_cart(); // lock the cart so users can't change it while page is reloading
+    location.reload();// update the info here so it's all new when we close the form


missing space between semicolon and comment

orenyk · 2015-04-07T19:33:13Z

Following up on my previous comment, the fact that clicking on the "Edit Reservation" button when the form is open keeps the changes (which it has to since we've already updated the cart) is incredibly counter-intuitive to me.

orenyk · 2015-04-07T19:36:41Z

Another note: the "Empty Cart" button is actually broken since it calls the /reservations/empty_cart method, which is matched by the reservation route and expects an id (which empty_cart evaluates to zero). That button should be replaced with a link that empties the cart and redirects to the catalog.

orenyk · 2015-04-07T19:37:32Z

app/views/reservations/_new_request.html.erb

+    </div>
+  <% end %>
+</div>
+<%# edit reservation here%>


There should be spaces after / before the ERB tags (i.e. <% # edit reservation here %>)

Also, if this code and the code in the _new_request.html.erb partial is identical, you should separate it out into a partial as well.

AlanLiu96 · 2015-04-27T00:21:50Z

I am creating a new form instead of embedding the cart. currently working on putting in the new routes and logic.

orenyk · 2015-05-27T03:29:22Z

@AlanLiu96 is this ready to be reviewed? For future reference, we'd use the 4 - Needs Review label for that.

AlanLiu96 · 2015-05-27T12:21:56Z

Almost, I'm finishing up the tests tonight/tomorrow, and then It'll be ready for review

orenyk · 2015-05-27T13:52:15Z

Great, keep up the good work 😄

orenyk · 2015-09-07T19:44:43Z

Whoops, I just realized that you had updated this status in May! Looking over it now...

orenyk · 2015-09-07T19:52:23Z

app/controllers/catalog_controller.rb

@@ -41,6 +42,48 @@ def update_user_per_cat_page
    end
  end

+  def submit_form # rubocop:disable MethodLength, AbcSize


This name is a bit too vague; let's make it a little more specific so that we know what it's for (updating the cart during confirmation... but not in so many words 😛).

Another, more fundamental, comment - this method duplicates a lot of code from other methods, which is not good. I'd really like it if we could simply call the ApplicationController#update_cart method in here to deal with updating the dates and then something like your code to handle quantity updates. This would require tweaking the structure of the form and potentially the update_cart method, but it would be much more sustainable.

Hmm.. I agree that it's not good that it duplicates code from update_cart, which hurts it in sustainability, but I feel like we should handle the dates in the form separately from the dates in the cart. Right now submitting the form with an error in dates doesn't affect the original cart, so the original values of the cart are restored. Integrating update_cart into it to handle the date validation on the form removes that function because it sets the dates back to their default values. I'm not sure which is more important in this case.
Another option is to call update_cart again after the validations on the form to handle the blackouts, but I'm not sure if it's worth it to call it just to handle the blackout notifications.

I'm ok with tweaking the cart date handling through this issue so that we can use the same code for both use cases. Don't forget, this issue is designed to allow for cart edits through the checkout page, so there really isn't such a thing as the "original" cart - we're meant to be editing the actual cart by submitting the form. If a user enters invalid dates here I think we'd want it to behave the way it would in the catalog views; that keeps us consistent.

That's just my opinion, though; let me know if that makes sense or if you still disagree 😄.

orenyk · 2015-09-07T21:44:13Z

This looks really good! It's crazy to compare it to what we had before 😄. I left a bunch of comments inline, here are a few other thoughts:

There's no longer an "Empty Cart" button - let's add it back in and make it redirect to the catalog after resetting the cart.
I'm not 100% settled on the UI here. I'd really like it if other members of the team played with this and gave their feedback as well.

Let me know if you have questions regarding the refactoring of the controller method, great work!

orenyk · 2015-12-22T01:33:15Z

spec/features/reservations_spec.rb

+        fill_in "quantity_field_#{@eq_model.id}", # edit and submit
+                with: (@eq_model.max_per_user)
+        quantity_forms[0].submit_form!
+        expect(page).to have_content 'Confirm Reservation'


~~I think you need to reload the page here to verify that everything still works. (EDIT: or is being treated as an HTML format request since it's from Capybara? We should confirm either way.)~~ (EDIT 2: Yea, after testing this with save_and_open_page it does submit the form with an HTML-format request so it reloads the page automatically.) Also, it doesn't look like you're testing to make sure that your change to the cart was actually saved. We could add another expectation here for that.

Also, this test will pass even if the title of the page is "Confirm Reservation Request", so it doesn't actually work. You'd probably have to add a test to ensure that it doesn't say the longer title.

orenyk · 2015-12-22T03:13:21Z

Ok, I ran through and left a bunch of notes inline. I also manually checked the various use cases specified in my earlier comment, and everything is working great, nice job!

At this point, I'd say the code is pretty much complete but the tests need a bit of work. The setup of the various specs doesn't necessarily take you to the page you're expecting or result in the expected scenario, and our tests for whether or not we're on the reservation or request view doesn't actually check what we're trying to check. When you're writing tests (and I know it gets complicated particularly with integration tests), try to make sure you know exactly what the scenario is that you're testing, that the steps you take result in that scenario, and that the expectations you run actually validate your "hypothesis". Definitely make good use of save_and_open_page in Capybara (followed by binding.pry if you want the test to wait while you view the current page) - just bear in mind that (I don't think) it shows any changes you've made to form inputs (at least, it wasn't showing quantities set to zero before submitting the form when it was testing for that, and the item was actually removed from the cart).

Use this PR as a place to comment on your testing work and don't hesitate to reach out if you have questions about any specific test. This is really fantastic work - the page feels really snappy, it's a huge improvement over our current UX (and you've really polished it through this incredibly long code review) - let's get the tests wrapped up and we can finally merge this in 😀. Let me know when it's ready for another review, good luck!

orenyk · 2016-02-10T16:16:26Z

@AlanLiu96, is this ready for re-review?

AlanLiu96 · 2016-02-10T17:05:24Z

Not yet, should be done by Friday though

orenyk · 2016-02-10T17:12:06Z

No worries just checking, thanks!

AlanLiu96 · 2016-02-11T23:05:26Z

I changed most of the tests so that they were properly redirecting to the right page and that it was running the tests as expected. The only part I'm still not sure about is how to fix the page redirection when the cart is empty. Right now it refreshes the page using js, which causes the redirect, but ideally it would directly redirect to the catalog page. I'm not sure how to make it do this when rails expects javascript.

AlanLiu96 · 2016-02-13T22:14:06Z

ready for re-review. For the javascript change, it now redirects using js to the root path.

orenyk · 2016-02-14T04:42:35Z

app/views/cart_js/reservation_form.js.erb

+resume_cart();
+
+<% if @errors.empty? or (can? :override, :reservation_errors) %>
+  $('#new_res_request').html('<%= escape_javascript(render :partial => '/reservations/new_reservation') %>');


This is very minor but I'm not thrilled with this ID (new_res_request) - we use the term 'request' to mean something specific. What about something like confirm-res-form (also note the use of dashes instead of underscores for CSS selectors)? Just a suggestion...

orenyk · 2016-02-14T05:41:04Z

Great work @AlanLiu96! I had a few minor comments but this is pretty much there. Unfortunately, you're going to have some merge conflicts to deal with before we can merge this in; once you've dealt with the remaining issues I'd recommend you squash all of your commits and then address the merge conflicts with a single rebase onto origin/master. Let's try and get this done on Monday. Really well done 😃!

AlanLiu96 · 2016-02-14T22:23:58Z

addressed all of the comments, going to go through the merge conflicts later today/tomorrow.

orenyk · 2016-02-15T06:47:49Z

Awesome, looks great! Responded to one point but it's really minor; if you're comfortable squashing on your own go ahead and you can start to work on the merge conflicts, otherwise we can work on it tomorrow night.

Resolves #237 - Redesign & Add form to confirm reservation page - Allow user to edit cart & dates on confirm reservation page - Lock cart when changing quantities - Add submit_cart_updates_form in catalog controller to update cart with changes on confirm reservation page & redirect - Add change_reservation_dates in catalog controller to update cart dates - Separate update_cart in application controller into two different methods: performing cart updates and appropriate redirections/js calls - Add reservation_form.js to dynamically switch between reservations and reservation requests - Add controller tests & feature tests

[237] change cart reservation

mnquintana reviewed Apr 6, 2015
View reviewed changes

orenyk added the 4 - Needs Review label Apr 7, 2015

orenyk reviewed Apr 7, 2015
View reviewed changes

orenyk added 1 - working and removed 4 - Needs Review labels Apr 7, 2015

AlanLiu96 added 0 - needs review and removed 1 - working labels May 14, 2015

orenyk added 1 - working and removed 0 - needs review labels May 27, 2015

AlanLiu96 added 4 - Needs Review and removed 1 - working labels May 30, 2015

orenyk reviewed Sep 7, 2015
View reviewed changes

orenyk added the 1 - working label Sep 7, 2015

orenyk reviewed Dec 22, 2015
View reviewed changes

orenyk added 1 - working and removed 4 - Needs Review labels Dec 22, 2015

AlanLiu96 added 4 - Needs Review and removed 1 - working labels Feb 13, 2016

orenyk reviewed Feb 14, 2016
View reviewed changes

orenyk added 1 - working and removed 4 - Needs Review labels Feb 14, 2016

AlanLiu96 force-pushed the 237_change_cart_reservation branch from 8b37e2d to 6430c70 Compare February 16, 2016 01:26

AlanLiu96 force-pushed the 237_change_cart_reservation branch from 6430c70 to e75f66a Compare February 16, 2016 01:42

orenyk added a commit that referenced this pull request Feb 16, 2016

Merge pull request #1221 from YaleSTC/237_change_cart_reservation

7c156ff

[237] change cart reservation

orenyk merged commit 7c156ff into master Feb 16, 2016

orenyk removed the 1 - working label Feb 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[237] change cart reservation #1221

[237] change cart reservation #1221

AlanLiu96 commented Apr 6, 2015

mnquintana Apr 6, 2015

orenyk commented Apr 7, 2015

orenyk Apr 7, 2015

orenyk commented Apr 7, 2015

orenyk commented Apr 7, 2015

orenyk Apr 7, 2015

orenyk Apr 7, 2015

AlanLiu96 commented Apr 27, 2015

orenyk commented May 27, 2015

AlanLiu96 commented May 27, 2015

orenyk commented May 27, 2015

orenyk commented Sep 7, 2015

orenyk Sep 7, 2015

orenyk Sep 7, 2015

AlanLiu96 Sep 12, 2015

orenyk Sep 13, 2015

orenyk commented Sep 7, 2015

orenyk Dec 22, 2015

orenyk Dec 22, 2015

orenyk commented Dec 22, 2015

orenyk commented Feb 10, 2016

AlanLiu96 commented Feb 10, 2016

orenyk commented Feb 10, 2016

AlanLiu96 commented Feb 11, 2016

AlanLiu96 commented Feb 13, 2016

orenyk Feb 14, 2016

orenyk commented Feb 14, 2016

AlanLiu96 commented Feb 14, 2016

orenyk commented Feb 15, 2016

[237] change cart reservation #1221

[237] change cart reservation #1221

Conversation

AlanLiu96 commented Apr 6, 2015

Choose a reason for hiding this comment

orenyk commented Apr 7, 2015

Choose a reason for hiding this comment

orenyk commented Apr 7, 2015

orenyk commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanLiu96 commented Apr 27, 2015

orenyk commented May 27, 2015

AlanLiu96 commented May 27, 2015

orenyk commented May 27, 2015

orenyk commented Sep 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyk commented Sep 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyk commented Dec 22, 2015

orenyk commented Feb 10, 2016

AlanLiu96 commented Feb 10, 2016

orenyk commented Feb 10, 2016

AlanLiu96 commented Feb 11, 2016

AlanLiu96 commented Feb 13, 2016

Choose a reason for hiding this comment

orenyk commented Feb 14, 2016

AlanLiu96 commented Feb 14, 2016

orenyk commented Feb 15, 2016