Expand html summary setter (#2150)

josenavas · antgonza · commit a7e3d8b806bc · 2017-06-29T11:31:56.000-06:00
* Changing the HTML setter to add support dirs

* Extending the patch operation

* Fixing remaining calls to html_summary_fp set

* Fixing test

* Deleting unused function

* Removing unused import

* Fixing tests

* Fixing failing test
diff --git a/qiita_db/artifact.py b/qiita_db/artifact.py
@@ -11,6 +11,9 @@
 from itertools import chain
 from datetime import datetime
 from os import remove
+from os.path import isfile
+from shutil import rmtree
+from functools import partial
 
 import networkx as nx
 
@@ -941,41 +944,55 @@ def html_summary_fp(self):
 
         return res
 
-    @html_summary_fp.setter
-    def html_summary_fp(self, value):
+    def set_html_summary(self, html_fp, support_dir=None):
         """Sets the HTML summary of the artifact
 
         Parameters
         ----------
-        value : str
+        html_fp : str
             Path to the new HTML summary
+        support_dir : str
+            Path to the directory containing any support files needed by
+            the HTML file
         """
         with qdb.sql_connection.TRN:
-            current = self.html_summary_fp
-            if current:
+            if self.html_summary_fp:
                 # Delete the current HTML summary
-                fp_id = current[0]
-                fp = current[1]
+                to_delete_ids = []
+                to_delete_fps = []
+                for fp_id, fp, fp_type in self.filepaths:
+                    if fp_type in ('html_summary', 'html_summary_dir'):
+                        to_delete_ids.append([fp_id])
+                        to_delete_fps.append(fp)
                 # From the artifact_filepath table
                 sql = """DELETE FROM qiita.artifact_filepath
                          WHERE filepath_id = %s"""
-                qdb.sql_connection.TRN.add(sql, [fp_id])
+                qdb.sql_connection.TRN.add(sql, to_delete_ids, many=True)
                 # From the filepath table
                 sql = "DELETE FROM qiita.filepath WHERE filepath_id=%s"
-                qdb.sql_connection.TRN.add(sql, [fp_id])
+                qdb.sql_connection.TRN.add(sql, to_delete_ids, many=True)
                 # And from the filesystem only after the transaction is
                 # successfully completed (after commit)
-                qdb.sql_connection.TRN.add_post_commit_func(remove, fp)
+
+                def path_cleaner(fp):
+                    if isfile(fp):
+                        remove(fp)
+                    else:
+                        rmtree(fp)
+                qdb.sql_connection.TRN.add_post_commit_func(
+                    partial(map, path_cleaner, to_delete_fps))
 
             # Add the new HTML summary
+            filepaths = [(html_fp, 'html_summary')]
+            if support_dir is not None:
+                filepaths.append((support_dir, 'html_summary_dir'))
             fp_ids = qdb.util.insert_filepaths(
-                [(value, 'html_summary')], self.id, self.artifact_type,
-                "filepath")
+                filepaths, self.id, self.artifact_type, "filepath")
             sql = """INSERT INTO qiita.artifact_filepath
                         (artifact_id, filepath_id)
                      VALUES (%s, %s)"""
-            # We only inserted a single filepath, so using index 0
-            qdb.sql_connection.TRN.add(sql, [self.id, fp_ids[0]])
+            sql_args = [[self.id, id_] for id_ in fp_ids]
+            qdb.sql_connection.TRN.add(sql, sql_args, many=True)
             qdb.sql_connection.TRN.execute()
 
     @property
diff --git a/qiita_db/handlers/artifact.py b/qiita_db/handlers/artifact.py
@@ -131,8 +131,17 @@ def patch(self, artifact_id):
                 raise HTTPError(400, 'Incorrect path parameter value')
             else:
                 artifact = _get_artifact(artifact_id)
+
+                try:
+                    html_data = loads(req_value)
+                    html_fp = html_data['html']
+                    html_dir = html_data['dir']
+                except ValueError:
+                    html_fp = req_value
+                    html_dir = None
+
                 try:
-                    artifact.html_summary_fp = req_value
+                    artifact.set_html_summary(html_fp, html_dir)
                 except Exception as e:
                     raise HTTPError(500, str(e))
         else:
diff --git a/qiita_db/handlers/tests/test_artifact.py b/qiita_db/handlers/tests/test_artifact.py
@@ -9,9 +9,10 @@
 from unittest import main, TestCase
 from json import loads
 from functools import partial
-from os.path import join, exists
+from os.path import join, exists, isfile
 from os import close, remove
