Merge pull request #2636 from getsentry/GH-2603

Fix comment HTTP updates canceled via race condition (fixes #2603)

Author: benvinegar

src/sentry/static/sentry/app/components/activity/noteContainer.jsx

@@ -1,17 +1,9 @@
 import React from 'react';
-import ApiMixin from '../../mixins/apiMixin';
-import IndicatorStore from '../../stores/indicatorStore';
-import GroupStore from '../../stores/groupStore';
 
 import Note from './note';
 import NoteInput from './noteInput';
-import {t} from '../../locale';
 
 const NoteContainer = React.createClass({
-  mixins: [
-    ApiMixin
-  ],
-
   getInitialState() {
     return {
       editing: false
@@ -27,27 +19,7 @@ const NoteContainer = React.createClass({
   },
 
   onDelete() {
-    let {group, item} = this.props;
-    // Optimistically remove from UI
-    let index = GroupStore.removeActivity(group.id, item.id);
-    if (index === -1) {
-        // I dunno, the id wasn't found in the GroupStore
-        return;
-    }
-
-    let loadingIndicator = IndicatorStore.add(t('Removing comment..'));
-
-    this.api.request('/issues/' + group.id + '/comments/' + item.id + '/' , {
-      method: 'DELETE',
-      error: (error) => {
-        // TODO(mattrobenolt): Show an actual error that this failed,
-        // but just bring it back in place for now
-        GroupStore.addActivity(group.id, item, index);
-      },
-      complete: () => {
-        IndicatorStore.remove(loadingIndicator);
-      }
-    });
+    this.props.onDelete(this.props.item);
   },
 
   render() {

src/sentry/static/sentry/app/views/groupActivity/index.jsx

@@ -1,22 +1,29 @@
 import React from 'react';
 
+import ApiMixin from '../../mixins/apiMixin';
+import GroupState from '../../mixins/groupState';
+
 import Duration from '../../components/duration';
 import Gravatar from '../../components/gravatar';
-import GroupState from '../../mixins/groupState';
-import MemberListStore from '../../stores/memberListStore';
 import TimeSince from '../../components/timeSince';
-import ConfigStore from '../../stores/configStore';
 import Version from '../../components/version';
-
 import NoteContainer from '../../components/activity/noteContainer';
 import NoteInput from '../../components/activity/noteInput';
-import {t, tn} from '../../locale';
 
+import ConfigStore from '../../stores/configStore';
+import GroupStore from '../../stores/groupStore';
+import IndicatorStore from '../../stores/indicatorStore';
+import MemberListStore from '../../stores/memberListStore';
+
+import {t, tn} from '../../locale';
 
 const GroupActivity = React.createClass({
   // TODO(dcramer): only re-render on group/activity change
 
-  mixins: [GroupState],
+  mixins: [
+    GroupState,
+    ApiMixin
+  ],
 
   formatActivity(author, item, params) {
     let data = item.data;
@@ -98,6 +105,31 @@ const GroupActivity = React.createClass({
     }
   },
 
+  onNoteDelete(item) {
+    let {group} = this.props;
+
+    // Optimistically remove from UI
+    let index = GroupStore.removeActivity(group.id, item.id);
+    if (index === -1) {
+      // I dunno, the id wasn't found in the GroupStore
+      return;
+    }
+
+    let loadingIndicator = IndicatorStore.add(t('Removing comment..'));
+
+    this.api.request('/issues/' + group.id + '/comments/' + item.id + '/' , {
+      method: 'DELETE',
+      error: (error) => {
+        // TODO(mattrobenolt): Show an actual error that this failed,
+        // but just bring it back in place for now
+        GroupStore.addActivity(group.id, item, index);
+      },
+      complete: () => {
+        IndicatorStore.remove(loadingIndicator);
+      }
+    });
+  },
+
   render() {
     let group = this.props.group;
     let me = ConfigStore.get('user');
@@ -114,7 +146,7 @@ const GroupActivity = React.createClass({
 
       if (item.type === 'note') {
         return (
-          <NoteContainer group={group} item={item} key={itemIdx} author={author} />
+          <NoteContainer group={group} item={item} key={itemIdx} author={author} onDelete={this.onNoteDelete} />
         );
       } else {
         return (

tests/js/spec/views/groupActivity/index.spec.jsx

@@ -0,0 +1,66 @@
+import React from 'react';
+import {shallow} from 'enzyme';
+
+import GroupActivity from 'app/views/groupActivity';
+import NoteInput from 'app/components/activity/noteInput';
+import ConfigStore from 'app/stores/configStore';
+import GroupStore from 'app/stores/groupStore';
+
+describe('GroupActivity', function() {
+  beforeEach(function() {
+    this.sandbox = sinon.sandbox.create();
+
+    this.sandbox.stub(ConfigStore, 'get').withArgs('user').returns({});
+  });
+
+  afterEach(function () {
+    this.sandbox.restore();
+  });
+
+  it('renders a NoteInput', function () {
+    let wrapper = shallow(<GroupActivity group={{id: '1337', activity: []}}/>, {
+      context: {
+        group: {id: '1337'},
+        project: {id: 'foo'},
+        team: {id: '1'},
+        organization: {id:'bar'}
+      }
+    });
+    expect(wrapper.find(NoteInput)).to.have.length(1);
+  });
+
+  describe('onNoteDelete()', function () {
+    beforeEach(function () {
+      this.instance = shallow(<GroupActivity group={{id: '1337', activity: []}}/>, {
+        context: {
+          group: {id: '1337'},
+          project: {id: 'foo'},
+          team: {id: '1'},
+          organization: {id:'bar'}
+        }
+      }).instance();
+    });
+
+    it('should do nothing if not present in GroupStore', function () {
+      let instance = this.instance;
+
+      this.sandbox.stub(GroupStore, 'removeActivity').returns(-1); // not found
+      let request = this.sandbox.stub(instance.api, 'request');
+
+      instance.onNoteDelete({id: 1});
+      expect(request.calledOnce).to.not.be.ok;
+    });
+
+    it('should remove remove the item from the GroupStore make a DELETE API request', function () {
+      let instance = this.instance;
+
+      this.sandbox.stub(GroupStore, 'removeActivity').returns(1);
+
+      let request = this.sandbox.stub(instance.api, 'request');
+      instance.onNoteDelete({id: 1});
+      expect(request.calledOnce).to.be.ok;
+      expect(request.getCall(0).args[0]).to.equal('/issues/1337/comments/1/');
+      expect(request.getCall(0).args[1]).to.have.property('method', 'DELETE');
+    });
+  });
+});