Stop using asynchronous requests on the edit form #3147

jcoyne · 2017-03-14T21:49:31Z

These requests caused the lock to be lost, so the page couldn't be saved
(#3127). Completely removed the "Add parent relationships" section as
this was not locking the parent. It was unclear how we should
simultaneously update multiple objects and what should happen if the
lock for one of them was stale.

This should also fix #3133 as we are no longer passing the
ordered_member_ids as an attribute to the work actor for update. Instead
we are using a custom attribute and only making adds and deletions
instead of rewriting the whole set.

Fixes #3133
Fixes #3127

houndci-bot · 2017-03-14T21:49:41Z

spec/javascripts/relationships_control_spec.js.coffee

+
+    it 'creates a row when something is selected', ->
+      spyOn(target.input, 'val').and.returnValue('123');
+      spyOn(target, 'searchData').and.returnValue({ id: '123', text: 'foo bar' });


Line exceeds maximum allowed length. Length is 82, max is 80.
Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:41Z

spec/javascripts/relationships_control_spec.js.coffee

+      expect(target.errors).toEqual([ 'ID cannot be empty.' ])
+
+    it 'creates a row when something is selected', ->
+      spyOn(target.input, 'val').and.returnValue('123');


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:41Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:41Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:41Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:41Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:42Z

spec/javascripts/relationships_control_spec.js.coffee

+
+    it 'creates a row when something is selected', ->
+      spyOn(target.input, 'val').and.returnValue('123');
+      spyOn(target, 'searchData').and.returnValue({ id: '123', text: 'foo bar' });


Line exceeds maximum allowed length. Length is 82, max is 80.
Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:42Z

spec/javascripts/relationships_control_spec.js.coffee

+      expect(target.errors).toEqual([ 'ID cannot be empty.' ])
+
+    it 'creates a row when something is selected', ->
+      spyOn(target.input, 'val').and.returnValue('123');


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:42Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:42Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:43Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:49:43Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:51:26Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-14T21:51:27Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-15T00:48:03Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-15T00:48:03Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-15T00:48:03Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

houndci-bot · 2017-03-15T00:48:03Z

spec/javascripts/autocomplete_spec.js.coffee

-
-        # verify that the ajax call was made
-        expect(@spy_on_json).toHaveBeenCalled()
+        expect(target.select2('open')).toBe(true);


Line contains a trailing semicolon.

mjgiarlo

Largely looks good to me. Left a few suggestions/questions. Would like someone else to review in depth.

mjgiarlo · 2017-03-14T22:24:26Z

app/actors/sufia/actors/attach_members_actor.rb

+        end
+
+        def ability
+          ::Ability.new(user)


Should this be memoized to avoid having to create a new Ability instance per call to #add?

Sure. Although it won't be noticeable compared to the time required to write to solr.

mjgiarlo · 2017-03-14T22:26:22Z

app/actors/sufia/actors/attach_members_actor.rb

+        end
+
+        # Remove the object from the members set and the ordered members list
+        def remove(id)


Should this method also do an ability check?

No, the controller is doing an ability check for this object before we get here.

mjgiarlo · 2017-03-14T22:30:47Z

app/views/curation_concerns/base/_find_work_widget.html.erb

- %>
+<%= text_field_tag "find_#{name}",
+                   '',
+                   placeholder: 'Search for a work',


Should this be in i18n?

These requests caused the lock to be lost, so the page couldn't be saved (#3127). Completely removed the "Add parent relationships" section as this was not locking the parent. It was unclear how we should simultaneously update multiple objects and what should happen if the lock for one of them was stale. This should also fix #3133 as we are no longer passing the ordered_member_ids as an attribute to the work actor for update. Instead we are using a custom attribute and only making adds and deletions instead of rewriting the whole set. Fixes #3133 Fixes #3127

dlpierce

I ran through the forms locally and it worked well. 👍

jcoyne added ready for review in progress and removed ready for review labels Mar 14, 2017

houndci-bot reviewed Mar 14, 2017

View reviewed changes

jcoyne force-pushed the remove_ajax branch from fca4aef to 8e6e916 Compare March 14, 2017 21:51

houndci-bot reviewed Mar 14, 2017

View reviewed changes

jcoyne force-pushed the remove_ajax branch from 8e6e916 to 0a751fd Compare March 15, 2017 00:47

houndci-bot reviewed Mar 15, 2017

View reviewed changes

jcoyne force-pushed the remove_ajax branch 2 times, most recently from dc74034 to 8e41a1a Compare March 15, 2017 02:45

mjgiarlo reviewed Mar 15, 2017

View reviewed changes

jcoyne force-pushed the remove_ajax branch from 8e41a1a to 6773615 Compare March 15, 2017 18:29

jcoyne force-pushed the remove_ajax branch from 6773615 to 0f94d45 Compare March 15, 2017 18:33

dlpierce approved these changes Mar 16, 2017

View reviewed changes

hackartisan merged commit e6a2c12 into master Mar 17, 2017

hackartisan removed the in progress label Mar 17, 2017

hackartisan deleted the remove_ajax branch March 17, 2017 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using asynchronous requests on the edit form #3147

Stop using asynchronous requests on the edit form #3147

jcoyne commented Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 14, 2017

houndci-bot Mar 15, 2017

houndci-bot Mar 15, 2017

houndci-bot Mar 15, 2017

houndci-bot Mar 15, 2017

mjgiarlo left a comment

mjgiarlo Mar 14, 2017

jcoyne Mar 15, 2017

mjgiarlo Mar 14, 2017

jcoyne Mar 15, 2017

mjgiarlo Mar 14, 2017

jcoyne Mar 15, 2017

dlpierce left a comment

Stop using asynchronous requests on the edit form #3147

Stop using asynchronous requests on the edit form #3147

Conversation

jcoyne commented Mar 14, 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

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

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

mjgiarlo left a 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

dlpierce left a comment

Choose a reason for hiding this comment