Delete prep template #850

antgonza · 2015-02-12T19:31:30Z

#842 should be merged first.

coveralls · 2015-02-12T21:47:51Z

Coverage decreased (-0.26%) to 78.33% when pulling 1b36d1e on antgonza:delete_prep_template into 1ca033a on biocore:master.

ElDeveloper · 2015-02-12T23:38:09Z

I think you need to pull from upstream/master, but I could be wrong.

On (Feb-12-15|11:31), Antonio Gonzalez wrote:

#842 should be merged first.
You can view, comment on, or merge this pull request online at:

#850

-- Commit Summary --

deletes raw data

Merge branch 'master' of https://github.com/biocore/qiita into delete-raw-data

addressing @josenavas, @ElDeveloper and @squirrelo comments

flake8

addressing @josenavas and @ElDeveloper comments

Merge branch 'master' of https://github.com/biocore/qiita into delete-raw-data

fixing sql and test

delete prep template via GUI

-- File Changes --
M qiita_db/data.py (98)
M qiita_db/test/test_data.py (37)
M qiita_pet/handlers/study_handlers/description_handlers.py (63)
M qiita_pet/templates/study_description.html (38)
M qiita_pet/templates/study_description_templates/prep_template_panel.html (5)
M qiita_pet/templates/study_description_templates/raw_data_tab.html (5)
-- Patch Links --

https://github.com/biocore/qiita/pull/850.patch
https://github.com/biocore/qiita/pull/850.diff

Reply to this email directly or view it on GitHub:
#850

…_prep_template

antgonza · 2015-02-13T13:45:14Z

Done.

coveralls · 2015-02-13T13:59:59Z

Coverage decreased (-0.16%) to 78.33% when pulling 8608e82 on antgonza:delete_prep_template into 3ff89b8 on biocore:master.

josenavas · 2015-02-13T17:39:29Z

qiita_pet/handlers/study_handlers/description_handlers.py

+            PrepTemplate.delete(prep_template_id)
+            msg = ("Prep template %d has been deleted" % prep_template_id)
+            msg_level = "success"
+            tab = 'study_information_tab'


Probably better to redirect in either case to the raw_data_tab? So from the user point of view the only change on the screen is that the prep template has been remove, rather than move to the RawData tab after the prep template is removed (he probably wants to add another prep template just after removing one).

josenavas · 2015-02-13T17:49:27Z

qiita_pet/handlers/study_handlers/description_handlers.py

