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

Remove 'None' value from list cycles_has_activity in ofec_committee_h… #3427

Merged
merged 2 commits into from
Dec 26, 2019

Conversation

fec-jli
Copy link
Contributor

@fec-jli fec-jli commented Dec 24, 2019

Summary (required)

C00140590 committee profile page is throwing a server error.

  • Resolves #[3426]
    Include a summary of proposed changes.
    modify cms load_committee_history function in views.py
    to clean cycles_has_activity, remove 'None' value in cycles_has_activity
    committee['cycles_has_activity'] = list(filter(None, committee.get('cycles_has_activity')))

Impacted areas of the application

committee profile page

How to test

  1. checkout branch
    2)pytest
    ./manage.py test
    3)npm run test-single
  2. test url, compare with prod.
    (wrong one)
    prod: https://www.fec.gov/data/committee/C00140590/
    local: http://127.0.0.1:8000/data/committee/C00140590/

(correct one should not affected)
prod: https://www.fec.gov/data/committee/C00105668/
local: http://127.0.0.1:8000/data/committee/C00105668/

We should create an separated issue to fix ofec_committee_history_mv

Copy link
Contributor

@jason-upchurch jason-upchurch left a comment

Choose a reason for hiding this comment

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

@fec-jli if we leave the logic as a control on the cms side rather than (or in addition to) sql, then should we also filter for any non-integer value rather than only None? For expediency I am approving now.

@fec-jli
Copy link
Contributor Author

fec-jli commented Dec 24, 2019

@fec-jli if we leave the logic as a control on the cms side rather than (or in addition to) sql, then should we also filter for any non-integer value rather than only None? For expediency I am approving now.

Thanks @jason-upchurch review it. for now, we only get 'None' value from sql query return, not sure if we will get other value. I will talk to database team to see any concern. Thanks.

@jason-upchurch
Copy link
Contributor

@fec-jli if we leave the logic as a control on the cms side rather than (or in addition to) sql, then should we also filter for any non-integer value rather than only None? For expediency I am approving now.

Thanks @jason-upchurch review it. for now, we only get 'None' value from sql query return, not sure if we will get other value. I will talk to database team to see any concern. Thanks.

Thanks @fec-jli I think the datatype in the db is specified as integer so I agree nothing else should come back (because we rely on the root for correct behavior).

Copy link
Contributor

@pkfec pkfec left a comment

Choose a reason for hiding this comment

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

Awesome @fec-jli! . This looks like a data issue. Thanks for adding extra validation when the cycles have bad data likeNone

@codecov-io
Copy link

codecov-io commented Dec 26, 2019

Codecov Report

Merging #3427 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3427      +/-   ##
===========================================
+ Coverage    74.92%   74.92%   +<.01%     
===========================================
  Files          120      120              
  Lines         7349     7351       +2     
  Branches       642      642              
===========================================
+ Hits          5506     5508       +2     
  Misses        1843     1843
Impacted Files Coverage Δ
fec/data/views.py 53.07% <100%> (+0.18%) ⬆️
fec/data/tests/test_committee.py 100% <100%> (ø) ⬆️

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 c59f13c...46106ba. Read the comment docs.

Copy link
Contributor

@jason-upchurch jason-upchurch left a comment

Choose a reason for hiding this comment

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

Looks great @fec-jli nice job!

@fec-jli fec-jli force-pushed the feature/fix_cycles_has_activity_none branch from 6a67678 to 46106ba Compare December 26, 2019 21:10
Copy link
Member

@lbeaufort lbeaufort left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @fec-jli!

@lbeaufort lbeaufort merged commit c19d350 into develop Dec 26, 2019
@lbeaufort lbeaufort deleted the feature/fix_cycles_has_activity_none branch December 26, 2019 21:32
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.

5 participants