Transfer update delete templates #2274

josenavas · 2017-09-06T15:21:42Z

Port all the remaining functionality from moi to the internal Qiita plugins.

Also updates the interface to reflect these changes.

codecov-io · 2017-09-07T20:36:36Z

Codecov Report

Merging #2274 into remove-moi will decrease coverage by 2.8%.
The diff coverage is 86.88%.

@@              Coverage Diff               @@
##           remove-moi    #2274      +/-   ##
==============================================
- Coverage       96.59%   93.78%   -2.81%     
==============================================
  Files              65      161      +96     
  Lines           12437    18249    +5812     
==============================================
+ Hits            12013    17115    +5102     
- Misses            424     1134     +710

Impacted Files	Coverage Δ
qiita_pet/test/test_prep_template.py	`97.87% <ø> (ø)`
qiita_db/test/test_commands.py	`99.6% <ø> (+0.28%)`	⬆️
qiita_db/commands.py	`96.55% <ø> (-0.48%)`	⬇️
...t/handlers/api_proxy/tests/test_sample_template.py	`98.55% <100%> (ø)`
...pet/handlers/api_proxy/tests/test_prep_template.py	`98.68% <100%> (ø)`
qiita_db/handlers/processing_job.py	`96.92% <100%> (+7.06%)`	⬆️
qiita_pet/test/tornado_test_base.py	`91.22% <100%> (ø)`
...dlers/study_handlers/tests/test_sample_template.py	`97.5% <100%> (ø)`
qiita_db/environment_manager.py	`45.74% <100%> (+0.29%)`	⬆️
qiita_db/processing_job.py	`92.9% <100%> (+1.15%)`	⬆️
... and 102 more

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 dc7f78c...eb2d314. Read the comment docs.

antgonza

2 things and:

need to add a r_client.flushdb() to the qiita-env clean_test command cause if not we end up with all the all keys, which make the system behave pretty oddly
we discussed that the full traceback of the failing job will be remove and only the main message was going to be displayed. For example, if the artifact has been analyzed we currently display: Error executing private task: ['Traceback (most recent call last):\n', ' File "/Users/antoniog/svn_programs/qiita/qiita_ware/private_plugin.py", line 333, in private_task\n TASK_DICT[task_name](job)\n', ' File "/Users/antoniog/svn_programs/qiita/qiita_ware/private_plugin.py", line 131, in delete_artifact\n qdb.artifact.Artifact.delete(artifact_id)\n', ' File "/Users/antoniog/svn_programs/qiita/qiita_db/artifact.py", line 510, in delete\n % \', \'.join([str(c.id) for c in instance.children]))\n', 'QiitaDBArtifactDeletionError: Cannot delete artifact 2: it has children: 4, 5, 6\n'], where ideally we will only show: Cannot delete artifact 2: it has children: 4, 5, 6.

antgonza · 2017-09-07T20:51:19Z

qiita_db/test/test_commands.py

@@ -216,7 +214,7 @@ def tearDown(self):
    def test_load_parameters_from_cmd_error(self):
        with self.assertRaises(qdb.exceptions.QiitaDBUnknownIDError):
            qdb.commands.load_parameters_from_cmd(
-                "test", self.fp, 20)
+                "test", self.fp, 20000)


Is this change necessary?

Yes, the command 20 exists now.

antgonza · 2017-09-07T20:56:58Z

qiita_ware/test/test_private_plugin.py

+        self.assertIn('Unknown value "unknown". Choose between "samples" '
+                      'and "columns"', job.log.msg)
+
+    def test_complete_job(self):


Any reason to have all these tests in a single test vs multiple or a new class?

Consistency with the rest of the tests in this class. A new class will trigger an unnecessary reset of the DB. If you feel strong on splitting in multiple tests I can do that.

Nop, just wondering, could you add some text on the top of the test, explaining this?

josenavas · 2017-09-08T15:03:30Z

Changes made. Now the errors are shown like:

