-
Notifications
You must be signed in to change notification settings - Fork 106
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
[WIP]Refactor elections endpoint to handle candidate finance totals export function #3292
Conversation
Refactor ElectionView to enable candidate finance totals(CFT) export function.
tests/test_downloads.py
Outdated
@@ -143,8 +143,8 @@ def test_download_cached(self, export, get_cached): | |||
assert not export.delay.called | |||
|
|||
def test_download_forbidden(self): | |||
with pytest.raises(ApiError): | |||
self.client.post_json(api.url_for(resource.DownloadView, path='elections', office='house', cycle=2018)) | |||
response = self.app.get(api.url_for(resource.DownloadView, path='elections', office='house', cycle=2018)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unclear about what this test is trying to accomplish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear either, Can we remove this test case function? Thanks @vrajmohan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there another view that does not support downloads that can be used for this test instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. I look at most of our endpoints, we do have some endpoints don't support download, but they are not added into this test_download_forbidden function.
How about change function name to donttest_download_forbidden
for now? in case, we need test other endpoints in the future? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand "they are not added into this test_download_forbidden function". Why can't we use one of those paths instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, I add '/elections/search' (resource.ElectionListView) endpoint into test_download_forbidden function. work well. Thanks.
tests/test_elections.py
Outdated
self.assertEqual(len(results), 0) | ||
|
||
response = self.app.get(api.url_for(ElectionView, office='senate', cycle=2012, state='ZZ', per_page=0)) | ||
self.assertEquals(response.status_code, 422) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unclear about what this test is trying to accomplish.
webservices/resources/elections.py
Outdated
query = self._get_records(kwargs) | ||
return utils.fetch_page(query, kwargs, cap=0) | ||
return query | ||
|
||
def _get_records(self, kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just rename this to build_query
with the right signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point. will do. Thanks
Fix 'empty query' test case. Add space to pass PEP 8.
22ed3be
to
7b19e6b
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3292 +/- ##
========================================
Coverage 87.72% 87.72%
========================================
Files 75 75
Lines 6159 6159
========================================
Hits 5403 5403
Misses 756 756
Continue to review full report at Codecov.
|
'elections/search' (resource.ElectionListView) don't support download function for now.
Summary
1)In order to enable 'export' button on these two sections in election summery page, we need refactor elections endpoint to extend ApiResouce and handle download function.
These two sections use same endpoint:
resource.ElectionView
a)Candidate financial totals section:
https://www.fec.gov/data/elections/senate/VA/2018/?tab=election-data
b)Candidate information section:
https://www.fec.gov/data/elections/senate/VA/2018/?tab=about-election
Include a summary of proposed changes
fec-cms need to be changed to enable the button and make event trigger work.
How to test the changes locally
1)setup openFEC locally
check out openFEC feature branch: feature/refactor_elections_endpoint
set some variables:
export SQLA_CONN=dev
export access_key_id=
export secret_access_key=
export bucket=
export region=
checkout fec-cms feature branch: feature/enable_cft_export_button
and point to your local api:
export FEC_WEB_API_KEY_PUBLIC=xxxxxxx
export FEC_WEB_API_KEY=xxxxxxx
export FEC_CMS_ENVIRONMENT=LOCAL
export FEC_API_URL=http://localhost:5000
export DATABASE_URL=postgresql://:@/cfdm_cms_test
3)start redis-server
redis-server
4)Setup Celery Worker and start
export SQLA_CONN=xxxxxxx
export access_key_id=
export secret_access_key=
export bucket=
export region=
celery worker --app webservices.tasks --loglevel INFO
5)test:
example1 url: http://127.0.0.1:8000/data/elections/senate/VA/2018/?tab=election-data
Click "Export" on Candidate financial totals section, download the file.
example2 url http://127.0.0.1:8000/data/elections/senate/VA/2018/?tab=about-election
Click "Export" on Candidate information section, download the file.