[WRSAT-488] Make email logic safer

app/mailers/screener_mailer.rb

@@ -3,7 +3,6 @@ class ScreenerMailer < ApplicationMailer
 
   def send_screener_results(outgoing_email:)
     @screener = outgoing_email.screener
-    @county_data = LocationData::Counties.get(@screener.state, @screener.county)
     attachments.inline["gbh_email_header.png"] = File.binread(Rails.root.join("app/assets/images/gbh_email_header.png"))
     mail(to: @screener.email, subject: I18n.t("views.screener_mailer.send_screener_results.subject"))
   end

app/models/location_data.rb

@@ -61,16 +61,6 @@ def self.load_all
 
     ALL_COUNTIES = load_all
 
-    def self.fetch_county!(state_code, county_key)
-      raise ArgumentError, "state_code is required" if state_code.blank?
-      raise ArgumentError, "county_key is required" if county_key.blank?
-
-      county = ALL_COUNTIES.dig(state_code, county_key)
-      raise StandardError, "County not found for #{state_code} / #{county_key}" unless county
-
-      county
-    end
-
     def self.for_state(state)
       ALL_COUNTIES[state] || {}
     end
@@ -80,29 +70,39 @@ def self.options_for(state)
     end
 
     def self.get(state, county_key)
-      for_state(state)[county_key]
+      raise ArgumentError, "state_code is required" if state.blank?
+      raise ArgumentError, "county_key is required" if county_key.blank?
+
+      county = ALL_COUNTIES.dig(state, county_key)
+      raise StandardError, "County not found for #{state} / #{county_key}" unless county
+
+      county
     end
 
     def self.website_for(state_code, county_key)
-      fetch_county!(state_code, county_key)[:website]
+      get(state_code, county_key)[:website]
     end
 
     def self.upload_portal_or_email_for(state_code, county_key)
-      county = fetch_county!(state_code, county_key)
+      county = get(state_code, county_key)
       county[:upload_portal_or_email].presence || county[:email]
     end
 
+    def self.email_for(state_code, county_key)
+      get(state_code, county_key)[:email]
+    end
+
     def self.mailing_address_for(state_code, county_key)
-      fetch_county!(state_code, county_key)[:mailing_address]
+      get(state_code, county_key)[:mailing_address]
     end
 
     def self.physical_address_for(state_code, county_key)
-      county = fetch_county!(state_code, county_key)
+      county = get(state_code, county_key)
       county[:physical_address].presence || county[:mailing_address]
     end
 
     def self.phone_for(state_code, county_key)
-      fetch_county!(state_code, county_key)[:phone]
+      get(state_code, county_key)[:phone]
     end
   end
 end

app/views/screener_mailer/send_screener_results.html.erb

@@ -26,19 +26,22 @@
 <h3 style="font-weight: bold;">
   <%= t('views.screener_mailer.send_screener_results.how_to_submit_heading') %>
 </h3>
+<% state = @screener.state %>
+<% county = @screener.county %>
+<% website = LocationData::Counties.website_for(state, county) %>
+<% email = LocationData::Counties.email_for(state, county) %>
 
 <ol style="line-height: 2;">
   <li>
     <strong><%= t('views.screener_mailer.send_screener_results.online_label') %></strong> <%= t('views.screener_mailer.send_screener_results.online_epass_html').html_safe %>
     <br/><br/>
-    <%= t('views.screener_mailer.send_screener_results.online_county', website: @county_data[:website]) %><%= t('views.screener_mailer.send_screener_results.online_county_with_email', email: @county_data[:email]) if @county_data[:email].present? %>.
-  </li>
+    <%= t('views.screener_mailer.send_screener_results.online_county', website: website) %><%= t('views.screener_mailer.send_screener_results.online_county_with_email', email: email) if email.present? %>.
   </li>
   <li>
     <strong><%= t('views.screener_mailer.send_screener_results.mail_label') %></strong> <%= t('views.screener_mailer.send_screener_results.mail_instructions') %>
     <br/><br/>
     <div style="font-style: italic;">
-      <% @county_data[:mailing_address].split("\n").map do |line| %>
+      <% LocationData::Counties.mailing_address_for(state, county).split("\n").map do |line| %>
         <%= line %>
         <br/>
       <% end %>
@@ -48,7 +51,7 @@
     <strong><%= t('views.screener_mailer.send_screener_results.in_person_label') %></strong> <%= t('views.screener_mailer.send_screener_results.in_person_instructions') %>
     <br/><br/>
     <div style="font-style: italic;">
-      <% @county_data[:physical_address].split("\n").map do |line| %>
+      <% LocationData::Counties.physical_address_for(state, county).split("\n").map do |line| %>
         <%= line %>
         <br/>
       <% end %>
@@ -61,11 +64,13 @@
 </p>
 
 <ul style="line-height: 2;">
-  <li><%= t('views.screener_mailer.send_screener_results.check_website', website: @county_data[:website]) %></li>
-  <% if @county_data[:email].present? %>
+  <li><%= t('views.screener_mailer.send_screener_results.check_website', website: website) %></li>
+  <% if email.present? %>
     <li>
-      <%= t('views.screener_mailer.send_screener_results.email_office', email: @county_data[:email]) %>
+      <%= t('views.screener_mailer.send_screener_results.email_office', email: email) %>
     </li>
   <% end %>
-  <li><%= t('views.screener_mailer.send_screener_results.call_office', phone: @county_data[:phone]) %></li>
+  <% if (phone = LocationData::Counties.phone_for(state, county)).present? %>
+    <li><%= t('views.screener_mailer.send_screener_results.call_office', phone: phone) %></li>
+  <% end %>
 </ul>

