[1624] Add option to disable requests by esoterik · Pull Request #1625 · YaleSTC/reservations

esoterik · 2016-08-10T21:07:19Z

Blocking on #1586.

Resolves #1624

orenyk · 2016-09-19T19:23:25Z

Can we rebase this before review?

esoterik · 2016-10-09T20:42:59Z

Rebased!

orenyk

A few minor changes and one discussion about the multiple extracted methods in the service object. Overall well done, hopefully we can actually get this stuff merged in 😄.

orenyk · 2016-10-10T13:48:16Z

lib/reservation_creator.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true


OMG, Service Objects!

orenyk · 2016-11-15T19:03:50Z

app/views/reservations/_requests_disabled.html.erb

+<div id="content" class="col-md-9">
+  <% unless @errors.empty? %>
+    <p>
+    <h3><i class="fa fa-exclamation-triangle warning-icon"></i> Would you like to resolve the following error(s)?</h3>


This should probably read Please resolve the following error(s): since they can't be overriden by the student.

orenyk · 2016-11-15T19:27:32Z

lib/reservation_creator.rb

+    override && !cart_errors.blank?
+  end
+
+  def valid_override?


Do we really need to extract this separately? I feel like we have a bunch of little methods that aren't strictly necessary. For example, the only time it's used (L31 - override? && !(override? && !notes.blank?)), is equivalent to override? && notes.blank? (if I'm expanding correctly).

I guess I see how this is useful in making the code easier to read, but I think what's bothering me is the use of override? inside valid_override? when it's just cancelled out in the override? necessarily. This also applies to valid_request?. Thoughts?

orenyk · 2016-11-15T19:28:40Z

spec/controllers/reservations_controller_spec.rb

+    before do
+      allow_any_instance_of(described_class).to \
+        receive(:cart).and_return(cart)
+      allow_any_instance_of(described_class).to receive(:fix_cart_date)


Yeesh these smell a bit to me, but it's definitely out of scope for this issue.

orenyk · 2016-11-15T19:33:16Z

spec/lib/reservation_creator_spec.rb

+                                         current_user: current_user,
+                                         **attrs)
+        results = creator.create!
+        expect(results[:result]).to be_truthy


Do we need to make assertions about the value of results[:result] since it's used when processing the successful result in the controller?

I think so -- this is also a part of the move away from controller tests (though Reservations might not really make it all the way there)

Wait, I feel like this discussion wasn't resolved. I think was asking if we should also be asserting against the actual value of results[:results], not just whether or not it's truthy. Am I missing something?

orenyk · 2016-11-15T20:13:31Z

spec/mailers/user_mailer_spec.rb

  before(:each) do
    @ac = mock_app_config(admin_email: 'admin@email.com',
-                          disable_user_emails: false)
+                          disable_user_emails: false,


Where did all of these come from and why did we only run into this now?

Oh wait, is it from switching from spy to instance_spy? Do you remember why we did this?

I'm pretty sure it's from switching to instance_spy which is safer because it only lets you stub out things that exist in the real class.

esoterik · 2016-11-20T21:32:57Z

refactored the service object a bit and rebased!

Resolves #1624 - Adds a Service Object for reservation creation

orenyk

One more comment on the ReservationCreator and a question about the templates. Almost there!

orenyk · 2016-11-22T12:41:07Z

app/views/reservations/new.html.erb

-	  <%= render :partial => "new_request" %>
-	<% end %>
-</div>
+  <%= render partial: 'new_reservation' %>


I'm not sure why we need partials for this if we now have independent views for each of the different cases that are rendered independently - can we just move the partial content into the full views?

I think it's necessary for how the reservation_form JS works

Yea fair enough. I feel like there might be a way to improve this but it's really not critical.

orenyk · 2016-11-23T04:28:07Z

lib/reservation_creator.rb

+  end
+
+  def needs_notes?
+    (request? || override?) && notes.blank?


Isn't request? || override? equivalent to !cart_errors.blank?, since request? is true if override is falsey and override? is true if override is truthy?

This would eliminate the need for the override? method, I believe.

esoterik · 2016-12-02T15:43:40Z

app/controllers/reservations_controller.rb

+      flash[:error] = 'Please give a short justification for this reservation'
+      @notes_required = true
+      @request_text = if AppConfig.get(:request_text).empty?
+                        'Please give a short justification for this '\


this is a little out of scope but it seems like we should just set this as a default value for request_text rather than having this live in the controller

Yea... that's a great point. I'll open up an issue.

esoterik · 2016-12-02T16:16:43Z

pushed an update

orenyk

Ok revisiting an old discussion, but this should be it. Thanks!

orenyk · 2016-12-07T17:15:43Z

spec/lib/reservation_creator_spec.rb

+                                         current_user: current_user,
+                                         **attrs)
+        results = creator.create!
+        expect(results[:result]).to be_truthy


Wait, I feel like this discussion wasn't resolved. I think was asking if we should also be asserting against the actual value of results[:results], not just whether or not it's truthy. Am I missing something?

esoterik added pri: 1 0 - needs review 0 - blocked pri: 2 and removed pri: 1 labels Aug 10, 2016

esoterik added the diff: 5 label Aug 25, 2016

esoterik added pri: 3 and removed pri: 2 0 - blocked labels Sep 12, 2016

orenyk self-assigned this Sep 19, 2016

esoterik force-pushed the 1624_disable_requests_option branch from 92c57bc to 435ba14 Compare October 9, 2016 20:35

orenyk suggested changes Nov 15, 2016

View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Nov 15, 2016

esoterik mentioned this pull request Nov 20, 2016

[1645] blackout conflict checking #1651

Merged

esoterik force-pushed the 1624_disable_requests_option branch from 38f1ec7 to 5da4f10 Compare November 20, 2016 21:32

esoterik added 0 - needs review and removed 1 - working labels Nov 20, 2016

Add option to disable requests

bfe944e

Resolves #1624 - Adds a Service Object for reservation creation

esoterik force-pushed the 1624_disable_requests_option branch from 5da4f10 to bfe944e Compare November 20, 2016 21:41

orenyk suggested changes Nov 23, 2016

View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Nov 23, 2016

esoterik commented Dec 2, 2016

View reviewed changes

get rid of override? method

35082ce

esoterik added 0 - needs review and removed 1 - working labels Dec 2, 2016

orenyk mentioned this pull request Dec 7, 2016

Move default request prompt out of controller #1654

Open

orenyk suggested changes Dec 7, 2016

View reviewed changes

orenyk added 1 - working and removed 0 - needs review labels Dec 7, 2016

Conversation

esoterik commented Aug 10, 2016

Uh oh!

orenyk commented Sep 19, 2016

Uh oh!

esoterik commented Oct 9, 2016

Uh oh!

orenyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoterik commented Nov 20, 2016

Uh oh!

orenyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esoterik commented Dec 2, 2016

Uh oh!

orenyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants