Bug #4015

[Workbench] Collection chooser should be filling in portable_data_hash as well as collection uuid when used to pick pipeline inputs.

Added by Peter Amstutz almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
Workbench
Target version:
Start date:
10/21/2014
Due date:
% Done:

100%

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

Description

Parameter hash should give portable data hash as the "value" but should also record the collection UUID so the current selection can be rendered in human-readable form.


Subtasks

Task #4304: Review 4015-collection-chooser-portable-data-hashResolvedTim Pierce

Task #4210: populate portable_data_hash in the collection chooserResolvedTim Pierce


Related issues

Blocks Arvados - Bug #4269: [API] Disallow collection UUIDs in script_parameters, only allow portable data hashResolved12/04/2014

Associated revisions

Revision 846ac5d4
Added by Tim Pierce almost 5 years ago

Merge branch '4015-collection-chooser-portable-data-hash'

Fixes #4015.

History

#1 Updated by Tom Clegg almost 5 years ago

  • Subject changed from Collection chooser should be filling in portable_data_hash, not uuid for collections, when used to pick pipeline inputs. to Collection chooser should be filling in portable_data_hash as well as collection uuid when used to pick pipeline inputs.
  • Description updated (diff)

Before #3036 the chooser would record a "name link" uuid, which served the same purpose as the current collection uuid, as well as manifest hash.

#2 Updated by Tom Clegg almost 5 years ago

  • Subject changed from Collection chooser should be filling in portable_data_hash as well as collection uuid when used to pick pipeline inputs. to [Workbench] Collection chooser should be filling in portable_data_hash as well as collection uuid when used to pick pipeline inputs.
  • Story points set to 0.5

#3 Updated by Tom Clegg almost 5 years ago

  • Target version changed from Bug Triage to Arvados Future Sprints

#4 Updated by Ward Vandewege almost 5 years ago

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

#5 Updated by Tim Pierce almost 5 years ago

  • Category set to Workbench
  • Assigned To set to Tim Pierce

#6 Updated by Tom Clegg almost 5 years ago

  • Description updated (diff)

In PipelineInstancesController#update there is some (possibly now unreachable) code that does something very similar:

              if resource_class_for_uuid(value_info[:value]) == Link
                # Use the link target, not the link itself, as script
                # parameter; but keep the link info around as well.
                link = Link.find value_info[:value]
                value_info[:value] = link.head_uuid
                value_info[:link_uuid] = link.uuid
                value_info[:link_name] = link.name
              else
                # Delete stale link_uuid and link_name data.
                value_info[:link_uuid] = nil
                value_info[:link_name] = nil
              end

Corresponding code in ApplicationHelper#render_pipeline_component_attribute:

      if value_info.is_a?(Hash)
        if (link = Link.find? value_info[:link_uuid])
          display_value = link.name
        elsif value_info[:link_name]
          display_value = value_info[:link_name]
        end
      end

This same approach should still work -- but rather than storing :link_name and :link_uuid, we want to store something like :selection_uuid and :selection_name. (Best not to use the word "collection": this same feature will apply to other objects when they become content addressable too.)

And, of course, store the selected collection's portable_data_hash instead of its uuid in value_info[:value].

#7 Updated by Ward Vandewege almost 5 years ago

  • Status changed from New to In Progress

#8 Updated by Radhika Chippada almost 5 years ago

I only have some comments about the tests.

  • pipeline_instances_test.rb

Please remove “from” in “Create a pipeline instance from without a project”

  • Is is really required that you add a new collection and new template for the test added in pipeline_instances_test? It seems like you could verify that the pdh is recorded using existing fixtures
  • Is it required that you add a new test for the verification? Could you not update an existing test to do this verification after selecting input?
  • Just in case, do fuse tests need to be updated for the new fixture additions (they seem to be modified by active user)?

#9 Updated by Tim Pierce almost 5 years ago

Radhika Chippada wrote:

I only have some comments about the tests.

  • pipeline_instances_test.rb

Please remove “from” in “Create a pipeline instance from without a project”

changed to "from outside of a project" to be less idiomatic :-)

  • Is is really required that you add a new collection and new template for the test added in pipeline_instances_test? It seems like you could verify that the pdh is recorded using existing fixtures
  • Is it required that you add a new test for the verification? Could you not update an existing test to do this verification after selecting input?

It might be possible to reuse existing collections, templates and tests. Since the existing test fixtures are written to address conditions for specific other unit tests, I thought that the better choice was to add new collection and pipeline fixtures. We've had problems in the past where modifying a fixture to do something differently for multiple tests led to weird cross-test failures.

On the other hand, maybe those are exactly the kind of failures we want to uncover and fix? I definitely want to follow best practices for unit and integration testing. If that means "make fixtures as generic as possible so they can be reused by multiple tests" I'll do that.

  • Just in case, do fuse tests need to be updated for the new fixture additions (they seem to be modified by active user)?

Oof, good catch. I ran the workbench tests but I think I forgot (again!) to run the FUSE tests. I'll double-check.

#10 Updated by Tim Pierce almost 5 years ago

Followup question from Radhika:

I could be wrong, but I could not find anything special in the new collection and it seemed similar to other collections such as "foo_collection_in_aproject" and you can use this instead of adding another one. Same goes with the template as well. Regarding the new test, it appears that you could test the Advanced tab and verify that the pdh info is added by updating the "def create_and_run_pipeline_in_aproject in_aproject" method in pipeline_instances_test.rb, just before the comment "Run button present and enabled" or just before checking the Graph tab. Sometimes, I prefer to update existing tests because it is an extension of current behavior rather than a completely new flow of events but it is your call if you would like to stick with a new test.

I think you're correct. Please look at the new branch at fff7228b. This backs out the new test fixtures, and updates the test Create and run a pipeline and the helper create_and_run_pipeline_in_aproject method.

#11 Updated by Radhika Chippada almost 5 years ago

Tim, thanks for updating. The tests are much better and feel like they are doing it all in a flow now. Thanks.

#12 Updated by Tim Pierce almost 5 years ago

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

Applied in changeset arvados|commit:846ac5d419862e2b0051ec0843e71c601b35da44.

Also available in: Atom PDF