fixed false assumption that a participant (source) has only one surve… #637
Conversation
…y_id, when deleting this participant.
|
@EmbrietteH @wasade @josenavas please have a look at this PR which should fix the issue #636 |
|
Please think about fact that a participant might have multiple email addresses. If so, do we want to list them only once in the ag.consent_revoked table or multiple times each with a different email? Currently, the later is implemented. |
There was a problem hiding this comment.
Thanks @sjanssen2
It is possible to add a test with the current data in the DB?
|
|
||
| sql = "DELETE FROM survey_answers WHERE survey_id = %s" | ||
| TRN.add(sql, [survey_id]) | ||
| for survey_id in survey_ids: |
There was a problem hiding this comment.
Change to:
sql = "DELETE FROM survey_answers WHERE survey_id IN %s"
TRN.add(sql, [tuple(survey_ids)])Note the call to tuple is required. This way all the delete is done in a single SQL query.
There was a problem hiding this comment.
I like the tuple style. And will change the code to it.
A question for understanding: I thought that all SQL statements of this function are executed as a transaction, so why do I have to explicitly care here to trigger only one SQL query?
There was a problem hiding this comment.
Even it is a single transaction the number of sql queries changes and each sql queries has some overhead. Reducing the number of SQL queries reduces the running time of the method.
There was a problem hiding this comment.
Aaand clearly I need more coffee after reading my english on my last comment 😄
There was a problem hiding this comment.
Understood :-) It's no issue of concurrence, but performance. Good to know.
|
|
||
| sql = "DELETE FROM survey_answers_other WHERE survey_id = %s" | ||
| TRN.add(sql, [survey_id]) | ||
| for survey_id in survey_ids: |
There was a problem hiding this comment.
Same as my previous comments - modify to use a single SQL query.
|
|
||
| # Reset survey attached to barcode(s) | ||
| for info in self.getParticipantSamples(ag_login_id, | ||
| participant_name): | ||
| self.deleteSample(info['barcode'], ag_login_id) | ||
|
|
||
| sql = "DELETE FROM promoted_survey_ids WHERE survey_id = %s" | ||
| TRN.add(sql, [survey_id]) | ||
| for survey_id in survey_ids: |
There was a problem hiding this comment.
Same here and in the next 3 for loops.
| @@ -333,7 +344,9 @@ def deleteAGParticipantSurvey(self, ag_login_id, participant_name): | |||
| sql = """INSERT INTO ag.consent_revoked | |||
| (ag_login_id,participant_name, participant_email) | |||
| VALUES (%s, %s, %s)""" | |||
| TRN.add(sql, [ag_login_id, participant_name, participant_email]) | |||
| for participant_email in list(participant_emails): | |||
There was a problem hiding this comment.
I don't think the call to list is needed (you can iterate over a set). However, this can be changed to:
sql_args = [[ag_login_id, participant_name, pemail] for pemail in participant_emails]
TRN.add(sql, sql_args, many=True)And add at the end of the function:
TRN.execute()So we make sure that whatever is listed here is actually executed before returning this functions (but not necessarily committed)
|
test are not possible, because there is no (survey_login_id, participant_name) tuple that appears more than once :-( |
|
👍 following @josenavas's comments |
|
I sat with @sjanssen2 and read the PR. It looks all good to the best of my knowledge. |
…y_id, when deleting this participant.
See issue #636