-
Notifications
You must be signed in to change notification settings - Fork 80
Delete preprocess data #1042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delete preprocess data #1042
Changes from 5 commits
16ff388
656128f
f9c0ca1
1bb2c91
3974fb7
a74fd0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -478,6 +478,44 @@ def test_create_data_type_only(self): | |
# preprocessed_data_id, filepath_id | ||
self.assertEqual(obs, [[3, obs_id - 1], [3, obs_id]]) | ||
|
||
def test_delete_basic(self): | ||
"""Correctly deletes a preprocessed data""" | ||
# testing regular delete | ||
ppd = PreprocessedData.create( | ||
self.study, self.params_table, | ||
self.params_id, self.filepaths, prep_template=self.prep_template, | ||
ebi_submission_accession=self.ebi_submission_accession, | ||
ebi_study_accession=self.ebi_study_accession) | ||
PreprocessedData.delete(ppd.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should have some SQL tests to make sure the delete actually removed all the rows expected from the DB. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add but I see them as useless as the test just after this one is to check that the deletion fails cause this object doesn't exist anymore, do you agree? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to make sure that the lines are all deleted out of the 4 or 5 tables, since the failure is only due to one row in one table. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the lines are in a queue so if one fails, all fail, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's for future-proofing to make sure the lines are always deleted no matter the code changes. If we change the code, we still need to make sure the functionality remains. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point @antgonza. However, this won't work in the case that we move the "delete" function from a classmethod to an instance method. This is something that I've been thinking and I was planning to propose to the group, as I think it makes way more sense to have the delete function as an instance method rather than a classmethod. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you feel like that's what we should do, IMOO and following the "rules" we discussed last week, that should be a separate issue. Anyway, the concern about having it as instance method is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree that it should be a separate issue. BTW, that concern is not solved with the current solution: PreprocessedData.delete(ppd.id)
ppd.whatever = 55 I moved the discussion here: #1045 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, I think that @ElDeveloper enhancements should be implemented, as it is safer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @ElDeveloper enhancements as well. |
||
|
||
# testing that it raises an error if ID doesn't exist | ||
with self.assertRaises(QiitaDBUnknownIDError): | ||
PreprocessedData.delete(ppd.id) | ||
|
||
# testing that we can not remove cause the preprocessed data != sandbox | ||
with self.assertRaises(QiitaDBStatusError): | ||
PreprocessedData.delete(1) | ||
|
||
def test_delete_advance(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. advance -> advanced |
||
# testing that we can not remove cause preprocessed data has been | ||
# submitted to EBI or VAMPS | ||
ppd = PreprocessedData.create( | ||
self.study, self.params_table, | ||
self.params_id, self.filepaths, prep_template=self.prep_template, | ||
ebi_submission_accession=self.ebi_submission_accession, | ||
ebi_study_accession=self.ebi_study_accession) | ||
|
||
# fails due to VAMPS submission | ||
ppd.update_vamps_status('success') | ||
with self.assertRaises(QiitaDBStatusError): | ||
PreprocessedData.delete(ppd.id) | ||
ppd.update_vamps_status('failed') | ||
|
||
# fails due to EBI submission | ||
ppd.update_insdc_status('success', 'AAAA', 'AAAA') | ||
with self.assertRaises(QiitaDBStatusError): | ||
PreprocessedData.delete(ppd.id) | ||
|
||
def test_create_error_dynamic_table(self): | ||
"""Raises an error if the preprocessed_params_table does not exist""" | ||
with self.assertRaises(IncompetentQiitaDeveloperError): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these messages (832 and 836) are likely to be seen by users on the GUI, do you think you could rephrase them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. I think it should be "sandboxed" not "sandbox".