Scottx611x/td api #1646

scottx611x · 2017-03-23T20:37:52Z

Closes #1613

Incorporation of related object deletion for ToolDefinitions
JSONSchema-based validation of Workflow step's annotations (Parameters/OutputFiles)
Proper Parameter/OutputFile association when generating ToolDefinitions

…into scottx611x/acceptance_testing

…ator

… build

codecov-io · 2017-03-23T20:47:36Z

Codecov Report

Merging #1646 into develop will increase coverage by 0.6%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           develop    #1646     +/-   ##
==========================================
+ Coverage    37.03%   37.64%   +0.6%     
==========================================
  Files          369      369             
  Lines        24093    24317    +224     
  Branches      1260     1260             
==========================================
+ Hits          8924     9153    +229     
+ Misses       15169    15164      -5

Impacted Files	Coverage Δ
refinery/tool_manager/serializers.py	`100% <100%> (ø)`	⬆️
refinery/tool_manager/admin.py	`100% <100%> (ø)`	⬆️
refinery/tool_manager/models.py	`95.18% <100%> (+1.63%)`	⬆️
...r/management/commands/generate_tool_definitions.py	`70.68% <66.66%> (+70.68%)`	⬆️
refinery/tool_manager/utils.py	`82.97% <72.09%> (-13.69%)`	⬇️
refinery/tool_manager/tests.py	`99.6% <99.38%> (-0.4%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8734d1c...cdc401e. Read the comment docs.

jkmarx · 2017-03-24T13:09:25Z

refinery/tool_manager/models.py

+
+
+@receiver(pre_delete, sender=ToolDefinition)
+def _tooldefinition_pre_delete(sender, instance, *args, **kwargs):


unsure of convention, _tool_definition_pre_delete?

Also, it's probably not a good idea to add the signal name to the handler name because signal name might change. It is better to describe what the function actually does (example)

jkmarx · 2017-03-24T13:27:47Z

refinery/tool_manager/serializers.py

@@ -13,10 +13,30 @@ class Meta:


 class ParameterSerializer(serializers.ModelSerializer):
+    galaxy_workflow_step = serializers.SerializerMethodField(
+        method_name="_get_galaxy_workflow_step", required=False,


For readability, suggestion:
galaxy_workflow_step = serializers.SerializerMethodField(
method_name="_get_galaxy_workflow_step",
required=False,
allow_null=False
)

hackdna · 2017-03-24T13:44:45Z

refinery/tool_manager/management/commands/generate_tool_definitions.py

@@ -48,6 +50,7 @@ def handle(self, **options):

            # Validate workflow annotation data, and try to create a
            # ToolDefinition if validation passes.
+


It might make more sense to get the list of all the workflows from all the engines first to avoid an extra indentation level. It might also be helpful to factor out this functionality into a separate re-useable utility function.

hackdna · 2017-03-24T14:24:02Z

refinery/tool_manager/management/commands/generate_tool_definitions.py

+                for step_index in workflow_data["steps"]:
+                    step = workflow_data["steps"][step_index]
+                    # Check if any annotation provided, and try to convert
+                    # to python dict if so


This is redundant: comments should be explaining "why", not "what".

hackdna · 2017-03-24T14:25:02Z

refinery/tool_manager/management/commands/generate_tool_definitions.py

+                            # `parameters` we can pass here without
+                            # raising an exception
+                            pass
+                        else:


Not needed. Removing will help reduce indentation levels.

hackdna · 2017-03-24T14:25:51Z

refinery/tool_manager/management/commands/generate_tool_definitions.py

+                                # `tool_inputs`
+                                if (not parameter["name"] in
+                                        step["tool_inputs"]):
+                                    raise RuntimeError(


Should this be CommandError?

hackdna · 2017-03-24T14:48:00Z

refinery/tool_manager/management/commands/generate_tool_definitions.py

+                                # annotation data against the available
+                                # parameters of the Workflow step's
+                                # `tool_inputs`
+                                if (not parameter["name"] in


This should probably be a membership test instead of a boolean

hackdna · 2017-03-24T14:59:38Z

refinery/tool_manager/models.py

+            ).galaxy_workflow_step
+        except (GalaxyParameter.DoesNotExist,
+                GalaxyParameter.MultipleObjectsReturned):
+            return None


Usually, return values should be of the same type and errors should be indicated by exceptions. Is returning None helpful here?

Yes, this just allows the Parameter Serializer to include the GalaxyParameter models fields when need be, and disclude them from the response if we are dealing with parent Parameter objects

hackdna · 2017-03-24T15:03:01Z

refinery/tool_manager/models.py

+
+
+@receiver(pre_delete, sender=ToolDefinition)
+def _tooldefinition_pre_delete(sender, instance, *args, **kwargs):


Also, it's probably not a good idea to add the signal name to the handler name because signal name might change. It is better to describe what the function actually does (example)

hackdna · 2017-03-24T15:03:24Z

refinery/tool_manager/models.py

+
+
+@receiver(post_delete, sender=ToolDefinition)
+def _tooldefinition_post_delete(sender, instance, *args, **kwargs):


Same as above

hackdna · 2017-03-24T15:03:30Z

refinery/tool_manager/models.py

+
+
+@receiver(pre_delete, sender=FileRelationship)
+def _filerelationship_pre_delete(sender, instance, *args, **kwargs):


Same as above

hackdna · 2017-03-24T15:07:20Z

refinery/tool_manager/utils.py


 logger = logging.getLogger(__name__)
+ANNOTATION_ERROR_MESSAGE = (


There probably should be no new line characters here.

What would be the alternative?

I think it's best to avoid "one log statement != one written log"

This reverts commit 69d5c65.

hackdna · 2017-03-27T20:03:38Z

refinery/tool_manager/utils.py

+    workflow_list = []
+    workflow_engines = WorkflowEngine.objects.all()
+
+    sys.stdout.write(


We should probably make this utility function re-useable in other parts of the application besides management commands. If so, it should not be writing anything to stdout.

hackdna · 2017-03-27T20:06:25Z

refinery/tool_manager/management/commands/generate_tool_definitions.py

-                "Generating ToolDefinitions from workflow engine {}\n"
-                .format(engine.name)
-            )
+        workflow_list = get_workflow_list()


get_workflow_list() can raise RuntimeError exceptions that should be handled

hackdna · 2017-03-27T20:10:32Z

refinery/tool_manager/tests.py

@@ -111,40 +114,44 @@ def test_for_proper_parameters_response(self):

 class ToolDefinitionGenerationTests(TestCase):
    def test_workflow_improperly_annotated(self):
-        with open("tool_manager/test-data/workflow_annotation_invalid.json",
-                  "r") as f:
+        with open("tool_manager/test-data/workflow_annotation_invalid"


Should this be test_data instead of test-data?

@hackdna I can rename the directory?

It's probably a good idea for consistency. All other folder and file names appear to have underscores.

scottx611x added 30 commits October 21, 2016 13:47

Cleanup some more tests

d7283b8

Add id to aid testing

c09101a

Add id to aid testing

c38f042

Cleanup login method

59bb514

Abstract out reused login moethod

facf9cb

Utilize relative import

1222f9c

Merge branch 'develop' of https://github.com/parklab/refinery-platform …

87101a8

…into scottx611x/acceptance_testing

PEP8

41a51c8

only run acceptance tests if we detect a build for a PR

1fbca66

Always run our acceptance tests

28279fb

Add acceptance tests for user registration and login

d73351b

Don't use separate shell script (for now)

be166d8

Remove cd comands

ab0ac77

Refactor selenium utility functions

363c8f6

Add script to run acceptance tests on vagrant or travis

3c39ee5

Lower DEFAULT_WAIT Now that we aren't relying on SauceLabs VMs

6287456

Cleanup setting of breakpoints, and refactor cleanup_on_error decor…

bcce1d9

…ator

Remove unnecessary env var, and re-add global as to ensure a single…

daca73f

… build

Sauce_connect isn't needed

aff4cf6

pip caching seems stable

d1d36fa

PEP8

8a8dbae

Fail shell script if any step doesn't work properly

28deb1d

GeckoDriver expects a more recent version of Firefox

6419461

Remove Comments

2e080fb

Update Default wait time

c8d0d16

Bump pytest-selenium version

c3b3ba8

Update Comment

f5b1de1

Utilize wait_until_id_clickable

bc77da6

Add tests for global analysis popover

27a82db

Add tests for Datasets panel

8eb8879

scottx611x added 9 commits March 23, 2017 14:20

Update comments

fa065ed

Remove extra space

2a6e5b4

RuntimeError is actually being raised here

379b72d

Rename method to create_tool_definition

fab910b

Add API test to check for different Parameter Types

c470b49

Merge remote-tracking branch 'origin/develop' into scottx611x/td_api

197af3f

Merge remote-tracking branch 'origin/develop' into scottx611x/td_api

98821e7

Styling fixes

c182855

Fix comment

8be855c

scottx611x requested review from hackdna and jkmarx March 23, 2017 20:37

jkmarx reviewed Mar 24, 2017

View reviewed changes

hackdna requested changes Mar 24, 2017

View reviewed changes

scottx611x added 9 commits March 27, 2017 14:52

Rename pre/post delete handlers

c7a195b

Fix styling

960338c

Fix more styling

6e027d8

Remove newline characters

3c006ea

Factor out workflow fetching, and cleanup indentation.

e1d4f79

Add a sample galaxy get_workflows() response for testing purposes

cb0d526

Test management command functionality with proper mock usage

7bce4a6

Remove comment explaining "what"

69d5c65

Revert "Remove comment explaining "what""

524734d

This reverts commit 69d5c65.

hackdna requested changes Mar 27, 2017

View reviewed changes

scottx611x added 3 commits March 27, 2017 16:35

Rename test-data to test_data

2c82cde

Utilize logger statements since not in a MGMT command anymore

b837dee

Handle RuntimeError

cdc401e

hackdna approved these changes Mar 28, 2017

View reviewed changes

scottx611x merged commit 3cf0725 into develop Mar 28, 2017

scottx611x deleted the scottx611x/td_api branch March 28, 2017 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scottx611x/td api #1646

Scottx611x/td api #1646

scottx611x commented Mar 23, 2017 •

edited

codecov-io commented Mar 23, 2017 •

edited

jkmarx Mar 24, 2017

hackdna Mar 24, 2017

jkmarx Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

scottx611x Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 24, 2017

scottx611x Mar 24, 2017

hackdna Mar 24, 2017

hackdna Mar 27, 2017

hackdna Mar 27, 2017

hackdna Mar 27, 2017

scottx611x Mar 27, 2017

hackdna Mar 27, 2017



		@receiver(pre_delete, sender=ToolDefinition)
		def _tooldefinition_pre_delete(sender, instance, args, *kwargs):

		@@ -48,6 +50,7 @@ def handle(self, **options):

		# Validate workflow annotation data, and try to create a
		# ToolDefinition if validation passes.



		@receiver(post_delete, sender=ToolDefinition)
		def _tooldefinition_post_delete(sender, instance, args, *kwargs):



		@receiver(pre_delete, sender=FileRelationship)
		def _filerelationship_pre_delete(sender, instance, args, *kwargs):


		logger = logging.getLogger(__name__)
		ANNOTATION_ERROR_MESSAGE = (

Scottx611x/td api #1646

Scottx611x/td api #1646

Conversation

scottx611x commented Mar 23, 2017 • edited

codecov-io commented Mar 23, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottx611x commented Mar 23, 2017 •

edited

codecov-io commented Mar 23, 2017 •

edited