Implement new organise handlers #47
Conversation
Aratz
force-pushed
the
BIO-2361_implement_handler
branch
from
942999a to
7777dd1
Compare
| "required": {"type": "boolean"}, | ||
| "link_type": { | ||
| "type": "string", | ||
| "enum": ["softlink", "hardlink", "copy"] |
There was a problem hiding this comment.
we decided on removing the hardlink option
|
|
||
| # Status codes | ||
| OK = 200 | ||
| ACCEPTED = 202 | ||
| NO_CONTENT = 204 | ||
|
|
||
| BAD_REQUEST = 400 | ||
| FORBIDDEN = 403 | ||
| NOT_FOUND = 404 | ||
| INTERNAL_SERVER_ERROR = 500 |
There was a problem hiding this comment.
you can of course import these codes directly from the http.HTTPStatus module to avoid re-defining them
| except jsonschema.ValidationError as validation_error: | ||
| self.set_status(BAD_REQUEST) | ||
| self.write(str(validation_error)) | ||
| return wrapper |
| staging and delivery. | ||
| """ | ||
|
|
||
| def initialize(self, **kwargs): |
There was a problem hiding this comment.
is it an expected pattern in the handlers to have a initialize-function instead of/in addition to the constructor?
| config = yaml.load(organise_config_file, Loader=yaml.CLoader) | ||
| with open(organise_config_dir / "schema.json", 'r') as organise_config_schema: | ||
| schema = json.load(organise_config_schema) | ||
| jsonschema.validate(config, schema) |
There was a problem hiding this comment.
for re-usability, it could be nice to break out the validation code to its own (static) function, perhaps even move it to another service if that fits with the overall code structure
maybe it's up for consideration if the validation should happen here in the handler or downstream in the service?
There was a problem hiding this comment.
also, this code is more or less repeated (with slight variations) in all three organize handlers, could you consider breaking it out to a helper function that can be called instead?
| if self.dummy_config[key] is None: | ||
| tempdir = tempfile.TemporaryDirectory() | ||
| self.__setattr__(f"_{key}", tempdir) | ||
| self.dummy_config[key] = tempdir.name |
| class TestOrganiseHandlers(AsyncHTTPTestCase): | ||
| API_BASE = "/api/1.0" | ||
|
|
||
| def get_app(self): |
There was a problem hiding this comment.
is the get_app function called automatically by the test framework when setting up the tests?
| self.assertEqual(response.code, 404) | ||
|
|
||
| def test_project_analysis_handler_missing_config_file(self): | ||
| analysis = "rnaseq" |
There was a problem hiding this comment.
super nitpick: might be a good idea to use a dummy value instead of a potentially valid analysis name
| method="POST", body='', | ||
| ) | ||
|
|
||
| self.assertEqual(response.code, 500) |
There was a problem hiding this comment.
this code is 500 but above (and below) it's a 404, is this intentional?
| method="POST", body='{}', | ||
| ) | ||
|
|
||
| self.assertEqual(response.code, 200) |
There was a problem hiding this comment.
shouldn't this give 404 or 500?
There was a problem hiding this comment.
This looks really nice! However, depending on the answer on if we really need several handlers when using a configuration file as a template for the organization I might have more comments. This is nothing that needs to be addressed now, but we should discuss it before continuing working on this project.
| url(r"/api/1.0/organise/runfolder/([^/]+)", OrganiseRunfolderHandler, | ||
| name="organise_runfolder", kwargs=kwargs), | ||
|
|
||
| url(r"/api/1.0/organise/delivery/runfolder/([^/]+)", |
There was a problem hiding this comment.
Do we actually need one endpoint for every type of organization we want to do? Shouldn't it be enough to have one OrganiseRunfolderConfigHandler since the organization it self is defined in the config? Please correct me if I get it all wrong :)
What problems does this PR solve?
This PR adds handlers to connect the webservice's endpoints to the organise service. This might be need to be adapted in the future depending on the rest of the code so this is only a draft for now.
How has the changes been tested?
Unit tests have been added, not sure if integration tests should be added already or if we should wait until we have all the pieces.
Reasons for careful code review
If any of the boxes below are checked, extra careful code review should be inititated.