-from tempfile import mkstemp
+from shutil import rmtree
+from tempfile import mkstemp, mkdtemp
 from json import dumps
 
 from tornado.web import HTTPError
@@ -39,15 +40,16 @@ class ArtifactHandlerTests(OauthTestingBase):
     def setUp(self):
         super(ArtifactHandlerTests, self).setUp()
 
-        fd, self.html_fp = mkstemp(suffix=".html")
-        close(fd)
-        self._clean_up_files = [self.html_fp]
+        self._clean_up_files = []
 
     def tearDown(self):
         super(ArtifactHandlerTests, self).tearDown()
         for fp in self._clean_up_files:
             if exists(fp):
-                remove(fp)
+                if isfile(fp):
+                    remove(fp)
+                else:
+                    rmtree(fp)
 
     def test_get_artifact_does_not_exist(self):
         obs = self.get('/qiita_db/artifacts/100/', headers=self.header)
@@ -110,18 +112,40 @@ def test_get_artifact(self):
         self.assertEqual(obs, exp)
 
     def test_patch(self):
+        fd, html_fp = mkstemp(suffix=".html")
+        close(fd)
+        self._clean_up_files.append(html_fp)
+        # correct argument with a single HTML
+        arguments = {'op': 'add', 'path': '/html_summary/',
+                     'value': html_fp}
+        artifact = qdb.artifact.Artifact(1)
+        self.assertIsNone(artifact.html_summary_fp)
+        obs = self.patch('/qiita_db/artifacts/1/',
+                         headers=self.header,
+                         data=arguments)
+        self.assertEqual(obs.code, 200)
+        self.assertIsNotNone(artifact.html_summary_fp)
+
+        # Correct argument with an HMTL and a directory
+        fd, html_fp = mkstemp(suffix=".html")
+        close(fd)
+        self._clean_up_files.append(html_fp)
+        html_dir = mkdtemp()
+        self._clean_up_files.append(html_dir)
         arguments = {'op': 'add', 'path': '/html_summary/',
-                     'value': self.html_fp}
-        self.assertIsNone(qdb.artifact.Artifact(1).html_summary_fp)
+                     'value': dumps({'html': html_fp, 'dir': html_dir})}
         obs = self.patch('/qiita_db/artifacts/1/',
                          headers=self.header,
                          data=arguments)
         self.assertEqual(obs.code, 200)
-        self.assertIsNotNone(qdb.artifact.Artifact(1).html_summary_fp)
+        self.assertIsNotNone(artifact.html_summary_fp)
+        html_dir = [fp for _, fp, fp_type in artifact.filepaths
+                    if fp_type == 'html_summary_dir']
+        self.assertEqual(len(html_dir), 1)
 
         # Wrong operation
         arguments = {'op': 'wrong', 'path': '/html_summary/',
-                     'value': self.html_fp}
+                     'value': html_fp}
         obs = self.patch('/qiita_db/artifacts/1/',
                          headers=self.header,
                          data=arguments)
@@ -131,7 +155,7 @@ def test_patch(self):
 
         # Wrong path parameter
         arguments = {'op': 'add', 'path': '/wrong/',
-                     'value': self.html_fp}
+                     'value': html_fp}
         obs = self.patch('/qiita_db/artifacts/1/',
                          headers=self.header,
                          data=arguments)
@@ -140,7 +164,7 @@ def test_patch(self):
 
         # Wrong value parameter
         arguments = {'op': 'add', 'path': '/html_summary/',
-                     'value': self.html_fp}
+                     'value': html_fp}
         obs = self.patch('/qiita_db/artifacts/1/',
                          headers=self.header,
                          data=arguments)
diff --git a/qiita_db/support_files/patches/54.sql b/qiita_db/support_files/patches/54.sql
@@ -117,4 +117,7 @@ BEGIN
     INSERT INTO qiita.command_parameter (command_id, parameter_name, parameter_type, required, default_value)
         VALUES (baf_cmd_id, 'analysis', 'analysis', True, NULL),
                (baf_cmd_id, 'merge_dup_sample_ids', 'bool', False, 'False');
-END $do$
+END $do$;
+
+-- Add a new filepath type
+INSERT INTO qiita.filepath_type (filepath_type) VALUES ('html_summary_dir');
diff --git a/qiita_db/test/test_artifact.py b/qiita_db/test/test_artifact.py
@@ -7,7 +7,7 @@
 # -----------------------------------------------------------------------------
 
 from unittest import TestCase, main
-from tempfile import mkstemp
+from tempfile import mkstemp, mkdtemp
 from datetime import datetime
 from os import close, remove
 from os.path import exists, join, basename
