State level action fixes & improvements#941
Conversation
| # This option may cause significant delays in view rendering with a large | ||
| # number of complex assets. | ||
| config.assets.debug = false | ||
| config.assets.debug = true |
There was a problem hiding this comment.
would like to leave this, got stuck on some JS issues for a while because i expected it to be true, though we don't have to leave it
There was a problem hiding this comment.
I'm good with leaving it in development! can modify locally if needed
spec/lib/civic_api_spec.rb
Outdated
| Rails.application.config.google_civic_api_url = "http://civic.example.com" | ||
| Rails.application.config.google_civic_api_url = "https://civic.example.com" | ||
| Rails.application.secrets.google_civic_api_key = "test-key-for-civic-api" | ||
| end |
There was a problem hiding this comment.
might be worth making a support file for stubbing this api
There was a problem hiding this comment.
would be worth it if it was easier to construct matching params strings, leaving for now
esoterik
changed the title
|
Ready for review! commit |
hartsick
left a comment
hartsick
left a comment
There was a problem hiding this comment.
Looks good! I left a few notes including where I made some small changes.
I'd love to talk about how state-level actions work and additional things we should track for future and make sure we have issues for those (expanding to target multiple roles, and handling multiple representative emails stood out to me—maybe there are more?).
spec/lib/civic_api_spec.rb
Outdated
| before do | ||
| Rails.application.config.google_civic_api_url = "http://civic.example.com" | ||
| Rails.application.config.google_civic_api_url = "https://civic.example.com" | ||
| Rails.application.secrets.google_civic_api_key = "test-key-for-civic-api" |
There was a problem hiding this comment.
Two notes here:
- in another PR I'm going to change this so that we stub the test environment rather than setting in test, since I'm not sure this gets reset after each test so we might have pollution. It will look like this:
allow(Rails.application.secrets).to receive(:google_civic_api_url) { "https://civic.example.com" }. Though we also have this directly set in the test env for secrets.yml, so can decide if it's necessary (might be for test clarity) - I think we want to update
Rails.application.configto beRails.application.secrets
app/lib/civic_api.rb
Outdated
|
|
||
| raise ArgumentError, "Invalid role for Civic API #{roles}" unless VALID_ROLES.include?(roles) | ||
| unless [address, roles].all? | ||
| raise ArgumentError, "required argument is nil "\ |
There was a problem hiding this comment.
I'm going to change this to just show what required arguments are nil, since we want to avoid sending the address to Sentry
Also, this line allows roles and address to be empty strings or arrays. I was confused about it before understanding we only allow one role to be selected for now (so it comes in as a nil rather than empty array), so am going to submit a change since I've already written it as part of above.
spec/requests/tools_spec.rb
Outdated
| let!(:headers) { { "CONTENT_TYPE" => "application/javascript" } } | ||
|
|
||
| before do | ||
| Rails.application.config.google_civic_api_url = "https://civic.example.com" |
There was a problem hiding this comment.
I went and set these in the test block for secrets.yml and deleted them from here - want to try to avoid potential test pollution by setting them here if we don't unset them.
I noticed config.google_civic_api_url is set from secrets.google_civic_api_url. It makes sense conceptually, but I think adds indirection that can be confusing. Might want to do a find + replace for Rails.application.config.google_civic_api_url and Rails.application.config.congress_forms_url, which is set similarly, to just point them to secrets
| end | ||
|
|
||
| describe "POST tools/state_reps" do | ||
| it "returns json containing rep data for a given address" do |
There was a problem hiding this comment.
Do we have a test that covers CivicApi.state_rep_search at a unit level? If not think it'd be helpful to cover at least the validation behaviors - think the happy path is covered by the system spec below.
|
Merging! Failing test is the flaky one I've been working to fix in a separate branch. |
2 participants
Fixes and refactors state level actions.
Still need to get styles matching other action types + do a clean up review of files (this involved a not insignificant amount of file renames and relocations and I haven't checked to clean up duplicates or anything yet)