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

[V0086] Fix candidate totals for future elections #3219

Merged
merged 4 commits into from
Jun 28, 2018

Conversation

lbeaufort
Copy link
Member

@lbeaufort lbeaufort commented Jun 18, 2018

Summary (required)

Fix candidate totals for future elections (2020/2022 presidential/senate)

I double-checked totals and tracked them here: https://docs.google.com/spreadsheets/d/1NUlhWZ6aHvNNSugPk3BWemDQSZn3pimjbqW_4IC_cnw/edit#gid=0

How to test the changes locally

Impacted areas of the application

List general components of the application that this PR will affect:

  • /candidates/totals endpoint
  • fec.gov/data/candidates/president/
  • fec.gov/data/candidates/house/ (should be no change)
  • fec.gov/data/candidates/senate/
  • Raising/Spending breakdowns when we bring them back

Before - no data 👎

screen shot 2018-06-19 at 6 38 43 pm

screen shot 2018-06-19 at 6 37 14 pm

After - data! 👍

screen shot 2018-06-19 at 6 38 15 pm

screen shot 2018-06-19 at 6 37 38 pm

Related PRs

Branched from feature/fix-special-candidate-totals, PR: #3198

@lbeaufort lbeaufort force-pushed the feature/fix-presidential-totals branch from cb9dc2b to f91e4f5 Compare June 19, 2018 15:04
@codecov-io
Copy link

codecov-io commented Jun 19, 2018

Codecov Report

Merging #3219 into develop will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3219      +/-   ##
===========================================
+ Coverage    87.78%   88.14%   +0.35%     
===========================================
  Files           77       77              
  Lines         6167     6147      -20     
===========================================
+ Hits          5414     5418       +4     
+ Misses         753      729      -24
Impacted Files Coverage Δ
webservices/utils.py 91.2% <100%> (+0.12%) ⬆️
webservices/common/models/candidates.py 100% <100%> (ø) ⬆️
webservices/resources/candidate_aggregates.py 96.72% <100%> (ø) ⬆️
webservices/flow.py 100% <100%> (ø) ⬆️
webservices/resources/audit.py 97.05% <0%> (-0.05%) ⬇️
webservices/rest.py 89.73% <0%> (-0.04%) ⬇️
webservices/tasks/__init__.py 68% <0%> (ø) ⬆️
webservices/tasks/legal_docs.py 0% <0%> (ø) ⬆️
webservices/docs.py 100% <0%> (ø) ⬆️
webservices/spec.py 100% <0%> (ø) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f920bdd...ef48865. Read the comment docs.

@lbeaufort lbeaufort force-pushed the feature/fix-presidential-totals branch from acc6359 to 3dd3293 Compare June 19, 2018 22:07
@lbeaufort lbeaufort changed the title [WIP] Fix candidate totals for future elections Fix candidate totals for future elections Jun 19, 2018
@lbeaufort lbeaufort requested review from hcaofec, pkfec and fec-jli June 19, 2018 22:29
@@ -357,6 +357,76 @@ def setUp(self):
fec_election_year=2018,
)

# Create data for future presidential - 2020. Use formula for future
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a good opportunity to try out Hypothesis and seeing how well it pairs with py.test's parameterized functions! I know there is a desire to get this out sooner rather than later, but I think these tests and this setup would make for an excellent candidate in trying this out in the future. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @ccostino! I forgot I put this note in here. I was thinking of using a get_current_cycle() + 2 but it looks like that function is only in the CMS.

@patphongs patphongs changed the title Fix candidate totals for future elections [DO NOT MERGE] Fix candidate totals for future elections Jun 20, 2018
@lbeaufort lbeaufort force-pushed the feature/fix-presidential-totals branch from b6dfe36 to 82b0259 Compare June 20, 2018 20:20
@@ -168,9 +170,12 @@ def test_candidate_aggregates_by_election(self):

class TestCandidateAggregates(ApiBaseTest):

current_cycle = get_current_cycle()
Copy link
Member Author

Choose a reason for hiding this comment

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

@ccostino I made these future cycles dynamic :)

@lbeaufort lbeaufort changed the title [DO NOT MERGE] Fix candidate totals for future elections [V0086] Fix candidate totals for future elections Jun 22, 2018
@lbeaufort lbeaufort requested a review from jwchumley June 25, 2018 17:56
@hcaofec
Copy link
Contributor