@@ -1129,20 +1129,41 @@ def test_html_summary_setter(self):
         path_builder = partial(join, db_fastq_dir, str(a.id))
 
         # Check the setter works when the artifact does not have the summary
-        a.html_summary_fp = fp
+        a.set_html_summary(fp)
         exp1 = path_builder(basename(fp))
         self.assertEqual(a.html_summary_fp[1], exp1)
 
         fd, fp = mkstemp(suffix=".html")
         close(fd)
         self._clean_up_files.append(fp)
 
+        dp = mkdtemp()
+        self._clean_up_files.append(dp)
+
         # Check the setter works when the artifact already has a summary
-        a.html_summary_fp = fp
+        # and with a directory
+        a.set_html_summary(fp, support_dir=dp)
         exp2 = path_builder(basename(fp))
         self.assertEqual(a.html_summary_fp[1], exp2)
         self.assertFalse(exists(exp1))
 
+        # Check that the setter correctly removes the directory if a new
+        # summary is added. Magic number 0. There is only one html_summary_dir
+        # added on the previous test
+        old_dir_fp = [old_fp for _, old_fp, fptype in a.filepaths
+                      if fptype == 'html_summary_dir'][0]
+        fd, fp = mkstemp(suffix='.html')
+        close(fd)
+        self._clean_up_files.append(fp)
+        a.set_html_summary(fp)
+        exp3 = path_builder(basename(fp))
+        self.assertEqual(a.html_summary_fp[1], exp3)
+        self.assertFalse(exists(exp2))
+        self.assertFalse(exists(old_dir_fp))
+        summary_dir = [old_fp for _, old_fp, fptype in a.filepaths
+                       if fptype == 'html_summary_dir']
+        self.assertEqual(summary_dir, [])
+
     def test_descendants_with_jobs_one_element(self):
         artifact = qdb.artifact.Artifact.create(
             self.filepaths_root, 'FASTQ', prep_template=self.prep_template)
diff --git a/qiita_db/test/test_setup.py b/qiita_db/test/test_setup.py
@@ -39,7 +39,7 @@ def test_filepath(self):
         self.assertEqual(get_count("qiita.filepath"), 25)
 
     def test_filepath_type(self):
-        self.assertEqual(get_count("qiita.filepath_type"), 21)
+        self.assertEqual(get_count("qiita.filepath_type"), 22)
 
     def test_study_prep_template(self):
         self.assertEqual(get_count("qiita.study_prep_template"), 2)
diff --git a/qiita_pet/handlers/api_proxy/__init__.py b/qiita_pet/handlers/api_proxy/__init__.py
@@ -30,7 +30,7 @@
     study_tags_request)
 from .artifact import (artifact_graph_get_req, artifact_types_get_req,
                        artifact_post_req, artifact_get_req,
-                       artifact_status_put_req, artifact_delete_req,
+                       artifact_status_put_req,
                        artifact_summary_get_request, artifact_get_prep_req,
                        artifact_summary_post_request, artifact_patch_request)
 from .ontology import ontology_patch_handler
@@ -51,7 +51,7 @@
            'prep_template_delete_req', 'artifact_get_prep_req',
            'prep_template_graph_get_req', 'prep_template_filepaths_get_req',
            'artifact_get_req', 'artifact_status_put_req',
-           'artifact_delete_req', 'prep_template_get_req', 'study_delete_req',
+           'prep_template_get_req', 'study_delete_req',
            'study_prep_get_req', 'sample_template_get_req',
            'artifact_graph_get_req', 'artifact_types_get_req',
            'artifact_post_req',
diff --git a/qiita_pet/handlers/api_proxy/artifact.py b/qiita_pet/handlers/api_proxy/artifact.py
@@ -17,7 +17,7 @@
 from qiita_core.qiita_settings import qiita_config
 from qiita_pet.handlers.api_proxy.util import check_access, check_fp
 from qiita_ware.context import safe_submit
-from qiita_ware.dispatchable import (copy_raw_data, delete_artifact)
+from qiita_ware.dispatchable import copy_raw_data
 from qiita_db.artifact import Artifact
 from qiita_db.user import User
 from qiita_db.metadata_template.prep_template import PrepTemplate
@@ -506,37 +506,6 @@ def artifact_graph_get_req(artifact_id, direction, user_id):
             'message': ''}
 
 
