Arvados

Tried this using bcosc's account. The sharing link IS created and the link works as well. However, the the page does not refresh after creating the link. It refreshes sometimes and other times it just sits there making you think it did not work. Same issue with unshare link as well. Fails to refresh consistently.

5bd0cf6 doesn't look very convincing to me. If the trouble were excess quoting, would this really cause "fail to refresh sometimes"? Also: why would .html_safe be needed here, while our other similar escape_javascript(render partial) bits work without it?

Radhika Chippada wrote:

Tried this using bcosc's account. The sharing link IS created and the link works as well. However, the the page does not refresh after creating the link. It refreshes sometimes and other times it just sits there making you think it did not work. Same issue with unshare link as well. Fails to refresh consistently.

Right now I see this issue as Radhika describes it. Tracked down the cause of the intermittent failure and created #4676 to deal with it. Still haven't reproduced the "Not Found" message reported in this ticket, but still working on that.

This happened because Bryan's browser sent the request as a GET, but the route is POST-only. From the logs:

Started GET "/collections/qr1hi-4zz18-ntyi7uygpyep0sx/share" for [Bryan] at 2014-10-23 17:29:10 +0000
Processing by CollectionsController#show_file as HTML
  Parameters: {"uuid"=>"qr1hi-4zz18-ntyi7uygpyep0sx", "file"=>"share"}
[a bunch of rendering happens]
#<ActionController::RoutingError: Path not found>
[more rendering]
Completed 404 Not Found in 4141ms (Views: 708.8ms | ActiveRecord: 0.0ms)

This seems to be what happens if you ctrl-click / middle-click a remote:true link: the browser ignores all the remote/method stuff and just does a plain old GET with the location given in href.

(You'd hope rails-ujs was dealing with this for us somehow. Perhaps it's hard to catch/distinguish a ctrl-click?)

4291-workbench-collection-sharing-button-wip is up for review. It fixes this issue by turning the link into a button, so you can't "open" it in a new window/tab. It also includes tests, a fix for #4676, and some general cleanups that I did while narrowing down the cause. The git log provides the rationale for each change.

e0c20695767a45366b9f5993b460b8b58841868f

You have a comment "Note that `form_tag` does take a string, so not every method should be symbolized."

Which would suggest that the following changes are incorrect?

--- a/apps/workbench/app/views/users/_add_ssh_key_popup.html.erb
+++ b/apps/workbench/app/views/users/_add_ssh_key_popup.html.erb
@@ -1,7 +1,7 @@
 <div class="modal-dialog modal-with-loading-spinner">
   <div class="modal-content">

-    <%= form_tag add_ssh_key_path, {method: 'get', id: 'add_new_key_form', name: 'add_new_key_form', class: 'form-search, new_authorized_key', remote: true} do %>
+    <%= form_tag add_ssh_key_path, {method: :get, id: 'add_new_key_form', name: 'add_new_key_form', class: 'form-search, new_authorized_key', remote: true} do %>

       <div class="modal-header">
         <button type="button" class="close" onClick="reset_form()" data-dismiss="modal" aria-hidden="true">&times;</button>
--- a/apps/workbench/app/views/users/_setup_popup.html.erb
+++ b/apps/workbench/app/views/users/_setup_popup.html.erb
@@ -1,7 +1,7 @@
 <div class="modal-dialog modal-with-loading-spinner">
   <div class="modal-content">

-    <%= form_tag setup_user_path, {id: 'setup_form', name: 'setup_form', method: 'get',
+    <%= form_tag setup_user_path, {id: 'setup_form', name: 'setup_form', method: :get,
         class: 'form-search', remote: true} do %>

     <div class="modal-header">

It should probably render a message in the following situation (at first I thought the sharing button code was broken...)

_sharing_button.html.erb

<% if @search_sharing.nil? %>
  <%# API token can't manage other tokens.  Disable this feature. %>
<% else ... %>
  ...
<% end %>

Also, the "Unshare" button should either come after the sharing link, or be on the same line as "Shared at:". You probably need to split <div>Shared at: ...</div> into three separate <div>s.

  <div>Shared at:
    <% button_attrs[:class] += " pull-right" %>
    <%= button_to("Unshare", {action: "unshare"}, button_attrs) %>
    <div class="smaller-text" style="word-break: break-all"><%= link_to download_link, download_link %></div>
  </div>

Peter Amstutz wrote:

You have a comment "Note that `form_tag` does take a string, so not every method should be symbolized."

Which would suggest that the following changes are incorrect?

Yes, thank you for catching that. In my defense, form_for takes a symbol. I got turned around on this about three times during development, and I'm a little unclear on how any mortal is supposed to keep them straight. Fixed these, and confirmed with git grep that there weren't any other mixups among that pair.

Implemented your other suggestions at ebdfbb9.

I ran all the workbench tests and got one failure:

  1) Failure:
PipelineInstancesTest#test_scroll_pipeline_instances_page_for_fuse_with_search_filter_FUSE_project_and_expect_1_<=_found_items_<=_1 [/home/peter/work/arvados/apps/workbench/test/integration/pipeline_instances_test.rb:475]:
Not found expected number of items. Expected 1 and found 2.
Expected: true
  Actual: false

However the error went away when I ran just that test file again. Hmm.

Peter Amstutz wrote:

However the error went away when I ran just that test file again. Hmm.

I was unable to reproduce the failure either when running the entire Workbench test suite, or the pipeline instances integration tests again. There have been intermittent failures of the scrolling tests on Jenkins. I'm inclined to say the test has a race condition and you got unlucky. We should fix them, but frankly I don't want to hold up this merge for it.

LGTM