Transactionize the Study, StudyPerson and Reference #1333
Conversation
| "reference_id = %s".format(self._table), (self._id,))[0] | ||
| _, basefp = get_mountpoint('reference')[0] | ||
| with TRN: | ||
| sql = """SELECT reference_name FROM qiita.{0} |
There was a problem hiding this comment.
these can be done with the convert_from_id function, correct?
There was a problem hiding this comment.
This is not converting from any id, is accessing to the reference_name attribute of the reference class.
|
@squirrelo I've addressed your comments @antgonza can you review this one? |
| with TRN: | ||
| # Get the status of all its processed data | ||
| sql = """SELECT processed_data_status | ||
| FROM qiita.processed_data_status pds |
There was a problem hiding this comment.
none blocking but you are missing a space.
| inv = conn_handler.execute_fetchone(sql, (self._id, )) | ||
| return inv[0] if inv is not None else inv | ||
| with TRN: | ||
| sql = """SELECT investigation_id FROM qiita.investigation_study |
There was a problem hiding this comment.
Will it be better to sort by investigation_id and limit 1?
There was a problem hiding this comment.
I don't think it is needed. If it exists, it will be only one. The inv[0][0] is because if it doesn't exist, TRN.execute_fetchindex returns an empty list, but if there is a value it will return [[inv_id]], and doesn't matter how I change the query, since this is because the transaction internally is doing a fetchall.
There was a problem hiding this comment.
A study can be part of more than one investigation though, right? Hence the investigation_study join table.
There was a problem hiding this comment.
I don't know. To be fair we have not been using investigation, and I think that right now is not needed given that we can have multiple prep templates in a single study.
Investigation was useful in the old database, were we had to create a study for each prep template... but right now we can create a single study and have everything there. And if multiple studies belong to "an investigation" that is a meta-analysis.
I think this is another discussion and this code is duplicating the functionality of the previous code, so if there are no more comments, we should merge this and keep moving forward with the transactionization of the system.
There was a problem hiding this comment.
Agreed. Should open an issue discussing if we need the investigation object.
|
minor comments |
|
Addressed/answered |
|
👍 |
|
One question. |
|
👍 |
Transactionize the Study, StudyPerson and Reference
Modifies the Study, StudyPerson and Reference objects to use transactions
Few comments:
Study.createwas not checking for duplicates. A check and a test have been added.StudyPerson.createwas not raising duplicate error in case of duplicate, just returned the existing object. Changed for consistency with the rest of the systemStudyPerson.existswas missing tests. Added.