hcaofec commented Jun 25, 2018

the creation script of this ofec_candidate_history_with_future_elections_mv

  1. is ofec_candidate_history_with_future_elections_mv going to replace ofec_candidate_history_mv eventually?
  2. does ofec_candidate_detail_mv have extra data that ofec_candidate_history_mv does not have? ofec_candidate_history seems to have all the future candidate_election_yr. So the union to ofec_candidate_detail_mv may not necessary.
  3. by rounding up future candidate_election_yr ( line 47 ), a few fec_election_year ( the ones in the middle) are missing. This could bring potential issue.
  4. might want to remove plural in table name to follow naming conventions

@vrajmohan @lbeaufort @jwchumley

@lbeaufort
Copy link
Member Author

Thanks so much for taking a look, @hcaofec!

  1. is ofec_candidate_history_with_future_elections_mv going to replace ofec_candidate_history_mv eventually?

I'm not sure it should. As I understand the business logic of ofec_candidate_history_mv , it's similar to disclosure.cand_valid_fec_yr and shows 2-year periods where the candidate had financial activity. No one has had financial activity in 2020 yet.

  1. does ofec_candidate_detail_mv have extra data that ofec_candidate_history_mv does not have? ofec_candidate_history seems to have all the future candidate_election_yr. So the union to ofec_candidate_detail_mv may not necessary.

I needed a way to bring in a row for a future election cycle, in order to join back to the totals. When I tried to union ofec_candidate_history to itself, it got more complicated, and I can't remember why.

  1. by rounding up future candidate_election_yr ( line 47 ), a few fec_election_year ( the ones in the middle) are missing. This could bring potential issue.

I will investigate, thanks!

  1. might want to remove plural in table name to follow naming conventions?

Yes, thanks!

@hcaofec
Copy link
Contributor

hcaofec commented Jun 26, 2018

ran this query in dev
select * from public.ofec_candidate_totals_mv_2020_tmp
where candidate_id in( 'S6CA00584')
order by cycle

fec_cycle 2020 is missing, and not sure if last row of financial data ( cycle=2022) makes sense or not

@jwchumley @lbeaufort

@lbeaufort
Copy link
Member Author

@hcaofec that looks right to me, we wouldn't want 2020 in there because the candidate (S6CA00584) is running in 2022. There should be a cycle for every year there's been financial activity, plus one for the election year.

candidate_id election_year cycle is_election receipts
S6CA00584 2016 2016 True (6-year totals) 15349610.00
S6CA00584 2016 2016 False (2-year totals) 15349610.00
S6CA00584 2022 2018 False (2-year totals) 3499303.00
S6CA00584 2022 2022 True (6-year totals) 3499303.00

@lbeaufort lbeaufort requested a review from rjayasekera June 26, 2018 20:19
@PaulClark2
Copy link
Contributor

@hcaofec @lbeaufort looks correct to me. We need the 2022 row to display the full 6-year period. There's no 2020 row because it's not 2019-20 yet.

candidate_id election_year cycle is_election receipts
S6CA00584 2022 2018 False (2-year totals) 3499303.00
S6CA00584 2022 2022 True (6-year totals) 3499303.00

@fec-jli when I click on the "details" link of security/snyk - data/flyway/build.gradle (fec-jli) above I get an error message: "organisation does not exist." Any ideas why?

@lbeaufort lbeaufort force-pushed the feature/fix-presidential-totals branch from d59707a to 5b00748 Compare June 27, 2018 18:34
@lbeaufort
Copy link
Member Author

lbeaufort commented Jun 27, 2018

Update: I am now using ofec_candidate_history_mv as the 2nd union. @hcaofec and I tested and it looks good. I also updated the testing views.

@lbeaufort lbeaufort force-pushed the feature/fix-presidential-totals branch from 5b00748 to ef48865 Compare June 27, 2018 21:04
@lbeaufort lbeaufort removed the request for review from pkfec June 27, 2018 21:12
@PaulClark2 PaulClark2 merged commit cd9269b into develop Jun 28, 2018
@lbeaufort lbeaufort deleted the feature/fix-presidential-totals branch August 17, 2018 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Presidential 2017 - 2020 data on adv data table not displaying properly
5 participants