Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugs/aperta 12001 cancel passive events on rr submit #3820

Merged
2 changes: 1 addition & 1 deletion app/controllers/scheduled_events_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class ScheduledEventsController < ApplicationController
before_action :authenticate_user!
respond_to :json

def update_state
def update
scheduled_event = ScheduledEvent.find(params[:id])
scheduled_event.switch_on! if params[:state] == 'active'
scheduled_event.switch_off! if params[:state] == 'passive'
Expand Down
2 changes: 1 addition & 1 deletion app/models/reviewer_report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,6 @@ def thank_reviewer
end

def cancel_reminders
scheduled_events.active.map(&:cancel!)
scheduled_events.cancelable.each(&:cancel!)
Copy link
Contributor

Choose a reason for hiding this comment

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

props on spelling cancelable correctly. I definitely did not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

end
end
3 changes: 2 additions & 1 deletion app/models/scheduled_event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class ScheduledEvent < ActiveRecord::Base
scope :passive, -> { where(state: 'passive') }
scope :due_to_trigger, -> { active.where('dispatch_at < ?', DateTime.now.in_time_zone) }
scope :serviceable, -> { where(state: ['passive', 'active', 'completed', 'inactive']) }
scope :cancelable, -> { where(state: ['passive', 'active']) }

before_save :deactivate, if: :should_deactivate?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grepped pretty hard, I think this event is only used internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you

before_save :reactivate, if: :should_reactivate?
Expand Down Expand Up @@ -68,7 +69,7 @@ def finished?
end

event(:cancel) do
transitions from: :active, to: :canceled
transitions from: [:active, :passive], to: :canceled
end
end

Expand Down
12 changes: 1 addition & 11 deletions client/app/models/scheduled-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,5 @@ export default DS.Model.extend({
errored: Ember.computed.equal('state', 'errored'),
inactive: Ember.computed.equal('state', 'inactive'),
active: Ember.computed.equal('state', 'active'),
canceled: Ember.computed.equal('state', 'canceled'),

restless: Ember.inject.service(),

updateState: function(newState) {
const url = `/api/scheduled_events/${this.get('id')}/update_state`;
return this.get('restless').put(url, {state: newState}).then(() => {
this.set('state', newState);
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

💯

canceled: Ember.computed.equal('state', 'canceled')
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ export default Ember.Component.extend({
actions: {
changeEventState(newVal) {
const state = newVal ? 'active' : 'passive';
this.get('event').updateState(state);
this.set('event.state', state);
this.get('event').save();
Copy link
Contributor

Choose a reason for hiding this comment

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

yay, we're using ember now :)

}
}
});
3 changes: 1 addition & 2 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,7 @@

resources :feature_flags, only: [:index, :update]

put 'scheduled_events/:id/update_state',
to: 'scheduled_events#update_state'
resources :scheduled_events, only: [:update]
end

get '/invitations/:token/accept',
Expand Down
8 changes: 4 additions & 4 deletions spec/controllers/scheduled_events_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
describe ScheduledEventsController do
let(:user) { FactoryGirl.create(:user) }

describe "PUT /update_state (passive)" do
describe "PUT /update (passive)" do
let(:scheduled_event_active) { FactoryGirl.create(:scheduled_event, dispatch_at: DateTime.now.utc + 2.days) }
subject(:do_request) { put :update_state, id: scheduled_event_active.id, state: 'passive', format: :json }
subject(:do_request) { put :update, id: scheduled_event_active.id, state: 'passive', format: :json }

it_behaves_like 'an unauthenticated json request'

Expand All @@ -21,9 +21,9 @@
end
end

describe "PUT /update_state (active)" do
describe "PUT /update (active)" do
let(:scheduled_event_passive) { FactoryGirl.create(:scheduled_event, :passive, dispatch_at: DateTime.now.utc + 2.days) }
subject(:do_request) { get :update_state, id: scheduled_event_passive.id, state: 'active', format: :json }
subject(:do_request) { get :update, id: scheduled_event_passive.id, state: 'active', format: :json }

it_behaves_like 'an unauthenticated json request'

Expand Down
21 changes: 21 additions & 0 deletions spec/models/reviewer_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,25 @@ def add_invitation(state)
expect { subject.set_due_datetime }.to change { subject.scheduled_events.count }.by(3)
end
end

describe '#cancel_reminders' do
before do
FactoryGirl.create(:feature_flag, name: 'REVIEW_DUE_AT')
FactoryGirl.create(:feature_flag, name: 'REVIEW_DUE_DATE')
FactoryGirl.create :review_duration_period_setting_template
end

it 'cancels all events with passive or active state' do
subject.set_due_datetime # makes 3 scheduled events with active state
subject.scheduled_events.first.switch_off! # passive state
subject.scheduled_events.last.cancel! # cancel state
cancelable = subject.scheduled_events.select(&:may_cancel?) # omg it compares object ids and not row id.
allow(subject.scheduled_events).to receive(:cancelable).and_return(cancelable)
Copy link
Contributor

@JackLaBarba JackLaBarba Dec 4, 2017

Choose a reason for hiding this comment

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

What's the motivation behind stubbing this instead of using the real scope? If it's so you can test the scope in a non-tautological way it's probably worth breaking out a separate test for the scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@egh watched me struggle with this, basically when cancelable gets called, it returns an array of objects that while containing the same active record objects, they have different ruby object ids, which screws with rspec. so this ensures they have the right object ids.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah, I just talked to @egh. I understand now that this was necessary for the subsequent message expectations to be set up properly (otherwise they're set up on different objects than those used inside of cancel_reminders).

However, if you ditch those expectations (my comment below), you can also ditch this stubbing and make the test a lot more clear.

subject.scheduled_events.each do |event|
count = event.canceled? ? 0 : 1
expect(event).to receive(:cancel!).exactly(count).times
Copy link
Contributor

@JackLaBarba JackLaBarba Dec 4, 2017

Choose a reason for hiding this comment

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

WDYTA setting up expectations about the ending data state rather than setting up message expectations? I think a test that essentially said "all three scheduled events should be cancelled" rather than "some number of models should have the cancel! messaged passed to them" is easier to follow, and more resilient to implementation changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my old age, I've started to shy away from test mocks and stubs unless there's a really good reason to use them (test speed, or an external service). I find that it's nice to get the extra coverage of integrations where you're able.

I'm going to paste this article without reading it: https://blog.kentcdodds.com/write-tests-not-too-many-mostly-integration-5e8c7fff591c hopefully it supports my point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/spec/models/reviewer_report_spec.rb b/spec/models/reviewer_report_spec.rb
index ff46118346..79079bd9d0 100644
--- a/spec/models/reviewer_report_spec.rb
+++ b/spec/models/reviewer_report_spec.rb
@@ -173,13 +173,8 @@ describe ReviewerReport do
       subject.set_due_datetime # makes 3 scheduled events with active state
       subject.scheduled_events.first.switch_off! # passive state
       subject.scheduled_events.last.cancel! # cancel state
-      cancelable = subject.scheduled_events.select(&:may_cancel?) # omg it compares object ids and not row id.
-      allow(subject.scheduled_events).to receive(:cancelable).and_return(cancelable)
-      subject.scheduled_events.each do |event|
-        count = event.canceled? ? 0 : 1
-        expect(event).to receive(:cancel!).exactly(count).times
-      end
       subject.send(:cancel_reminders)
+      expect(subject.scheduled_events.all?(&:canceled?)).to be(true)
     end
   end
 end

does that look good?

Copy link
Contributor

Choose a reason for hiding this comment

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

love it. Thanks ben!

end
subject.send(:cancel_reminders)
end
end
end