Update CSF 201 emails

[hacodeorg]

PLC-86 and PLC-186

1. Delete "workshop survey reminder" email
2. Add CSF 201 pre-workshop survey link to reminder email
3. Add CSF 201 post-workshop survey link to exit email
4. Update email previews comparison script so it doesn't exit if downloading 1 preview fails. And a few minor changes

How tested

• Manual test by reading email previews at http://localhost-studio.code.org:3000/rails/mailers/pd/workshop_mailer/
• Automated email "eye-test" by running

  bin\oneoff\diff_email_previews download base   # runs before doing feature work
  bin\oneoff\diff_email_previews download new   # runs after doing feature work
  bin\oneoff\diff_email_previews diff base new   # then open diffresult.txt file
  

• Unit test: bin/rails test test/models/pd/enrollment_test.rb

[hacodeorg]

I use guard clause like this return ... if|unless... instead of if-elsif-elsif... to make it less error-prone and easier to read (reference). Please let me know which style you prefer?

[islemaster]

I prefer the original if-elsif-else structure of this method.

I'm not against guard clauses as a pattern. I just don't think this is the right place to use them. The article you reference is all about avoiding nested if/else logic for readability, but there's no nesting here. It's more of a one-layer switch statement.

More specifically, I find guard clauses useful for validating input or handling unexpected cases. For example:

def my_workshops
  return [] unless current_user
  # Happy-path logic
end

My interpretation of this method is that all of the cases are happy-path cases, likely to occur in ordinary use. For that reason, I think the if-elsif-else structure is a clearer expression of the intended behavior of the method. I also think the elegance of the method as a single expression is very ruby-idiomatic.

I understand that's very touchy-feely-squishy code style reasoning, and a reasonable person might come to the opposite conclusion. Therefore I'm interested in other opinions. @clareconstantine and @Hamms do you have thoughts on this?

[clareconstantine]

While both are reasonable, I agree with Brad on this one. I also find the original structure much easier to read and reason about - it basically functions as a switch statement and I like that the original structure clearly communicates that. And it is more ruby-ish in that it takes advantage of ruby returning the last evaluated expression and doesn't need to use the return keyword.

[hacodeorg]

Thanks guys for the input. I'm okay with keeping it as-is now that I understand what it does. (At first it was quite confusing reading 9 condensed lines with multiple conditions).

[Hamms]

I could really go either way on this; the things that make this method hard to reason about for me are the opaqueness of the conditions themselves and the repeated code in the url generation.

Would y'all feel differently about guard clauses vs if/else if the method looked like this?

def exit_survey_url
  return pd_counselor_admin_workshop_survey_url(code) if workshop.counselor_admin?
  return pd_new_workshop_survey_url(code) if workshop.summer?
  return pd_csd_csp_2018_workshop_survey_url(workshop.sessions.size, code) if workshop.csd_csp_2018?
  return pd_csf_2018_workshop_survey_url(code) if workshop.csf_201?

  pd_workshop_survey_url(code)
end

[islemaster]

I agree that reads better than either approach given the current conditions, but I still feel a slight mismatch using guard-clauses for something more like a case statement (this might be the single-return-rule hammered into me as an undergrad rearing its ugly head). In fact, I might prefer an actual case statement, with the conditions on the left:

def exit_survey_url
  case
  when workshop.counselor_admin? then pd_counselor_admin_workshop_survey_url(code)
  when workshop.summer?          then pd_new_workshop_survey_url(code)
  when workshop.csd_csp_2018?    then pd_csd_csp_2018_workshop_survey_url(workshop.sessions.size, code)
  when workshop.csf_201?         then pd_csf_2018_workshop_survey_url(code)
  else                                pd_workshop_survey_url(code)
  end
end

[islemaster]

Unrelated: After staring at this method for a while, it's starting to feel like it should live on Workshop instead of Enrollment.

[hacodeorg]

Reverting this back to use if-elsif-else. 2 open questions that we might want to address later:

1. Is there a better way to write multiple branches with long, sometime complex, conditions. I know the pain of reading it will fade away once you understand it, however that feeling will come back when you forget what it does :)
2. Survey link(s) should be in Pd::Workshop rather than Pd::Enrollment.

[islemaster]

👍 Great!

[islemaster]

Suggested change

	= link_to 'this survey', 'http://studio.code.org/pd/workshop_survey/csf/pre201'
	= link_to 'this survey', 'https://studio.code.org/pd/workshop_survey/csf/pre201'

[hacodeorg]

I should have not missed this. Thanks Brad

[islemaster]

With the refactor, I'm not sure what logic changed in this method. For whatever logic did change, is there a test that should be updated?

[hacodeorg]

Yes, I use bin/oneoff/diff_email_previews to compare emails that contain exit survey links before and after the change.

[clareconstantine]

While both are reasonable, I agree with Brad on this one. I also find the original structure much easier to read and reason about - it basically functions as a switch statement and I like that the original structure clearly communicates that. And it is more ruby-ish in that it takes advantage of ruby returning the last evaluated expression and doesn't need to use the return keyword.