Story #4525

[Workbench] Remove stale clippy code.

Added by Radhika Chippada about 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
12/03/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

examples
  • views/collections/_show_files.html.erb (see note-1 below)
  • a good chunk of assets/selection.js.erb
  • add_form_selection_sources stuff in helpers/application_helper.rb

Subtasks

Task #4707: Review branch: 4525-remove-stale-clippy-codeResolvedTom Clegg


Related issues

Is duplicate of Arvados - Story #3791: [Workbench] Retire clippy. This requires some means other than clippy to select individual files as input to a pipeline.Resolved

Associated revisions

Revision bfaa0f8f
Added by Radhika Chippada almost 6 years ago

closes #4525
closes #4694
Merge branch '4525-remove-stale-clippy-code'

History

#1 Updated by Radhika Chippada about 6 years ago

Since clippy is now retired (#3177), we can update the script tag in collection show_files partial.

<script>
// The "each" loop in select_all_files() and unselect_all_files()
// is needed because .trigger("change") does not play well with clippy.
// Once clippy has been retired, we should be able to compress this
// into .filter(":visible").prop("checked", true).trigger("change").
...
</script>

#2 Updated by Radhika Chippada about 6 years ago

  • Subject changed from [Workbench] Remove logic in collection show_file partial that deals with clippy. to [Workbench] Remove logic in collection show_file partial that deals with clippy since clippy is now retired.
  • Category set to Workbench

#3 Updated by Tom Clegg about 6 years ago

  • Subject changed from [Workbench] Remove logic in collection show_file partial that deals with clippy since clippy is now retired. to [Workbench] Remove stale clippy code.
  • Description updated (diff)

#4 Updated by Radhika Chippada almost 6 years ago

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

#5 Updated by Radhika Chippada almost 6 years ago

  • Please use 4525-remove-stale-clippy-code to review code updates.
  • The branch 4525-remove-stale-clippy-code addressed the three items listed in description. It also addresses updating selection menu options when project tabs are switched (#4694)
  • All workbench tests are passing
  • Manual testing passed for the following:
    • Select one or more collections and expect the project selection options selected
    • select only one pipeline instance and expect that compare selection option is still disabled
    • select two or more pipeline instances and expect that compare selection option is enabled and compare operation works
    • Copy selected, move selected, combine selected worked as expected
    • In a Collection page: select all; unselect all; select one or more and then select all; deselect one or more and then click on unselect all; select all and unselect just one using the checkbox are all working after the uddate
    • In a collection page, combine selected files is working as expected
    • Add data to a project (using the selection in the search dialog) worked
    • Run a pipeline by selecting an input (in the input selection dialog) worked
    • Switch project tabs and observe that the selection options are reflecting only what is applicable to this tab

#6 Updated by Tom Clegg almost 6 years ago

In apps/workbench/app/helpers/application_helper.rb I think the following lines provide a few recent items from get_n_objects_of_class, and aren't related to clippy:

-
-    if selectables.any?
-      lt += raw("add_form_selection_sources(#{selectables.to_json});\n")
-    end

I'm not keen on the ":visible" selector. Doesn't that omit stuff you selected on page 2 before scrolling up to the top to hit the action button, for example? Anyway, I think the real issue here was that enable_disable_selection_actions wasn't restricting itself to one $container at a time. I've pushed an alternate solution on 4525-remove-stale-clippy-code-TC @ 51aaccb. Thoughts?

#7 Updated by Radhika Chippada almost 6 years ago

Tom,

  • I added back add_form_selection_sources in js and helper. I did not add back the function select_form_sources, which needs get_selection_list, which in turn needs storage and adding each selection to storage. Please confirm this is correct. My manual testing (using the above listed steps) worked. Also, all the tests passed.
  • Merged your branch with updated enable_disable_selection_actions into my branch. All manual and test verifications passed. So, I think this is good.

Thanks.

#8 Updated by Tom Clegg almost 6 years ago

Looked closer at the "selection sources" stuff. I think the current setup is:

In the helper method:
  • Build an array of options in selectables: [{name: "foo", uuid: "bar", type: "baz"}, ...]
  • Use the inline script tag to pass selectables to add_form_selection_sources().
  • Use the inline script tag to instruct X-editable to retrieve the selection dropdown content from select_form_sources().
In selection.js:
  • Combine the selectables with clippy stuff, and feed the combined set to X-editable when it calls select_form_sources.

I think that means all we need now is:

In the helper method:
  • Build an array of options in selectables -- but formatted the way x-editable wants it: [{text: "foo", value: "bar"}, ...]
  • Put the array in "data-source" in the link_to attrtext, '#', ... call.
  • No inline script tag.
In selection.js:
  • Remove all of the form_selection_sources stuff.

#9 Updated by Tom Clegg almost 6 years ago

(update after IRC chat)

If the "selectables" stuff isn't used at all, we don't need "data-source" from my previous comment, and we can remove the whole # preload data block from application_helper.rb as well.

#10 Updated by Radhika Chippada almost 6 years ago

Peter said (IRC):

I think you want to get rid of
#preload data and the code and the "if" block and the <script> section
and keep the link_to

The link_to will be used when
attrvalue.is_a? Fixnum datatype = 'number' and
attrvalue.is_a? String datatype = 'text'
if you have a number or text you still need to render the editing control

#11 Updated by Tom Clegg almost 6 years ago

LGTM (cdef2ed).

Now that #selection-form-content doesn't exist, I think we can also delete this from apps/workbench/app/assets/javascripts/selection.js.erb:

     $('#selection-form-content').on("click", function(e) {
         e.stopPropagation();
     });

...and the entire apps/workbench/app/assets/stylesheets/selection.css file.

#12 Updated by Radhika Chippada almost 6 years ago

Thanks Tom. I made those changes also.

#13 Updated by Radhika Chippada almost 6 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:bfaa0f8f3547fe8d08312f3fd08a0828faf647f5.

#14 Updated by Ward Vandewege almost 6 years ago

  • Story points set to 1.0

Also available in: Atom PDF