-
Notifications
You must be signed in to change notification settings - Fork 6
Import unused sample #2391
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
Import unused sample #2391
Conversation
645c2b7 to
46cdcfe
Compare
46cdcfe to
64a54c5
Compare
fa1e4fb to
ba4bfcb
Compare
We still have to pick the right survey, and probably show a preview. See #2389
We still have to implement the source survey picker, and check it's finished. See #2389
We don't want to import from surveys that are still running. See #2389
We now have to use it from the UI. See #2389
It was copied from another modal, but then turned up unnecessary. See #2389
ba4bfcb to
34a4e79
Compare
ggiraldez
left a comment
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.
Looks good as far as I can evaluate with minimal Elixir and completely rusted redux/react knowledge. Left a small suggestion on some query, but it's probably not too important.
| where: s.project_id == ^project.id and s.state == :terminated, | ||
| select: %{survey_id: s.id, name: s.name, ended_at: s.ended_at, respondents: sum(fragment("if(?, ?, ?)", r.disposition == :registered, 1, 0))}, |
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.
I suppose this could also be a select ..., count(*) as respondents from ... where r.disposition = 'registered' which may be more efficient. I'm not sure about the Ecto syntax though.
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.
The Ecto syntax is not a problem:
from s in Survey,
left_join: r in Respondent,
on: r.survey_id == s.id,
where: s.project_id == ^project.id and s.state == :terminated and r.disposition == :registered,
select: %{survey_id: s.id, name: s.name, ended_at: s.ended_at, respondents: count(s.id)},
group_by: [s.id],
order_by: [desc: count(s.id)]
But then you don't get the surveys that have 0 available respondents listed. I'd rather explicitely show that there are no available respondents rather than ignore the survey at all.
I could, though, add a comment on the code explaining this.
There's probably another query that can be written for that, but not sure it'd be better.
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.
Good point. You can use a left join for that, and count(r.id) or any other field that is NULL when joining an empty set.
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.
Ohhhh! The key is to add the r.disposition = 'terminated' condition to the JOIN instead of the WHERE (third query in the screenshot), so the join is made with an empty half-row. When the condition is on the WHERE (second query), you first match surveys and respondents, but then discard the joined row altogether for not satisfying the condition.
| sample_name = "__imported_from_survey_#{source_survey.id}.csv" | ||
| case RespondentGroupAction.load_entries(entries, survey) do | ||
| {:ok, loaded_entries} -> | ||
| survey |> RespondentGroupAction.disable_incentives_if_disabled_in_source!(source_survey) |
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.
This may merit a comment. Why is adding a respondent group to the survey updating an attribute of the survey?
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.
That's a business rule - if we're importing contacts from a deanonimized survey, this one is now also deanonimized.
I guess we can't compute that value from the respondent groups "at runtime" in case one can "game" the system by adding/removing respondent groups somehow - but I'm not really sure about that. I mostly followed what we're doing when adding respondents from CSVs.
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.
That's fair. I guess what threw me off was mostly that the updating function is in the respondent group file, but it updates the survey. I see that the module is RespondentGroupAction, but I still find it a bit confusing. Also, how are incentives related to anonymization? Is that legacy naming?
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.
No, it's not legacy. Incentives distribution is made by having the user download a list of phone numbers, and that's the only way for a user to get phone numbers out of Surveda. Those numbers already responded surveys, so there's potential to link responses to phone numbers - breaking the anonymity.
There might be a better way to model this "tainting" of the respondent group, but I'm not sure which one that'd be.
After what we discussed with @ggiraldez in #2391
After what we discussed with @ggiraldez in #2391
Let users import into a survey all the uncontacted respondents from another finished one.
The source survey must be already terminated, and has to belong to the same project as the target one.
surveda-import-sample.webm
Error handling:

Fixes #2389