[1645] blackout conflict checking by yongjie-lin · Pull Request #1651 · YaleSTC/reservations

yongjie-lin · 2016-11-12T21:42:32Z

No description provided.

esoterik

Looks pretty good overall! There's some refactoring to do; let me know if you have any questions.

esoterik · 2016-11-20T20:22:35Z


  def update
+    p = blackout_params
+    p[:created_by] = current_user.id 


We shouldn't be updating created_by -- we want the record of whoever originally created the blackout

esoterik · 2016-11-20T20:28:20Z

+          .active
+
+    # save and exit
+    if res.empty? && @blackout.update_attributes(p)


I'm a little hesitant to do both of these at the same time -- I think we should only attempt to update the blackout if there aren't any conflicting reservations.

This is also a great opportunity to create a service object to handle blackout updating. PR #1624 has an example of a service object for reservation creation.

Essentially, this method should only get the params and pass them to the service object, which will handle the update process and return the appropriate messages (e.g. errors or the updated object)

esoterik · 2016-11-20T20:30:01Z

        it 'should not save the blackout' do
          expect { post :create, blackout: attributes }.not_to\
-            change { Blackout.all.count }
+                                                          change { Blackout.all.count }


Whitespace error!

esoterik · 2016-11-20T20:38:04Z

+                                     start_date: Time.zone.today)
+
+        it_behaves_like 'does not update blackout', attributes
+      end 


These tests are pretty good, but they should probably be replaced with tests of the service object that you'll write. PR #1625 has examples of this.

esoterik · 2016-11-20T20:38:42Z

+    if res.empty? && @blackout.update_attributes(p)
+      redirect_to @blackout, notice: 'Blackout was successfully updated.' 
    else
+      if res.any?


I don't think we need this if block -- we want the detailed reservation info always.

esoterik · 2017-01-18T16:42:23Z

@yongjie-lin is this ready for re-review?

yongjie-lin · 2017-01-18T16:48:40Z

Yeah!

…

On Wed, Jan 18, 2017 at 11:42 AM, Sydney Young ***@***.***> wrote: @yongjie-lin <https://github.com/yongjie-lin> is this ready for re-review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1651 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVZNeFjQWHysngnhncJj5uVrxpvT_Yyeks5rTkDwgaJpZM4KwiQi> .

esoterik

there's a just few small changes to be made! I know you had questions about test coverage, and I think it's fine, but I have to run right now so I will review that more thoroughly tonight.

esoterik · 2017-01-18T16:52:28Z

+                                  update_p: blackout_params)
+    result = updater.update!
+
+    if result[:error]


I think that you don't need to pull out these methods here (though I realize you were following my example, apologies!)

esoterik · 2017-01-18T17:01:52Z

@@ -0,0 +1,37 @@
+class BlackoutUpdater
+  # Service Object to update blackouts in the blackouts controller
+  def initialize(blackout:, update_p:)


update_p should be renamed to either params, new_attrs, or update_params, take your pick!

esoterik · 2017-01-18T17:07:14Z

+    conflicts.each do |conflict|
+      message += "#{conflict.md_link}, "
+    end
+    return message[0, message.length - 2]\


why is this array index necessary?

esoterik · 2017-01-18T17:07:54Z

+    list_conflicts(conflicts)
+  end
+
+  def list_conflicts(conflicts)


rename to error_string

esoterik · 2017-01-18T17:09:14Z

+    end
+    return message[0, message.length - 2]\
+           + '. Please update their due dates and try again.'
+  end


I think the processing of the conflicts should be pulled out to a separate method so that this method reads: "The following reservation(s) will be unable to be returned #{conflicts_string}. Please update their due dates and try again."

esoterik · 2017-01-18T17:13:41Z

+        before { put :update, id: FactoryGirl.create(:blackout),
+                 blackout: attributes }
+
+        it { is_expected.to set_flash }


this should specify set_flash[:error]

esoterik · 2017-01-18T17:15:01Z

+        it 'should not save the blackout' do
+          expect(attributes).not_to be_nil
+          expect(assigns(:blackout)).not_to be_nil
+          expect(assigns(:blackout)[:notice]).not_to eq(attributes[:notice])


this should be split up into separate tests / refactored to only have one expect

esoterik · 2017-01-18T17:19:57Z

+describe BlackoutUpdater do
+  describe 'update!' do
+    let!(:blackout) {instance_spy(Blackout, :update_attributes => nil)}
+    let!(:cf) {double("conflict", :md_link => "Test")}


it's not as DRY, but it's preferred to not use let! and just declare the variables you need inside your tests. This way, every individual test is readable and doesn't depend on things declared elsewhere.

esoterik · 2017-01-18T17:20:11Z

+                                                    :active).and_return([])
+        updater = BlackoutUpdater.new(blackout: blackout, update_p: {})
+        @results = updater.update!
+      end


same with this before block.

esoterik · 2017-01-18T17:20:28Z

+                                                      :active).and_return([cf])
+          updater = BlackoutUpdater.new(blackout: blackout, update_p: {})
+          @results = updater.update!
+        end


and this before too

yongjie-lin · 2017-01-21T21:12:05Z

I've made the requested changes, please review!

Edit: just realized I forgot to implement the conditional

esoterik · 2017-01-22T21:23:08Z

before I review, I just noticed that you have a merge commit in your branch -- you should revert it and rebase your branch on top of master instead (git rebase -i master)

esoterik

almost there!

esoterik · 2017-01-22T21:25:12Z

+      context 'with conflicting blackout' do
+        it 'does not update blackout' do
+          blackout = instance_spy(Blackout, update_attributes: nil)
+          cf = double('conflict', md_link: 'Test')


this should be either an instance_spy('Reservation') or a ReservationMock.new, not just a plain double