Error (log id: 3): Prep template 1 already has an artifact associated

where the number in the log id is another entry in the LogEntry table that contains the entire traceback and can be used by developers.

antgonza · 2017-09-08T15:05:53Z

Thanks for the changes. Found one more while trying to delete a prep with artifacts: "Couldn't remove prep template: Cannot remove prep template 1 because it has an artifact associated with it". Note that trying to erase a sample info file is fine.

josenavas · 2017-09-08T15:06:41Z

I don't understand what is the issue with that comment?

antgonza · 2017-09-08T15:08:39Z

That the message is confusing: "Couldn't remove prep template: Cannot remove prep template 1 because it has an artifact associated with it" should only be "Cannot remove prep template 1 because it has an artifact associated with it"

josenavas · 2017-09-08T15:14:06Z

oh, now makes sense :-) changed

ElDeveloper

A handful of minor comments, otherwise looks good!

ElDeveloper · 2017-09-08T16:35:34Z

qiita_db/processing_job.py

+            self._set_status('queued')
+            # At this point we are going to involve other processes. We need
+            # to commit the changes to the DB or the other processes will not
+            # see this changes


this -> these

ElDeveloper · 2017-09-08T16:39:28Z

qiita_db/sql_connection.py

-        raise ValueError(
-            "Error running SQL: %s. MSG: %s\n" % (
-                errorcodes.lookup(error.pgcode), error.message))
+        try:


This block seems rather confusing, you try to find the error code, and check if that raises a KeyError, if it doesn't you show the error and the message, and if it does you just show the message. Are pgcodes not guaranteed to be in the errorcodes?

I found one case in which pgcodes was not in the error codes. It was mainly due to a bad usage of psycopg2 and the multiprocessing module. That is now fixed, but without this try/except I was unable to see the error message. Thus, in general they will be found, but just in case that in the future we find another issue, this will allow us to debug.

Wow, that's very strange!

ElDeveloper · 2017-09-08T16:44:31Z

qiita_ware/private_plugin.py

+            st.extend_and_update(df)
+            remove(fp)
+
+            # Join all the warning messages into one.NOte that this info


one.NOte -> one. Note

ElDeveloper · 2017-09-08T16:49:30Z

qiita_db/sql_connection.py

@@ -937,3 +942,12 @@ def add_post_rollback_func(self, func, *args, **kwargs):

 # Singleton pattern, create the transaction for the entire system
 TRN = Transaction()
+
+
+def create_new_transacion():


transacion -> transaction

josenavas · 2017-09-08T17:04:49Z

Thanks @ElDeveloper - comments should be addressed now!

@josenavas

