[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