[ARVADOS] created: 165a594bf8606864c62f86405e318c68c2426c38

git at public.curoverse.com git at public.curoverse.com
Thu Mar 19 02:44:51 EDT 2015


        at  165a594bf8606864c62f86405e318c68c2426c38 (commit)


commit 165a594bf8606864c62f86405e318c68c2426c38
Author: Tom Clegg <tom at curoverse.com>
Date:   Thu Mar 19 01:50:53 2015 -0400

    5198: Remove href attributes from remote:true links to prevent ctrl-click et al. from doing the wrong thing.

diff --git a/apps/workbench/app/assets/javascripts/link_to_remote.js b/apps/workbench/app/assets/javascripts/link_to_remote.js
new file mode 100644
index 0000000..f4f4f3e
--- /dev/null
+++ b/apps/workbench/app/assets/javascripts/link_to_remote.js
@@ -0,0 +1,23 @@
+$.rails.href = function(element) {
+    if (element.is('a')) {
+        // data-remote=true links must put their remote targets in
+        // data-remote-href="..." instead of href="...".  This helps
+        // us avoid accidentally using the same href="..." in both the
+        // remote (Rails UJS) and non-remote (native browser) handlers
+        // -- which differ greatly in how they use that value -- and
+        // forgetting to test any non-remote cases like "open in new
+        // tab". If you really want copy-link-address/open-in-new-tab
+        // to work on a data-remote=true link, supply the
+        // copy-and-pastable URI in href in addition to the AJAX URI
+        // in data-remote-href.
+        //
+        // (Currently, the only places we make any remote links are
+        // link_to() in ApplicationHelper, which renames href="..." to
+        // data-remote-href="...", and select_modal, which builds a
+        // data-remote=true link on the client side.)
+        return element.data('remote-href');
+    } else {
+        // Normal rails-ujs behavior.
+        return element.attr('href');
+    }
+}
diff --git a/apps/workbench/app/assets/javascripts/select_modal.js b/apps/workbench/app/assets/javascripts/select_modal.js
index 3b51faa..d9ad7f8 100644
--- a/apps/workbench/app/assets/javascripts/select_modal.js
+++ b/apps/workbench/app/assets/javascripts/select_modal.js
@@ -151,7 +151,7 @@ $(document).on('click', '.selectable', function() {
             return false;
         }
         $('<a />').
-            attr('href', $form.attr('data-search-modal')).
+            attr('data-remote-href', $form.attr('data-search-modal')).
             attr('data-remote', 'true').
             attr('data-method', 'GET').
             hide().
diff --git a/apps/workbench/app/helpers/application_helper.rb b/apps/workbench/app/helpers/application_helper.rb
index d02d058..794ce1b 100644
--- a/apps/workbench/app/helpers/application_helper.rb
+++ b/apps/workbench/app/helpers/application_helper.rb
@@ -52,6 +52,34 @@ module ApplicationHelper
     ArvadosBase::resource_class_for_uuid(attrvalue, opts)
   end
 
+  # When using {remote:true} or {method:...}, move the target URI from
+  # href to data-remote-href. Otherwise, browsers offer features like
+  # "open in new window" and "copy link address" which bypass Rails'
+  # click handler and therefore end up at incorrect/nonexistent routes
+  # (by ignoring data-method) and expect to receive pages rather than
+  # javascript responses.
+  #
+  # See assets/javascripts/link_to_remote.js for supporting code.
+  def link_to *args, &block
+    if (args.last and args.last.is_a? Hash and
+        (args.last[:remote] or
+         (args.last[:method] and
+          args.last[:method].to_s.upcase != 'GET')))
+      if Rails.env.test?
+        # Capybara/phantomjs can't click_link without an href, even if
+        # the click handler means it never gets used.
+        raw super.gsub(' href="', ' href="#" data-remote-href="')
+      else
+        # Regular browsers work as desired: users can click A elements
+        # without hrefs, and click handlers fire; but there's no "copy
+        # link address" option in the right-click menu.
+        raw super.gsub(' href="', ' data-remote-href="')
+      end
+    else
+      super
+    end
+  end
+
   ##
   # Returns HTML that links to the Arvados object specified in +attrvalue+
   # Provides various output control and styling options.
diff --git a/apps/workbench/test/controllers/projects_controller_test.rb b/apps/workbench/test/controllers/projects_controller_test.rb
index c2089ad..ec17e8e 100644
--- a/apps/workbench/test/controllers/projects_controller_test.rb
+++ b/apps/workbench/test/controllers/projects_controller_test.rb
@@ -28,7 +28,7 @@ class ProjectsControllerTest < ActionController::TestCase
         id: readonly_project_uuid
       }, session_for(which_user)
       buttons = css_select('[data-method=post]').select do |el|
-        el.attributes['href'].match /project.*owner_uuid.*#{readonly_project_uuid}/
+        el.attributes['data-remote-href'].match /project.*owner_uuid.*#{readonly_project_uuid}/
       end
       if should_show
         assert_not_empty(buttons, "did not offer to create a subproject")
diff --git a/apps/workbench/test/diagnostics/pipeline_test.rb b/apps/workbench/test/diagnostics/pipeline_test.rb
index 168656d..f9e324c 100644
--- a/apps/workbench/test/diagnostics/pipeline_test.rb
+++ b/apps/workbench/test/diagnostics/pipeline_test.rb
@@ -39,7 +39,7 @@ class PipelineTest < DiagnosticsTest
       wait_for_ajax
 
       # All needed input are filled in. Run this pipeline now
-      click_link 'Components'
+      find('a,button', text: 'Components').click
       find('a,button', text: 'Run').click
 
       # Pipeline is running. We have a "Stop" button instead now.
diff --git a/apps/workbench/test/helpers/share_object_helper.rb b/apps/workbench/test/helpers/share_object_helper.rb
index 783e2bb..ba09acc 100644
--- a/apps/workbench/test/helpers/share_object_helper.rb
+++ b/apps/workbench/test/helpers/share_object_helper.rb
@@ -55,10 +55,9 @@ module ShareObjectHelper
         # poltergeist returns true for confirm(), so we don't need to accept.
       end
     end
-    wait_for_ajax
+    # Ensure revoked permission disappears from page.
     using_wait_time(Capybara.default_wait_time * 3) do
-      assert(page.has_no_text?(name),
-             "new share row still exists after being revoked")
+      assert_no_text name
       assert_equal(start_rows.size - 1, share_rows.size,
                    "revoking share did not remove row from sharing table")
     end

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list