+            msg = ("Couldn't remove prep template: %s" % str(e))
+            msg_level = "danger"
+
+        callback((msg, msg_level, 'raw_data_tab',


This now is broken. If the removal worked successfully, the PrepTemplate with id prep_template_id no longer works. You just need to move the query to PrepTemplate(prep_template_id).raw_data before the try statement

Duh!! Thanks for catching that.

josenavas · 2015-02-13T17:59:11Z

qiita_pet/handlers/study_handlers/description_handlers.py

@@ -613,6 +613,33 @@ def delete_raw_data(self, study, user, callback):

        callback((msg, msg_level, tab, tab_id, None))

+    def delete_prep_template(self, study, user, callback):


This is still broken... (sorry) the actual code should be this one:

prep_template_id = int(self.get_argument('prep_template_id')) tab_id = PrepTemplate(prep_template_id).raw_data try: PrepTemplate.delete(prep_template_id) msg = ("Prep template %d has been deleted" % prep_template_id) msg_level = "success" prep_id = None except Exception as e: msg = ("Couldn't remove prep template: %s" % str(e)) msg_level = "danger" prep_id = prep_template_id callback((msg, msg_level, 'raw_data_tab', tab_id, prep_template_id))

Got it, I think you are right ...

coveralls · 2015-02-13T18:02:56Z

Coverage decreased (-0.11%) to 78.39% when pulling 36f0628 on antgonza:delete_prep_template into 3ff89b8 on biocore:master.

josenavas · 2015-02-13T18:05:54Z

I found another issue. Part of it is out of the scope of this PR but I think is easy enough to fix here too.

The removal of a raw data/prep template can be done independently of the study status. However, if a study is private/public raw data and prep templates can be removed. I don't think this should be allowed, at least if the study is public. If the study is private, however, I can see some discussion. But I would advocate that the user is adding metadata (prep template) so it should be checked...

coveralls · 2015-02-13T18:09:42Z

Coverage decreased (-0.14%) to 78.36% when pulling 1742f8f on antgonza:delete_prep_template into 3ff89b8 on biocore:master.

antgonza · 2015-02-13T18:10:33Z

I see you point but I do not think that's possible because to be able to make it public you need processed object and if you have one, you can not remove the preprocessed data, and if you ..., get it? Either way, I can add that test, but depending where we do the validation (qiita_db or qiita_pet), we will need to check the study status. I will like to have it in qiita_db but that will require to pass/create an study object within those objects. What do you think?

josenavas · 2015-02-13T18:20:05Z

You can have a processed object from one raw data, but not from the other one. And you still will be able to delete that other one... Is just the idea that our "public" data should not be mutable in an easy way (or at least not by a normal user, just by an admin...).

I don't have strong feelings on where to put the check. Although it is a pretty light check on qiita_db that will not require interaction with the server, as the the study_status property is already used extensively in the study_description page. you can potentially propagate that value through the calls to the uimodules, so less DB interactions are needed.

(Also if you feel like it, adding a double check on qiita_db never hurts!)

josenavas · 2015-02-13T18:33:45Z

qiita_pet/handlers/study_handlers/description_handlers.py

+            msg_level = "danger"
+            prep_id = prep_template_id
+
+        callback((msg, msg_level, 'raw_data_tab', tab_id, None))


You should pass prep_id instead of None here.

coveralls · 2015-02-13T20:53:33Z

Coverage decreased (-0.17%) to 78.33% when pulling 8574c3a on antgonza:delete_prep_template into 3ff89b8 on biocore:master.

antgonza · 2015-02-13T21:51:17Z

@josenavas and I discussed his comment offline and realized that we have a bigger issue and it's described here: #855

josenavas · 2015-02-13T22:06:56Z

Given that we have created another issue to keep track of my concern, I would say that this PR is good to go. So if another developer can review this and agrees with the code, feel free to merge!

ElDeveloper · 2015-02-13T22:14:38Z

qiita_pet/handlers/study_handlers/description_handlers.py

+            is done
+        """
+        prep_template_id = int(self.get_argument('prep_template_id'))
+        tab_id = PrepTemplate(prep_template_id).raw_data


It seems like tab_id could easily be replaced by prep_id. Plus I don't understand why the name of this variable is tab...

is not the prep_id. Is the raw_data_id. Also the name is tab because the callback function expect a message, the message level, the name of tab group to show, the id in that group (tab_id) and the id of the sub_tab in case that the previous ID points to another tab group.

Oh, ok. In any case using a single variable should work (I think).

On (Feb-13-15|14:17), josenavas wrote:

@@ -613,6 +613,34 @@ def delete_raw_data(self, study, user, callback):

callback((msg, msg_level, tab, tab_id, None))

def delete_prep_template(self, study, user, callback):

"""Delete the selected prep template

Parameters

study : Study

The current study object

user : User

The current user object

callback : function

The callback function to call with the results once the processing

is done

"""

prep_template_id = int(self.get_argument('prep_template_id'))

tab_id = PrepTemplate(prep_template_id).raw_data

is not the prep_id. Is the raw_data_id. Also the name is tab because the callback function expect a message, the message level, the name of tab group to show, the id in that group (tab_id) and the id of the sub_tab in case that the previous ID points to another tab group.

Reply to this email directly or view it on GitHub:
https://github.com/biocore/qiita/pull/850/files#r24701698

ElDeveloper · 2015-02-13T22:35:12Z

Just one comment, looks good otherwise.

antgonza · 2015-02-14T14:38:01Z

OK, the latest push should address your two comments. Note that I completely deactivated the collapse on the prep templates cause (1) I spent a lot of time trying to figure out how to prevent it when you cancel the erase and (2) cause this is really confusing to users and the reason I deactivated by default in the first place.

coveralls · 2015-02-14T15:03:38Z

Coverage decreased (-0.16%) to 78.34% when pulling 5bf3134 on antgonza:delete_prep_template into 3ff89b8 on biocore:master.

ElDeveloper · 2015-02-14T19:55:23Z

Thanks working in that, but now I can't click the delete button. On the
bright side, the accordion no longer collapses.

On (Feb-14-15| 6:38), Antonio Gonzalez wrote:

OK, the latest push should address your two comments. Note that I completely deactivated the collapse on the prep templates cause (1) I spent a lot of time trying to figure out how to prevent it when you cancel the erase and (2) cause this is really confusing to users and the reason I deactivated by default in the first place.

Reply to this email directly or view it on GitHub:
#850 (comment)

antgonza · 2015-02-15T04:02:41Z

The tests passed but not sure why the PR wasn't updated.

Delete prep template

ElDeveloper · 2015-02-15T04:07:20Z

Tested it and it looks good!

antgonza added 2 commits February 12, 2015 12:30

delete prep template via GUI

fba813b

fixing flake8

1b36d1e

Merge branch 'master' of https://github.com/biocore/qiita into delete…

8608e82

…_prep_template

josenavas reviewed Feb 13, 2015
View reviewed changes

comment from @josenavas

9c7d91f

josenavas reviewed Feb 13, 2015
View reviewed changes

:(

36f0628

josenavas reviewed Feb 13, 2015
View reviewed changes

:)

1742f8f

josenavas reviewed Feb 13, 2015
View reviewed changes

antgonza added 2 commits February 13, 2015 11:58

thanks @josenavas

fe74ae2

:'(

8574c3a

antgonza mentioned this pull request Feb 13, 2015

shared status shouldn't be by study but by processed data #855

Closed

ElDeveloper reviewed Feb 13, 2015
View reviewed changes

adding comments from @ElDeveloper

5bf3134

antgonza added 2 commits February 14, 2015 14:13

thanks @ElDeveloper

d5e26b1

:|

5de0e08

ElDeveloper added a commit that referenced this pull request Feb 15, 2015

Merge pull request #850 from antgonza/delete_prep_template

d398414

Delete prep template

ElDeveloper merged commit d398414 into qiita-spots:master Feb 15, 2015

antgonza mentioned this pull request Feb 19, 2015

Prep templates not deleting correctly #781

Closed

		@@ -613,6 +613,33 @@ def delete_raw_data(self, study, user, callback):

		callback((msg, msg_level, tab, tab_id, None))

		def delete_prep_template(self, study, user, callback):

Delete prep template #850

Delete prep template #850

Uh oh!

Conversation

antgonza commented Feb 12, 2015

Uh oh!

coveralls commented Feb 12, 2015

Uh oh!

ElDeveloper commented Feb 12, 2015

Uh oh!

antgonza commented Feb 13, 2015

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

josenavas Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

antgonza Feb 13, 2015 via email

Choose a reason for hiding this comment

Uh oh!

josenavas Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

antgonza Feb 13, 2015 via email

Choose a reason for hiding this comment

Uh oh!

josenavas Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

antgonza Feb 13, 2015 via email

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

josenavas commented Feb 13, 2015

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

antgonza commented Feb 13, 2015 via email

Uh oh!

josenavas commented Feb 13, 2015

Uh oh!

josenavas Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 13, 2015

Uh oh!

antgonza commented Feb 13, 2015

Uh oh!

josenavas commented Feb 13, 2015

Uh oh!

ElDeveloper Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

josenavas Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

ElDeveloper Feb 13, 2015

Choose a reason for hiding this comment

Uh oh!

ElDeveloper commented Feb 13, 2015

Uh oh!

antgonza commented Feb 14, 2015

Uh oh!

coveralls commented Feb 14, 2015

Uh oh!

ElDeveloper commented Feb 14, 2015

Uh oh!

antgonza commented Feb 15, 2015

Uh oh!

ElDeveloper commented Feb 15, 2015

Uh oh!

Uh oh!