app/views/screener_mailer/send_screener_results.text.erb

@@ -11,25 +11,31 @@
 <%= t('views.screener_mailer.send_screener_results.reference_code', code: @screener.confirmation_code) %>
 
 ---
+<% state = @screener.state %>
+<% county = @screener.county %>
+<% website = LocationData::Counties.website_for(state, county) %>
+<% email = LocationData::Counties.email_for(state, county) %>
 
 <%= t('views.screener_mailer.send_screener_results.how_to_submit_heading') %>
 
 1. <%= t('views.screener_mailer.send_screener_results.online_label') %> <%= t('views.screener_mailer.send_screener_results.online_epass_text') %>
 
-   <%= t('views.screener_mailer.send_screener_results.online_county', website: @county_data[:website]) %><%= " #{t('views.screener_mailer.send_screener_results.online_county_with_email', email: @county_data[:email])}" if @county_data[:email].present? %>.
+   <%= t('views.screener_mailer.send_screener_results.online_county', website: website) %><%= " #{t('views.screener_mailer.send_screener_results.online_county_with_email', email: email)}" if email.present? %>.
 
 2. <%= t('views.screener_mailer.send_screener_results.mail_label') %> <%= t('views.screener_mailer.send_screener_results.mail_instructions') %>
 
-   <%= @county_data[:mailing_address].split("\n").join("\n   ") %>
+   <%= LocationData::Counties.mailing_address_for(state, county).split("\n").join("\n   ") %>
 
 3. <%= t('views.screener_mailer.send_screener_results.in_person_label') %> <%= t('views.screener_mailer.send_screener_results.in_person_instructions') %>
 
-   <%= @county_data[:physical_address].split("\n").join("\n   ") %>
+   <%= LocationData::Counties.physical_address_for(state, county).split("\n").join("\n   ") %>
 
 <%= t('views.screener_mailer.send_screener_results.questions_intro') %>
 
-- <%= t('views.screener_mailer.send_screener_results.check_website', website: @county_data[:website]) %>
-<% if @county_data[:email].present? %>
-- <%= t('views.screener_mailer.send_screener_results.email_office', email: @county_data[:email]) %>
+- <%= t('views.screener_mailer.send_screener_results.check_website', website: website) %>
+<% if email.present? %>
+- <%= t('views.screener_mailer.send_screener_results.email_office', email: email) %>
+<% end %>
+<% if (phone = LocationData::Counties.phone_for(state, county)).present? %>
+- <%= t('views.screener_mailer.send_screener_results.call_office', phone: phone) %>
 <% end %>
-- <%= t('views.screener_mailer.send_screener_results.call_office', phone: @county_data[:phone]) %>

spec/controllers/out_of_state_controller_spec.rb

@@ -35,7 +35,9 @@ def county_name(county)
   describe ".county_not_supported?" do
     it "returns false when county is nil" do
       screener = create(:screener, state: state_with_counties, county: nil)
-      expect(described_class.county_not_supported?(screener)).to eq(false)
+      expect {
+        described_class.county_not_supported?(screener)
+      }.to raise_error(ArgumentError, /county_key is required/)
     end
 
     it "returns false for supported counties" do

spec/models/location_data_spec.rb

@@ -63,22 +63,6 @@
       end
     end
 
-    describe ".get" do
-      it "returns the correct data for each county" do
-        all_counties.each do |state, counties|
-          counties.each do |county_name, data|
-            result = described_class.get(state, county_name)
-            expect(result).to eq(data)
-          end
-        end
-      end
-
-      it "returns nil for unknown county or state" do
-        expect(described_class.get("NC", "Fake County")).to be_nil
-        expect(described_class.get("FAKE", "Some County")).to be_nil
-      end
-    end
-
     describe "county counts" do
       it "reports correct number of counties per state" do
         all_counties.each do |state, counties|
@@ -89,30 +73,30 @@
       end
     end
 
-    describe ".fetch_county!" do
+    describe ".get" do
       let(:state) { all_counties.keys.first }
       let(:county) { all_counties[state].keys.first }
 
       it "returns the county when valid inputs are provided" do
-        result = described_class.fetch_county!(state, county)
+        result = described_class.get(state, county)
         expect(result).to eq(all_counties[state][county])
       end
 
       it "raises error when state_code is blank" do
         expect {
-          described_class.fetch_county!(nil, county)
+          described_class.get(nil, county)
         }.to raise_error(ArgumentError, /state_code is required/)
       end
 
       it "raises error when county_key is blank" do
         expect {
-          described_class.fetch_county!(state, nil)
+          described_class.get(state, nil)
         }.to raise_error(ArgumentError, /county_key is required/)
       end
 
       it "raises error when county does not exist" do
         expect {
-          described_class.fetch_county!(state, "Fake County")
+          described_class.get(state, "Fake County")
         }.to raise_error(StandardError, /County not found/)
       end
     end
@@ -133,6 +117,22 @@
       end
     end
 
+    describe ".email_for" do
+      let(:state) { all_counties.keys.first }
+      let(:county) { all_counties[state].keys.first }
+
+      it "returns the email for a valid county" do
+        expected = all_counties[state][county][:email]
+        expect(described_class.email_for(state, county)).to eq(expected)
+      end
+
+      it "raises when county not found" do
+        expect {
+          described_class.email_for(state, "Fake County")
+        }.to raise_error(StandardError)
+      end
+    end
+
     describe ".upload_portal_or_email_for" do
       let(:state) { all_counties.keys.first }