* replace printable for try/except utf-8 * add sleeps for tests * moving wait_for_prep_information_job * just leaving the changes on this pr * Fixing tests * Reverting changes * dist: precise * addressing @josenavas comments * fix-2212 * Adding branch description to Contributing (#2256) * Adding branch description * Adding spaces * Adding timeline * upgrading software * mv r_client to qiita * from redis import Redis * rm moi and ipython * Move qiita_db/private.py -> qiita_ware/private_plugin.py * _system_call as system_call * flake8 * rm create_raw_data * Transferring VAMPS submission to internal job * rm other unnecessary files * addressing @josenavas comment * Fixing merge conflicts * Adding tests to private plugin * rm wrapper.py * Removing import * Fixing import * Modifying GUI to use the plugin * moving qiita pet in .travis.yml * Transfer submit to vamps (#2265) * Move qiita_db/private.py -> qiita_ware/private_plugin.py * Transferring VAMPS submission to internal job * Fixing import * some other .travis.yml fixes * addressing @josenavas comment * Adding success test * adding some methods * flake8 * Transfer copy raw data (#2267) * Move qiita_db/private.py -> qiita_ware/private_plugin.py * Transferring VAMPS submission to internal job * Fixing merge conflicts * Adding tests to private plugin * Removing import * Fixing import * Modifying GUI to use the plugin * Adding success test * change imports * adding delete_artifact and create_sample_template * fixing errors and gui * fix delete error * ENH: Make jobs list modal With some recent changes to the position of the navigation bar, the header of the jobs list is cut from screen. With this patch in place, the jobs list will be shown as a modal window, so that won't be a problem anymore. * Transfer update delete templates (#2274) * Moving update_sample_template * Transfer update_sample_template * Porting update prep template * Moving delete sample or column * Removing tests * Removing dispatchable and its tests * Updating interface to use the new functionality' * Adapting the prep template GUI * Submitting jobs * Fixing tests * Removing qiita_ware/context.py * flake8ing * Fixing _system_call * Safeguarding the call to rollback * Unmasking more errors * Forcing different connections on different processes * Moving job completion to internal plugin structure * Removing unused code * Forcing the creation of a new transaction on the jobs * Fixing tests * forcing the commit * Fixing all tests * Addressing @antgonza's comments * Addressing @antgonza's comment * Addressing @ElDeveloper's comments * BUG: Fix job updates * Fixing the redis DB (#2277) * Fixing the redis DB * Addressing @antgonza's comments * redbiom install * redbiom to install_requires * mv moi-ws to qiita_websocket * init commit * addressing @wasade comment * rm webdis.log * cleaning code for initial review * install latest redbiom * fix test * ENH: Change phrasing of upload text (#2281) * ENH: Change phrasing of upload text As per a user's request. * Fix typo in docs Fixes #2259 * addressing @wasade comments and adding other tests * flake8 * addressing @josenavas comments * fix #2258 * fix #2258 * fix #858 (#2286) * addressing @ElDeveloper and @josenavas comments * @ElDeveloper :| * redbiom now adds per sample studies to analysis * jobs-list-as-modal * addresssing @josenavas comments * rm () from update_processing_job_data * add prints to review errors * rm prints * Fix 2190 (#2292) * Adding patch to add the 'name' parameter to all validate commands * Adding the parameter 'name' automatically and adding a test for it * Removing extra blank line * Adding dflt value to artifact name * Edited the wrong file * Adding user defined name at creation time * Fixing test * add is_from_analysis to artifact_handlers (#2293) * add is_from_analysis to artifact_handlers * fix test_post_metadata * fix error * WIP: rm sudo from travis * adding sed for config file * fix sed * rm & * cat redis.conf * using local redis.conf * # protected-mode yes * # supervised no * redis-server --port 7777 & * Fix 1293 (#2291) * fix #1293 * flake8 * fix errors * addressing @ElDeveloper comments * rm redis.conf * fix awaiting_approval list bug * Fix #2276 (#2294) * Fix #2276 * Factoring out generate nginx directory file list * Factoring out the nginx file list writing * Factoring out generating the file list of an artifact * Factoring out the header setting * Addressing @antgonza's comment * Addressing @wasade's comments

@josenavas

* replace printable for try/except utf-8 * add sleeps for tests * moving wait_for_prep_information_job * just leaving the changes on this pr * Fixing tests * Reverting changes * dist: precise * addressing @josenavas comments * fix-2212 * Adding branch description to Contributing (#2256) * Adding branch description * Adding spaces * Adding timeline * upgrading software * mv r_client to qiita * from redis import Redis * rm moi and ipython * Move qiita_db/private.py -> qiita_ware/private_plugin.py * _system_call as system_call * flake8 * rm create_raw_data * Transferring VAMPS submission to internal job * rm other unnecessary files * addressing @josenavas comment * Fixing merge conflicts * Adding tests to private plugin * rm wrapper.py * Removing import * Fixing import * Modifying GUI to use the plugin * moving qiita pet in .travis.yml * Transfer submit to vamps (#2265) * Move qiita_db/private.py -> qiita_ware/private_plugin.py * Transferring VAMPS submission to internal job * Fixing import * some other .travis.yml fixes * addressing @josenavas comment * Adding success test * adding some methods * flake8 * Transfer copy raw data (#2267) * Move qiita_db/private.py -> qiita_ware/private_plugin.py * Transferring VAMPS submission to internal job * Fixing merge conflicts * Adding tests to private plugin * Removing import * Fixing import * Modifying GUI to use the plugin * Adding success test * change imports * adding delete_artifact and create_sample_template * fixing errors and gui * fix delete error * ENH: Make jobs list modal With some recent changes to the position of the navigation bar, the header of the jobs list is cut from screen. With this patch in place, the jobs list will be shown as a modal window, so that won't be a problem anymore. * Transfer update delete templates (#2274) * Moving update_sample_template * Transfer update_sample_template * Porting update prep template * Moving delete sample or column * Removing tests * Removing dispatchable and its tests * Updating interface to use the new functionality' * Adapting the prep template GUI * Submitting jobs * Fixing tests * Removing qiita_ware/context.py * flake8ing * Fixing _system_call * Safeguarding the call to rollback * Unmasking more errors * Forcing different connections on different processes * Moving job completion to internal plugin structure * Removing unused code * Forcing the creation of a new transaction on the jobs * Fixing tests * forcing the commit * Fixing all tests * Addressing @antgonza's comments * Addressing @antgonza's comment * Addressing @ElDeveloper's comments * BUG: Fix job updates * Fixing the redis DB (#2277) * Fixing the redis DB * Addressing @antgonza's comments * redbiom install * redbiom to install_requires * mv moi-ws to qiita_websocket * init commit * addressing @wasade comment * rm webdis.log * cleaning code for initial review * install latest redbiom * fix test * ENH: Change phrasing of upload text (#2281) * ENH: Change phrasing of upload text As per a user's request. * Fix typo in docs Fixes #2259 * addressing @wasade comments and adding other tests * flake8 * addressing @josenavas comments * fix #2258 * fix #2258 * fix #858 (#2286) * addressing @ElDeveloper and @josenavas comments * @ElDeveloper :| * redbiom now adds per sample studies to analysis * jobs-list-as-modal * addresssing @josenavas comments * rm () from update_processing_job_data * add prints to review errors * rm prints * Fix 2190 (#2292) * Adding patch to add the 'name' parameter to all validate commands * Adding the parameter 'name' automatically and adding a test for it * Removing extra blank line * Adding dflt value to artifact name * Edited the wrong file * Adding user defined name at creation time * Fixing test * add is_from_analysis to artifact_handlers (#2293) * add is_from_analysis to artifact_handlers * fix test_post_metadata * fix error * WIP: rm sudo from travis * adding sed for config file * fix sed * rm & * cat redis.conf * using local redis.conf * # protected-mode yes * # supervised no * redis-server --port 7777 & * Fix 1293 (#2291) * fix #1293 * flake8 * fix errors * addressing @ElDeveloper comments * rm redis.conf * fix awaiting_approval list bug * Fix #2276 (#2294) * Fix #2276 * Factoring out generate nginx directory file list * Factoring out the nginx file list writing * Factoring out generating the file list of an artifact * Factoring out the header setting * Addressing @antgonza's comment * Addressing @wasade's comments * Fixing patch * fix error * Fixing failing test * fix #2214 * addressing @josenavas comment * fix #2209 * fix #2331 * fix #2326 (#2328) * fix #2326 * addressing @ElDeveloper comment * fix #2226 (#2330) * fix #2226 * addressing @ElDeveloper comment * fix #2336 * fix #2316 * Fixes 2269 * addressing @josenavas comments * Fixes 2038 (#2349) * fixes #2038 * Adding test * Trying to debug * Checking value * More debugging * Undo changes * Fixing failure * fix #2333 * addressing @josenavas * fixes #2245 (#2350) * fixes #2245 * Addressing @antgonza's comments * Fixing test * Fixing Qiita installation (#2362) * Sync-ing with master (#2367) * fix calls to system_call and ebi submissions * fixing errors * fix if state == submitting: * just raise error * EBISubmissionError -> ComputeError * fix #2084 (#2365) * fix #2125 (#2366) * fix #2125 * fix error * fix #2364 * fix #1812 * add tests * flake8 * populating ProcessingJob.create True * fix more errors * addressing comments from @josenavas and @stephanieorch * mv ProcessingJob.create True around * Partial #2237 (#2368) * fix calls to system_call and ebi submissions * fixing errors * fix if state == submitting: * just raise error * EBISubmissionError -> ComputeError * Sorting values * Case insensitive sorting * Addressing @ElDeveloper's comments * fix-1591 (#2370) * fix-1591 * removing warning ATTN @josenavas, fix tests * addressing @ElDeveloper and @josenavas comments * Fix 2230 (#2372) * fix calls to system_call and ebi submissions * fixing errors * fix if state == submitting: * just raise error * EBISubmissionError -> ComputeError * Fix #2230 solved using modal in order to prevent large file download * Function moved into in order to keep order. Some details fixed. * addressing @wasade and @ElDeveloper comments * fixing errors * fix GUI and erros * addressing @ElDeveloper comments * rm artifacts from parameters listing * fix flake8 * fix ilike quote params * addressing @josenavas comment and adding test for job without children * fix errors * addressing @josenavas comment * Fixing network labels (#2376) * Fixing network labels * Fixing error * Update redbiom.html * Update redbiom.html * Update redbiom.html * Patch 61 - transfer all parameters to str (#2379) * Patch 61 - transfer all parameters to str * Fixing errors * rm lower from redbiom * fixing smal details and adding emp_release1 * fixing

josenavas added 11 commits August 31, 2017 14:38

Moving update_sample_template

9481d42

Transfer update_sample_template

998197e

Porting update prep template

656ac7f

Moving delete sample or column

37a5a74

Removing tests

4a6b3b2

Solving merge conflicts

0aaa53f

Removing dispatchable and its tests

6f52ebd

Updating interface to use the new functionality'

c2e7d8b

Adapting the prep template GUI

c450079

Submitting jobs

22ce457

Fixing tests

d2259b0

josenavas mentioned this pull request Sep 6, 2017

Remove moi #2260

Merged

21 tasks

josenavas added 12 commits September 6, 2017 08:26

Removing qiita_ware/context.py

6ec24d9

flake8ing

e9901ed

Fixing _system_call

2019e71

Safeguarding the call to rollback

830312d

Unmasking more errors

3f4cdcd

Forcing different connections on different processes

d9d51e6

Moving job completion to internal plugin structure

4339eda

Removing unused code

fa391d0

Forcing the creation of a new transaction on the jobs

dd74e11

Fixing tests

407fd8b

forcing the commit

966eb0f

Fixing all tests

1a4026c

antgonza requested changes Sep 7, 2017

View reviewed changes

Addressing @antgonza's comments

dc960a3

Addressing @antgonza's comment

63519f6

antgonza approved these changes Sep 8, 2017

View reviewed changes

ElDeveloper approved these changes Sep 8, 2017

View reviewed changes

Addressing @ElDeveloper's comments

eb2d314

antgonza merged commit 22d52f0 into qiita-spots:remove-moi Sep 8, 2017

josenavas mentioned this pull request Sep 19, 2017

Release candidate (#2298) #2299

Merged

Transfer update delete templates #2274

Transfer update delete templates #2274

Uh oh!

Conversation

josenavas commented Sep 6, 2017

Uh oh!

codecov-io commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

antgonza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Sep 8, 2017

Uh oh!

antgonza commented Sep 8, 2017

Uh oh!

josenavas commented Sep 8, 2017

Uh oh!

antgonza commented Sep 8, 2017

Uh oh!

josenavas commented Sep 8, 2017

Uh oh!

ElDeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Sep 8, 2017

Uh oh!

Uh oh!

codecov-io commented Sep 7, 2017 •

edited

Loading