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

benjaminkreen · 2017-12-01T22:30:10Z

JIRA issue: https://jira.plos.org/jira/browse/APERTA-12001

What this PR does:

Submitting a reviewer report should cancel scheduled events in the passive state as well.

Special instructions for Review or PO:

See ticket for issue repro instructions.

Code Review Tasks:

Author tasks (delete tasks that don't apply to your PR, this list should be finished before code review):

I have ensured that the Heroku Review App has successfully deployed and is ready for PO UAT.

Reviewer tasks (these should be checked or somehow noted before passing on to PO):

I read through the JIRA ticket's AC before doing the rest of the review
I ran the code (in the review environment or locally). I agree the running code fulfills the Acceptance Criteria as stated on the JIRA ticket
I read the code; it looks good
I have found the tests to be sufficient for both positive and negative test cases

benjaminkreen · 2017-12-02T00:20:12Z

client/app/models/scheduled-event.js

@@ -18,8 +18,9 @@ export default DS.Model.extend({

  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);
+    return this.get('restless').put(url, {state: newState}).then((data) => {


the UI wasn't refreshing because the object was dirty and not reloading.

Just a drive-by comment/question. I could not tell from the ember code: does pushing the payload update the state? If not, is the this.set still needed?

indeed this.store.pushPayload will update this ember object's state, hence I removed the this.set('state', newState);

What you did seems like an improvement to me, but I get the willies from this type of un-restful stuff in ember. Every call to 'restless' seems like tech debt to me, especially since the semantics are almost identical to a model update. Something like this would make me feel a lot better:

updateState(newState) { this.set('state', 'newState'}) return this.save(); }

which is trivial to the point of being unnecessary (hence an indication that we're actually using ember data properly.

I wont insist that you change it, but just wanted to express my reservations about the pattern.

agreed, I'm not sure why the scheduled_events_controller.rb was setup that way... I'd be down to refactor that in an empowerment ticket.

Just a note that my question about whether the state was updated would have been unnecessary with this implementation.

okay, i've fixed this, gonna push.

JackLaBarba · 2017-12-04T17:56:50Z

app/models/reviewer_report.rb

@@ -170,6 +170,6 @@ def thank_reviewer
  end

  def cancel_reminders
-    scheduled_events.active.map(&:cancel!)
+    scheduled_events.cancelable.each(&:cancel!)


props on spelling cancelable correctly. I definitely did not.

machty/ember-concurrency#41 my fav

JackLaBarba · 2017-12-04T19:11:37Z

spec/models/reviewer_report_spec.rb

+      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)


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.

@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.

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.

JackLaBarba · 2017-12-04T19:25:40Z

spec/models/reviewer_report_spec.rb

+      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


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.

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.

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?

love it. Thanks ben!

JackLaBarba · 2017-12-04T19:34:12Z

client/app/pods/components/scheduled-event-status/component.js

@@ -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();


yay, we're using ember now :)

JackLaBarba

Great changes here. In particular, thanks for the changes to make updating scheduled_events more restful. 👍

benjaminkreen · 2017-12-06T17:33:30Z

[60] pry(main)> ReviewerReport.joins(:due_datetime).where(state: ['submitted', 'invitation_not_accepted']).select { |x| x.scheduled_events.any?(&:may_cancel?) }.count
=> 1

So from my somewhat recent prod snapshot, there's only one outstanding reviewer report that would need migrating. If that's still the case, i'd just sweep it under the rug, it will go away once that decision is made (I think).

benjaminkreen · 2017-12-06T17:42:54Z

app/models/scheduled_event.rb


-  before_save :deactivate, if: :should_deactivate?


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

I believe you

egh · 2017-12-06T22:20:49Z

@benjaminkreen "on hold" or "merge when green"? :)

benjaminkreen · 2017-12-06T22:23:26Z

its good from a PO stance, I just wanted to run my last commit by @slimeate

JackLaBarba · 2017-12-06T22:52:01Z

app/models/scheduled_event.rb

@@ -38,12 +41,13 @@ def finished?
    state :errored
    state :passive
    state :canceled # Canceled automatically after a qualifying event, e.g., a review is submitted
+    state :deactivated

    event(:reactivate) do
      transitions from: [:completed, :inactive], to: :active


We'll probably want to add the ability to reactivate disabled events. Ensure that we haven't reintroduced APERTA-11985.

Btw, check out my comment here: https://jira.plos.org/jira/browse/APERTA-11985?focusedCommentId=193071&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-193071 seems related to the issue you're dealing with now. Specifically, it may be possible to reactivate events to have the correct toggle state now that we have the new deactivated state.

Does this need addressing before we merge this?

we chatted offline, its resolved :)

JackLaBarba · 2017-12-06T22:57:25Z

spec/models/reviewer_report_spec.rb

+      subject.scheduled_events.first.switch_off! # passive state
+      subject.scheduled_events.last.cancel! # cancel state
+      subject.send(:cancel_reminders)
+      expect(subject.scheduled_events.none?(&:may_cancel?)).to be(true)


might be good to be a bit more specific here--asserting the actual data states you expect, or asserting that the number of events which may_cancel? changes before and after cancel_reminders. It's possible that an error in the definition of may_cancel? would cause this test to pass when it actually shouldn't. For instance, if none of the events may_cancel? in the initial conditions, cancel_reminders would essentially no-op and this assertion would still pass.

diff --git a/spec/models/reviewer_report_spec.rb b/spec/models/reviewer_report_spec.rb index d7f5a99e6b..2dc8747ab0 100644 --- a/spec/models/reviewer_report_spec.rb +++ b/spec/models/reviewer_report_spec.rb @@ -173,8 +173,9 @@ 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 - subject.send(:cancel_reminders) - expect(subject.scheduled_events.none?(&:may_cancel?)).to be(true) + old_states = subject.scheduled_events.map(&:state) + new_states = old_states.map { |x| x == 'passive' ? 'deactivated' : 'canceled' } + expect { subject.send(:cancel_reminders) }.to change { subject.scheduled_events.reload.map(&:state) }.from(old_states).to(new_states) end end

APERTA-12001 cancel passive events on rr submit

fc90dd2

benjaminkreen added core wip labels Dec 1, 2017

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 1, 2017 22:30 Inactive

benjaminkreen force-pushed the bugs/APERTA-12001-cancel-passive-events-on-rr-submit branch from 601dd39 to f59ab5d Compare December 1, 2017 22:34

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 1, 2017 22:35 Inactive

APERTA-12001 add test

97ec21b

benjaminkreen force-pushed the bugs/APERTA-12001-cancel-passive-events-on-rr-submit branch from f59ab5d to 97ec21b Compare December 1, 2017 22:53

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 1, 2017 22:53 Inactive

benjaminkreen added 2 commits December 1, 2017 15:19

APERTA-12001 Fix scheduled-event ember model

be7c1fc

APERTA-12001 update spec condition

7bea1d5

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 2, 2017 00:01 Inactive

APERTA-12001 fix test

dd03fbd

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 2, 2017 00:18 Inactive

benjaminkreen commented Dec 2, 2017

View reviewed changes

benjaminkreen added needs review and removed wip labels Dec 2, 2017

JackLaBarba self-assigned this Dec 4, 2017

JackLaBarba added in review and removed needs review labels Dec 4, 2017

JackLaBarba reviewed Dec 4, 2017

View reviewed changes

APERTA-12001 Refactor scheduled events to be RESTful

4d47299

plos-ci-agent had a problem deploying to plos-ciagent-pr-3820 December 4, 2017 19:10 Failure

JackLaBarba reviewed Dec 4, 2017

View reviewed changes

plos-ci-agent had a problem deploying to plos-ciagent-pr-3820 December 4, 2017 19:11 Failure

plos-ci-agent had a problem deploying to plos-ciagent-pr-3820 December 4, 2017 19:13 Failure

plos-ci-agent had a problem deploying to plos-ciagent-pr-3820 December 4, 2017 19:16 Failure

plos-ci-agent requested a deployment to plos-ciagent-pr-3820 December 4, 2017 19:22 Pending

JackLaBarba reviewed Dec 4, 2017

View reviewed changes

plos-ci-agent had a problem deploying to plos-ciagent-pr-3820 December 4, 2017 19:35 Failure

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 4, 2017 19:59 Inactive

APERTA-12001 Update tests

923ea66

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 4, 2017 20:38 Inactive

JackLaBarba approved these changes Dec 4, 2017

View reviewed changes

JackLaBarba added needs PO acceptance and removed in review labels Dec 4, 2017

APERTA-12001 fix object spacing bug

5e96f3f

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 4, 2017 22:19 Inactive

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 5, 2017 23:44 Inactive

benjaminkreen force-pushed the bugs/APERTA-12001-cancel-passive-events-on-rr-submit branch from 4f95a60 to 518f0ea Compare December 5, 2017 23:48

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 5, 2017 23:49 Inactive

APERTA-12001 add new state to scheduled events

0b33211

benjaminkreen force-pushed the bugs/APERTA-12001-cancel-passive-events-on-rr-submit branch from 518f0ea to 0b33211 Compare December 6, 2017 00:08

plos-ci-agent temporarily deployed to plos-ciagent-pr-3820 December 6, 2017 00:08 Inactive

benjaminkreen commented Dec 6, 2017

View reviewed changes

benjaminkreen added the on hold label Dec 6, 2017

ppolischukplos added merge when green and removed needs PO acceptance labels Dec 6, 2017

JackLaBarba reviewed Dec 6, 2017

View reviewed changes

APERTA-12001 update test

e771820

plos-ci-agent deployed to plos-ciagent-pr-3820 December 7, 2017 00:46 View deployment

benjaminkreen removed the on hold label Dec 7, 2017

JackLaBarba merged commit bbe3aa4 into master Dec 7, 2017

JackLaBarba deleted the bugs/APERTA-12001-cancel-passive-events-on-rr-submit branch December 7, 2017 17:23

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benjaminkreen commented Dec 1, 2017 •

edited by JackLaBarba

Loading

benjaminkreen Dec 2, 2017

jhaungs Dec 4, 2017

benjaminkreen Dec 4, 2017

JackLaBarba Dec 4, 2017

benjaminkreen Dec 4, 2017

jhaungs Dec 4, 2017

benjaminkreen Dec 4, 2017

JackLaBarba Dec 4, 2017

benjaminkreen Dec 4, 2017

JackLaBarba Dec 4, 2017 •

edited

Loading

benjaminkreen Dec 4, 2017

JackLaBarba Dec 4, 2017

JackLaBarba Dec 4, 2017 •

edited

Loading

JackLaBarba Dec 4, 2017

benjaminkreen Dec 4, 2017

JackLaBarba Dec 4, 2017

JackLaBarba Dec 4, 2017

JackLaBarba left a comment

benjaminkreen commented Dec 6, 2017

benjaminkreen Dec 6, 2017

JackLaBarba Dec 6, 2017

egh commented Dec 6, 2017

benjaminkreen commented Dec 6, 2017

JackLaBarba Dec 6, 2017

egh Dec 7, 2017

benjaminkreen Dec 7, 2017

JackLaBarba Dec 6, 2017

benjaminkreen Dec 7, 2017

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

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

Conversation

benjaminkreen commented Dec 1, 2017 • edited by JackLaBarba Loading

What this PR does:

Special instructions for Review or PO:

Code Review Tasks:

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

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

Choose a reason for hiding this comment

JackLaBarba Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackLaBarba Dec 4, 2017 • edited Loading

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

Choose a reason for hiding this comment

JackLaBarba left a comment

Choose a reason for hiding this comment

benjaminkreen commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egh commented Dec 6, 2017

benjaminkreen commented Dec 6, 2017

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

Choose a reason for hiding this comment

benjaminkreen commented Dec 1, 2017 •

edited by JackLaBarba

Loading

JackLaBarba Dec 4, 2017 •

edited

Loading

JackLaBarba Dec 4, 2017 •

edited

Loading