Skip to content
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

Enable State-level Emails #903

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Enable State-level Emails #903

wants to merge 4 commits into from

Conversation

wtcruft
Copy link
Contributor

@wtcruft wtcruft commented Jan 18, 2023

This pull request adds functionality to enable users to email their state-level representatives. The overall design of the code is informed by the following constraint: the Google Civic API’s determination that we do not store the data we pull from their API. Therefore, there are minimal changes to the database.

Lib
Email addresses of state-level representatives are pulled from the aforementioned third-party API in a new library, civic_api.rb. Two methods are available here, one which pulls a single representative given a specific address and postal code (user-supplied), and the other which pulls all representatives in one legislative body (upper, lower, or a Governor) given only a state. This library gives us the flexibility to implement small changes in the future which will enable both state-wide email address look-ups as well as multiple-legislative body look-ups.

The first method, state_rep_search, is the current and primary use case. It looks up a single legislator, given a user-supplied address and an admin-supplied legislative level. Example usage:

CivicApi.state_rep_search(‘815 Eddy St San Francisco CA 94109’, ‘legislatorLowerBody’)

Database
Acceptable values for the 2nd parameter are ‘legislatorLowerBody’, ‘legislatorUpperBody’, and ‘headOfGovernment’. These are set by the admin user in the action creation panel, when they choose the legislative body for which users will look up the email address. The admin will also select from a dropdown list which state the action is intended for. This differentiates a “state” email campaign from a “custom” email campaign (the kind that already exist in the action center). These selections in the admin panel make changes in the database, inserting boolean values for state as well as for target_state_lower_chamber, target_state_upper_chamber and target_governor on the given EmailCampaign, if chosen.

Views
The templates for the email addresses have been split up so that the email address can be pulled from the API and displayed on the front-end. This way, the user can check who they will be emailing and then the email address will be injected into the template that is usually generated without having to go into our database, and without affecting the current custom email generation. In the same way as before these changes, only the “thank you for taking action” emails are sent from the back-end.

Tests
While there are new tests for the added functionality, it was also necessary to make additional testing changes. Several tests were failing both on the main branch and here. Rather than put those changes in a different pull request, which would block this work from passing tests until it was merged, I have changed some of the test configuration and a few existing tests in this branch so that the suite is more up to date.

@@ -194,14 +194,16 @@ def action_page_params
petition_attributes: [:id, :title, :description, :goal, :enable_affiliations],
affiliation_types_attributes: [:id, :name],
tweet_attributes: [
:id, :target, :target_house, :target_senate, :message, :cta, :bioguide_id,
:id, :target, :target_house, :target_senate, :target_state_lower_chamber,
:target_state_upper_chamber, :message, :cta, :bioguide_id,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is no longer necessary, it was left over from an earlier iteration of the work. I will remove it along with the rest of the code review changes.

Copy link
Member

@esoterik esoterik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review is just from reading through the code; I'll do a bit of QA and review the tests more thoroughly on Monday.

Overall this is really great! I suggested some minor-to-medium-sized refactors in my comments. The biggest lift of these would be changing the leg_level method and associated database columns into an enum if we're not planning on allowing a single action to multiple levels.

I also sorta started one refactor into a service object pattern for the API calls in a comment because I thought it might be helpful / more clear. I'm also not sure how widely-known of a pattern this is, so if you haven't heard of it this blogpost does a great job of explaining it -- I use this kind of design a lot!

I also think that it would be nice to have this kind of action be its own kind of campaign rather than adding onto the already existing email campaign, but I understand why you implemented it this way -- really the whole action type / campaign system could use a good rethink + refactor in this app anyway. I'm fine with leaving state level actions as a part of email campaigns for now and opening an issue to separate the two action types in the future.

@@ -2,6 +2,9 @@ class EmailCampaign < ActiveRecord::Base
belongs_to :topic_category
has_one :action_page

# No DC
STATES = %w(AK AL AR AZ CA CO CT DE FL GA HI IA ID IL IN KS KY LA MA MD ME MI MN MO MS MT NC ND NE NH NJ NM NV NY OH OK OR PA RI SC SD TN TX UT VA VT WA WI WV WY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we might have this info elsewhere in the app -- I'll check.

Also, we should add a .freeze to the end of this array for a small performance gain -- I'm pretty sure that this app is on a ruby version where that's necessary, but I might be wrong!

@@ -31,6 +31,9 @@ Follow these instructions to run the Action Center using Docker (recommended). T
* Allows users to submit e-messages to congress
* [Call Congress](https://github.com/EFForg/call-congress) url and API key
* Connects calls between citizens and their congress person using the Twilio API
* [Google Civic Information API](https://developers.google.com/civic-information) url and API key
* Representative information powered by the Civic Information API
* We use this when we need to give a user the ability to find their representatives to complete a state-level email action
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to add the API restrictions into the readme as well as the code comment you have in the CivicApi module

if params[:state_rep_email]
redirect_to @action_page.email_campaign.service_uri(params[:service], { email: params[:state_rep_email] })
else
redirect_to @action_page.email_campaign.service_uri(params[:service])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if we used strong parameters here to pass only one argument to #service_uri -- maybe something like

def email
   # beginning omitted
   redirect_to @action_page.email_campaign.service_uri(email_uri_params.to_h)
end

def email_uri_params
  params.permit(%i(service state_rep_email))
end

and then for the method

def service_uri(service:, state_rep_email: nil)
   # ...
end

<% end %>

<div id="state-level-target-selection">
<p>For now, please choose only one.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that we'll want to be able to select more than one of these in the future? If not, this would be a great attribute set to use an enum for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! That is the future vision, to select more than one.

<%= "We could not find their email address." %>
<% end %>
</div>
<% end -%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be simplified to

<% if sr["emails"].present? %>
  They can be reached at: <%= sr["emails"].join(", ") %>
<% else %>
  We could not find their email address.
<% end %>

role = "headOfGovernment" if self.target_governor

return role
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to use guard clauses here + have an explicit default value:

def leg_level(email_campaign)
  return "headOfGovernment" if email_campaign.target_governor
  return "legislatorUpperBody" if email_campaign.target_state_upper_chamber
  return "legislatorLowerBody" if email_campaign.target_state_lower_chamber
  ""
end

lib/civic_api.rb Outdated
begin
error = JSON.parse(e.http_body)["error"]
rescue
raise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should still raise an error here I think?

# need to append division information to API route
path_params = { ocdId: "ocd-division%2Fcountry%3Aus%2Fstate%3A#{state.downcase}" }
# `administrativeArea1` param restricts the search to state-level legislators (and governors)
query_params = { levels: "administrativeArea1", recursive: true, roles: roles, key: civic_api_key }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the query params that are the same in both methods were stated as shared defaults, maybe a method like

def self.query_params(**additional_params)
  { levels: "administrativeArea1", key: civic_api_key }.merge(additional_params)
end

db/schema.rb Outdated
@@ -354,7 +358,7 @@

create_table "tweets", force: :cascade do |t|
t.string "target", limit: 255
t.string "message", limit: 255
t.string "message"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed from the diff

@@ -51,7 +51,7 @@
config.include Capybara::DSL
config.include FeatureHelpers, type: :feature

WebMock.disable_net_connect!(allow_localhost: true)
WebMock.disable_net_connect!(allow_localhost: true, allow: "chromedriver.storage.googleapis.com")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhhhh this fixes one of the more annoying errors that I've run into so nicely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants