Bug #4291

[Workbench] Create sharing link for a collection is broken

Added by Bryan Cosca about 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
-
Target version:
Start date:
10/28/2014
Due date:
% Done:

100%

Estimated time:
(Total: 3.00 h)
Story points:
0.5

Description

It gives me a Not Found message. i.e. collections/qr1hi-4zz18-ntyi7uygpyep0sx under sharing and permissions button. (not sure if this coincides with the slowness of qr1hi this day)


Subtasks

Task #4330: Review branch: 4291-collection-sharing-link-refresh-issueClosed

Task #4688: Review 4291-workbench-collection-sharing-button-wipResolvedPeter Amstutz


Related issues

Copied to Arvados - Bug #4676: [Workbench] Collection "Sharing link" AJAX intermittently returns text/htmlResolved11/25/2014

Associated revisions

Revision 994fa456
Added by Brett Smith almost 6 years ago

Merge branch '4291-workbench-collection-sharing-button-wip'

Closes #4291, #4676, #4688.

History

#1 Updated by Radhika Chippada about 6 years ago

  • Target version set to Bug Triage

#2 Updated by Radhika Chippada about 6 years ago

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.

#3 Updated by Radhika Chippada about 6 years ago

  • Status changed from New to In Progress
  • Assigned To set to Radhika Chippada
  • Target version changed from Bug Triage to 2014-10-29 sprint

#4 Updated by Tom Clegg about 6 years ago

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?

#5 Updated by Ward Vandewege about 6 years ago

  • Subject changed from Create sharing link for a collection is broken to [Workbench] Create sharing link for a collection is broken

#6 Updated by Ward Vandewege about 6 years ago

  • Story points set to 0.5

#7 Updated by Radhika Chippada about 6 years ago

  • Status changed from In Progress to New
  • Assigned To deleted (Radhika Chippada)
  • Target version changed from 2014-10-29 sprint to Arvados Future Sprints

#8 Updated by Tom Clegg about 6 years ago

  • Target version changed from Arvados Future Sprints to 2014-12-10 sprint

#9 Updated by Brett Smith about 6 years ago

  • Assigned To set to Brett Smith

#10 Updated by Brett Smith about 6 years ago

  • Status changed from New to In Progress

#11 Updated by Brett Smith about 6 years ago

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.

#12 Updated by Brett Smith about 6 years ago

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)

#13 Updated by Tom Clegg about 6 years ago

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?)

#14 Updated by Brett Smith almost 6 years ago

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.

#15 Updated by Peter Amstutz almost 6 years ago

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">

#16 Updated by Peter Amstutz almost 6 years ago

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>

#17 Updated by Brett Smith almost 6 years ago

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.

#18 Updated by Peter Amstutz almost 6 years ago

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

#19 Updated by Peter Amstutz almost 6 years ago

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

#20 Updated by Brett Smith almost 6 years ago

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.

#21 Updated by Peter Amstutz almost 6 years ago

LGTM

#22 Updated by Brett Smith almost 6 years ago

  • % Done changed from 33 to 100
  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:994fa4569b8da43486719ce0da770ab1887b24b9.

Also available in: Atom PDF