-def artifact_delete_req(artifact_id, user_id):
-    """Deletes the artifact
-
-    Parameters
-    ----------
-    artifact_id : int
-        Artifact being acted on
-    user_id : str
-        The user requesting the action
-
-    Returns
-    -------
-    dict
-        Status of action, in the form {'status': status, 'message': msg}
-        status: status of the action, either success or error
-        message: Human readable message for status
-    """
-    pd = Artifact(int(artifact_id))
-    pt_id = pd.prep_templates[0].id
-    access_error = check_access(pd.study.id, user_id)
-    if access_error:
-        return access_error
-
-    job_id = safe_submit(user_id, delete_artifact, artifact_id)
-    r_client.set(PREP_TEMPLATE_KEY_FORMAT % pt_id,
-                 dumps({'job_id': job_id, 'is_qiita_job': False}))
-
-    return {'status': 'success',
-            'message': ''}
-
-
 def artifact_status_put_req(artifact_id, user_id, visibility):
     """Set the status of the artifact given
 
diff --git a/qiita_pet/handlers/api_proxy/tests/test_artifact.py b/qiita_pet/handlers/api_proxy/tests/test_artifact.py
@@ -27,7 +27,7 @@
 from qiita_db.exceptions import QiitaDBWarning
 from qiita_pet.handlers.api_proxy.artifact import (
     artifact_get_req, artifact_status_put_req, artifact_graph_get_req,
-    artifact_delete_req, artifact_types_get_req, artifact_post_req,
+    artifact_types_get_req, artifact_post_req,
     artifact_summary_get_request, artifact_summary_post_request,
     artifact_patch_request, artifact_get_prep_req)
 
@@ -234,7 +234,7 @@ def test_artifact_summary_get_request(self):
         with open(fp, 'w') as f:
             f.write('<b>HTML TEST - not important</b>\n')
         a = Artifact(1)
-        a.html_summary_fp = fp
+        a.set_html_summary(fp)
         self._files_to_remove.extend([fp, a.html_summary_fp[1]])
         exp_files.append(
             (a.html_summary_fp[0],
@@ -393,22 +393,6 @@ def test_artifact_patch_request_errors(self):
                           'operations: replace'}
         self.assertEqual(obs, exp)
 
-    def test_artifact_delete_req(self):
-        obs = artifact_delete_req(self.artifact.id, 'test@foo.bar')
-        exp = {'status': 'success', 'message': ''}
-        self.assertEqual(obs, exp)
-
-        # This is needed so the clean up works - this is a distributed system
-        # so we need to make sure that all processes are done before we reset
-        # the test database
-        wait_for_prep_information_job(1)
-
-    def test_artifact_delete_req_no_access(self):
-        obs = artifact_delete_req(self.artifact.id, 'demo@microbio.me')
-        exp = {'status': 'error',
-               'message': 'User does not have access to study'}
-        self.assertEqual(obs, exp)
-
     def test_artifact_get_prep_req(self):
         obs = artifact_get_prep_req('test@foo.bar', [4])
         exp = {'status': 'success', 'msg': '', 'data': {
diff --git a/qiita_pet/handlers/artifact_handlers/tests/test_base_handlers.py b/qiita_pet/handlers/artifact_handlers/tests/test_base_handlers.py
@@ -145,7 +145,7 @@ def test_artifact_summary_get_request(self):
         with open(fp, 'w') as f:
             f.write('<b>HTML TEST - not important</b>\n')
         a = Artifact(1)
-        a.html_summary_fp = fp
+        a.set_html_summary(fp)
         self._files_to_remove.extend([fp, a.html_summary_fp[1]])
         exp_files.append(
             (a.html_summary_fp[0],
@@ -375,7 +375,7 @@ def test_get_artifact_summary_handler(self):
         with open(fp, 'w') as f:
             f.write('<b>HTML TEST - not important</b>\n')
         a = Artifact(1)
-        a.html_summary_fp = fp
+        a.set_html_summary(fp)
         self._files_to_remove.extend([fp, a.html_summary_fp[1]])
 
         summary = relpath(a.html_summary_fp[1], qiita_config.base_data_dir)
diff --git a/qiita_pet/handlers/study_handlers/tests/test_artifact.py b/qiita_pet/handlers/study_handlers/tests/test_artifact.py
@@ -123,6 +123,8 @@ def test_post_artifact(self):
             'import-artifact': ''}
         response = self.post('/study/new_artifact/', args)
         self.assertEqual(response.code, 200)
+        self.assertEqual(loads(response.body),
+                         {'status': 'success', 'message': ''})
 
         # make sure new artifact created
         wait_for_prep_information_job(self.prep.id)
diff --git a/qiita_pet/handlers/study_handlers/tests/test_sample_template.py b/qiita_pet/handlers/study_handlers/tests/test_sample_template.py
