Bug #4015
closed
[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 over 9 years ago. Updated over 9 years ago.
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.
Related issues
Updated by Tom Clegg over 9 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.
Updated by Tom Clegg over 9 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
Updated by Tom Clegg over 9 years ago
- Target version changed from Bug Triage to Arvados Future Sprints
Updated by Ward Vandewege over 9 years ago
- Target version changed from Arvados Future Sprints to 2014-10-29 sprint
Updated by Tim Pierce over 9 years ago
- Category set to Workbench
- Assigned To set to Tim Pierce
Updated by Tom Clegg over 9 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].
Updated by Ward Vandewege over 9 years ago
- Status changed from New to In Progress
Updated by Radhika Chippada over 9 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)?
Updated by Tim Pierce over 9 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.
Updated by Tim Pierce over 9 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.
Updated by Radhika Chippada over 9 years ago
Tim, thanks for updating. The tests are much better and feel like they are doing it all in a flow now. Thanks.
Updated by Tim Pierce over 9 years ago
- Status changed from In Progress to Resolved
- % Done changed from 50 to 100
Applied in changeset arvados|commit:846ac5d419862e2b0051ec0843e71c601b35da44.