Facilitator application csv #26723
Facilitator application csv #26723
Conversation
clareconstantine
commented
Jan 24, 2019
clareconstantine
commented
- Add lots of columns to the facilitator application csv
- The csv contains the same columns when it's downloaded from quick view and when it's downloaded from cohort view
clareconstantine
requested review from
islemaster,
bethanyaconnor and
hacodeorg
| elsif course == 'csd' | ||
| CSV_LABELS.keys.select {|k| k.to_s.start_with?('csf', 'csp')} | ||
| else | ||
| CSV_LABELS.keys.select {|k| k.to_s.start_with?('csf', 'csd_training')} |
There was a problem hiding this comment.
Why csd_training here but just csd above?
There was a problem hiding this comment.
Many questions are common to csd and csp, and they are all prefixed with 'csd_csp'. So for a csp csv, we want to include all of those. The only csd-specific question is the one prefixed 'csd_training', so we want to exclude that one here.
The course names, as stored in the course column, are 'csf', 'csd', or 'csp'. I should probably be using the constants for these.
| csv << columns | ||
| row = [] | ||
| CSV_LABELS.keys.each do |k| | ||
| if columns_to_exclude&.include? k.to_sym |
There was a problem hiding this comment.
Would it make sense for the columns_to_remove method to never return nil so the safe-navigation operator isn't necessary here? It seems to me like an empty collection is a reasonable default case.
| if columns_to_exclude&.include? k.to_sym | ||
| next | ||
| end | ||
| row.push(full_answers[k] || try(k) || all_scores[k]) |
There was a problem hiding this comment.
Style nits (with the caveat that I'm not the most fluent rubyist):
I feel like I've been pushed toward inlining one-line conditionals like this one:
next if columns_to_exclude&.include? k.to_symThen again, I wonder if it's simpler change if A then next else B to if !A then B:
unless columns_to_exclude&.include? k.to_sym
row.push(full_answers[k] || try(k) || all_scores[k])
endOr (and I'm not sure this is cleaner) can we just filter the set we're iterating over?
CSV_LABELS.keys.
reject {|k| columns_to_exclude&.include? k.to_sym}.
each {|k| row.push(full_answers[k] || try(k) || all_scores[k]}I don't want to be prescriptive here - totally open to conversation about what we find most readable.
| question_4: "Interview #{INTERVIEW_QUESTIONS[:question_4]}", | ||
| question_5: "Interview #{INTERVIEW_QUESTIONS[:question_5]}", | ||
| question_6: "Interview #{INTERVIEW_QUESTIONS[:question_6]}", | ||
| question_7: "Interview #{INTERVIEW_QUESTIONS[:question_7]}" |
There was a problem hiding this comment.
This all looks a lot like an i18n file. Is this in the wrong spot?
There was a problem hiding this comment.
Since our PD is currently US-only (except specific arrangements with a few international partners), none of it is translated. These strings are similar to the application questions, which are stored in a constants file, but I shortened some of them for brevity in the CSV header (although they are still long).
| 'Regional Partner' | ||
| ) | ||
| csv << columns | ||
| # Filter out extraneous answers based on selected program (course) |
There was a problem hiding this comment.
This function creates a list of columns to filter, correct? If so, can you update this column to a bit clearer?
