delete sample template #1059
delete sample template #1059
Conversation
| queue, | ||
| "DROP TABLE IF EXISTS qiita.{0}".format(table_name)) | ||
|
|
||
| conn_handler.add_to_queue( |
There was a problem hiding this comment.
Drop the table twice? Also it should probably just be drop table, that way we get an error if the table doesn't exist. This will point out that the DB is in an inconsistent state.
|
Couple comments. Also there is no test code for this delete function. |
|
Comments addressed on last commit but not sure what you mean about tests, can you clarify? Note that the base-delete is tested both in there and the sample-template-delete. |
|
I meant qiita_pet test, sorry for not being clear. |
|
Added test for this and other delete funcs. Ready for another pass.
|
| response = self.post('/study/description/1', | ||
| {'sample_template_id': 1, | ||
| 'action': 'delete_sample_template'}) | ||
| self.assertEqual(response.code, 200) |
There was a problem hiding this comment.
Can you also make sure that the sample template is actually deleted here? Same goes for all other delete tests. Just want to make sure the functionality is actually happening along with the page loading fine.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh no, this isn't to make sure the delete function works, just that the delete page actually calls delete and the items are actually deleted when you hit the buttons on the page.
There was a problem hiding this comment.
There was a problem hiding this comment.
Just because the delete function works in qiita_db, doesn't mean the call works in qiita_pet. The delete function may change, breaking qiita_pet, or some other thing may change. If you test in qiita pet that the deletion actually happens, you know the functionality always works. As an example, in test_delete_raw_data, you can make sure that the QiitaDBIDError is raised when instantiating RawData(1) to make sure it was deleted by the page call.
There was a problem hiding this comment.
Concrete example here Where the database change called for in the page is explicitly tested that it happens.
|
One comment. |
|
I have added the extra tests but while doing this I realized that it will be better to move the delete method from base_metadata_template.py to sample_template.py as the error raised by the method is specific to sample template. Note that the prep template has its own method. Ready for another review. |
|
That is the easy solution for the delete. I will refactor it as a part of my work to get all changes from #1021 in the system... |
|
Cool, can you also review/merge this PR?
|
| @@ -382,5 +382,57 @@ class TestEBISubmitHandler(TestHandlerBase): | |||
| pass | |||
|
|
|||
|
|
|||
| class TestDelete(TestHandlerBase): | |||
There was a problem hiding this comment.
I think this tests should include database = True; since they can potentially modify the database, in case of malfunction; making a lot of tests to fail and harder to find the original problem.
|
IMOO, it is very misleading that the button for removing the sample template is next to "Study information", since we are not removing the study information. Also it is the only tab on that group that has the
|
|
👍 @squirrelo is your comment about the tests solved? If so, can you merge once the tests pass? |
No description provided.