esoterik · 2017-01-22T21:25:59Z

+      end
+    end
+    context 'with errors' do
+      context 'with conflicting blackout' do


this should be with a conflicting reservation, right?

esoterik · 2017-01-22T21:26:32Z

+          results = updater.update!
+          expect(results[:error]).to eq('The following reservation(s) will be'\
+            ' unable to be returned: Test. Please update their due dates and '\
+            'try again.')


instead of testing the whole string, test if it includes the reservation's md_link

yongjie-lin · 2017-01-29T00:38:25Z

Ready for review!

esoterik

mostly small changes, but I found one bug in the error handling

esoterik · 2017-01-30T20:55:29Z

+
+  def error
+    return 'Invalid blackout parameters. Please try again.' unless
+      @blackout.validate


this doesn't work -- this is checking that the blackout is valid before you try to update it

also, this wasn't caught by any tests -- there should be a controller-level test that checks the response to attempting to update a blackout with invalid attributes

esoterik · 2017-01-30T20:56:02Z

+    return 'Invalid blackout parameters. Please try again.' unless
+      @blackout.validate
+    conflicts
+    return unless @conflicts.any?


you can get rid of the line above and just have return unless conflicts.any?

esoterik · 2017-01-30T20:59:06Z

+
+  def update_handler
+    blackout.update_attributes(params)
+    { result: 'Blackout was successfully updated.', error: nil }


in order to check for a successful update and return error messages if necessary, this should read

if blackout.update_attributes(params) return { success hash } else return { result: nil, error: blackout.errors.full_messages } end

esoterik · 2017-01-30T21:00:54Z

+        it 'does not update blackout' do
+          blackout = instance_spy(Blackout, update_attributes: nil,
+                                            validate: true)
+          cf = instance_spy('conflict', md_link: 'Dummy conflict')


this should be instance_spy('Reservation', ...) not instance_spy('conflict', ...)

esoterik · 2017-01-30T21:01:12Z

+        it 'does not return a result' do
+          blackout = instance_spy(Blackout, update_attributes: nil,
+                                            validate: true)
+          cf = instance_spy('conflict', md_link: 'Dummy conflict')


this should be instance_spy('Reservation', ...) not instance_spy('conflict', ...)

yongjie-lin · 2017-02-03T21:35:38Z

Ready for review!

esoterik

just a couple of tests to get rid of, then this is ready to squash and merge

esoterik · 2017-02-04T18:13:51Z

+        it { is_expected.to render_template(:edit) }
+        it 'should have attributes' do
+          expect(attributes).not_to be_nil
+        end


this test is unnecessary -- you're testing that the shared example wasn't given nil, which you have control over

esoterik · 2017-02-04T18:14:30Z

+        end
+        it 'should have a blackout to update' do
+          expect(assigns(:blackout)).not_to be_nil
+        end


this is also unnecessary

closes #1645 - change app/controllers/blackouts_controller.rb - add lib/blackout_updater.rb - change spec/controllers/blackouts_controller_spec.rb - add spec/lib/blackout_updater_spec.rb - add service object for updating blackouts - add checking of conflicting reservation when updating blackouts

esoterik · 2017-02-04T20:57:28Z

@yongjie-lin don't forget to add this to the changelog (branch v630_changelog)

orenyk added the 0 - needs review label Nov 12, 2016

esoterik suggested changes Nov 20, 2016

View reviewed changes

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

esoterik changed the title ~~1645 blackout conflict checking~~ [1645] blackout conflict checking Nov 20, 2016

esoterik self-assigned this Nov 22, 2016

esoterik suggested changes Jan 18, 2017

View reviewed changes

esoterik removed the 1 - working label Jan 18, 2017

yongjie-lin added the 0 - needs review label Jan 21, 2017

esoterik suggested changes Jan 22, 2017

View reviewed changes

esoterik removed the 0 - needs review label Jan 22, 2017

yongjie-lin force-pushed the 1645_blackout_conflict_checking branch 2 times, most recently from 9f2161f to b67cf26 Compare January 29, 2017 00:30

yongjie-lin added the 0 - needs review label Jan 29, 2017

esoterik suggested changes Jan 30, 2017

View reviewed changes

esoterik removed the 0 - needs review label Jan 30, 2017

yongjie-lin force-pushed the 1645_blackout_conflict_checking branch from 0e1e994 to b67cf26 Compare February 3, 2017 21:22

yongjie-lin added the 0 - needs review label Feb 3, 2017

esoterik suggested changes Feb 4, 2017

View reviewed changes

esoterik removed the 0 - needs review label Feb 4, 2017

yongjie-lin force-pushed the 1645_blackout_conflict_checking branch from cbeb3fc to 07a3be5 Compare February 4, 2017 19:23

yongjie-lin force-pushed the 1645_blackout_conflict_checking branch from 07a3be5 to 93608aa Compare February 4, 2017 19:26

esoterik approved these changes Feb 4, 2017

View reviewed changes

esoterik merged commit ac2ade3 into master Feb 4, 2017

esoterik deleted the 1645_blackout_conflict_checking branch February 4, 2017 20:57

Uh oh!

Conversation

yongjie-lin commented Nov 12, 2016

Uh oh!

esoterik 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!

esoterik commented Jan 18, 2017

Uh oh!

yongjie-lin commented Jan 18, 2017 via email

Uh oh!

esoterik 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!

yongjie-lin commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esoterik commented Jan 22, 2017

Uh oh!

esoterik 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!

yongjie-lin commented Jan 29, 2017

Uh oh!

esoterik 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!

yongjie-lin commented Feb 3, 2017

Uh oh!

esoterik left a comment

Choose a reason for hiding this comment

Uh oh!

yongjie-lin commented Jan 21, 2017 •

edited

Loading