Refactor candidate view, fix 404's for future candidates #2320

lbeaufort · 2018-08-29T17:32:15Z

Summary (required)

Replaces PR Refactor candidate view, fix 404's for future candidates #2316 - bad branch name
Resolves 404 Errors when clicking through to individual candidate/committee pages #2294
Refactor candidate view, fix 404's for future cycle candidates without committees, simplify 2-year select macro and use it for the candidate raising/spending pages

Impacted areas of the application

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

Candidate profile pages
Candidate raising/spending pages now show two-year period label (that's what we display) instead of election (election label was incorrect). Committee two-year period dropdowns should be unchanged - renamed to two_year_select
Test previously 404-ing candidate pages:
http://localhost:8000/data/candidate/H0TX09168/
http://localhost:8000/data/candidate/P40003832/
Test future/past House/Senate/Presidential pages

Screenshots:

Before

After

Related PRs

#2302 Add tests for candidate view
#2311 Refactor tests for candidate view

Fix line lengths, whitespace, remove statements that do nothing

Use @library.filter filters.date()

max_cycle and cycle will always be the same value

This change originally came from PR fecgov/openFEC-web-app#1945. After much investigation, I can't see any use case for this logic given the current setup, so I'm removing it. The user interface ensures on the "financial summary", "about this candidate", and "spending by other tabs", where the cycle options correspond with election_years, election_year/cycle will always be a valid election year.

The max() function in "Annotate committees with most recent available cycle" was throwing errors - this simplifies the logic

Allow two_year_select to be passed election_full values for candidate pages

codecov-io · 2018-08-29T20:42:53Z

Codecov Report

Merging #2320 into develop will increase coverage by 0.01%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           develop    #2320      +/-   ##
===========================================
+ Coverage    74.06%   74.08%   +0.01%     
===========================================
  Files          112      112              
  Lines         6582     6567      -15     
  Branches       807      800       -7     
===========================================
- Hits          4875     4865      -10     
+ Misses        1655     1654       -1     
+ Partials        52       48       -4

Impacted Files	Coverage Δ
fec/data/tests/test_candidate.py	`100% <100%> (ø)`	⬆️
fec/data/views.py	`33.51% <85%> (-1.9%)`	⬇️

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 e6f7998...32abdd3. Read the comment docs.

lbeaufort · 2018-08-29T20:50:03Z

fec/data/templates/macros/cycle-select.jinja

-{% set cycle = cycle | int %}
-  <div class="content__section">
-    <label for="{{id}}-cycle" class="label cycle-select__label">Election</label>
-    <select id="{{id}}-cycle" class="js-cycle" name="cycle" data-cycle-location="query" data-election-full="False">


This should be data-election-full="false", lower case (fixed with this commit). It was causing a bug where going from raising/spending pages on an off-election year and then back to the financial summary tab wasn't selecting the 2-year time period button:

vrajmohan

Made some minor comments. I can merge it even without those being addressed.

vrajmohan · 2018-08-31T15:26:10Z

fec/data/templates/partials/candidate/about-candidate.jinja

@@ -48,7 +48,7 @@
                {% if statement.fec_file_id %}
                  <div class="t-small u-small-icon-padding--left"> {{ statement.fec_file_id }}</div>
                {% endif %}
-                <div class="u-small-icon-padding--left"> Filed {{ statement.receipt_date }}</div>
+                <div class="u-small-icon-padding--left"> Filed {{ statement.receipt_date|date }}</div>


vrajmohan · 2018-08-31T15:32:08Z

fec/data/views.py

@@ -126,7 +126,7 @@ def get_candidate(candidate_id, cycle, election_full):

    result_type = 'candidates'
    duration = election_durations.get(candidate['office'], 2)
-    min_cycle = cycle - duration if election_full else cycle
+    cycle_start_year = cycle - duration if election_full else cycle


This is still confusing to me. If the cycle is 2016 and the office is "President", isn't the cycle start year 2011 and not 2010?

vrajmohan · 2018-08-31T15:33:44Z

fec/data/views.py

-    max_cycle = cycle if cycle <= utils.current_cycle() else utils.current_cycle()
-    show_full_election = election_full if cycle <= utils.current_cycle() else False
+    cycles = [
+        cycle_tmp for cycle_tmp in candidate['cycles']


Minor - it is cleaner to use cycle as the list comprehension variable as done in line 164 below.

vrajmohan · 2018-08-31T15:37:12Z

fec/data/views.py

@@ -252,8 +254,7 @@ def candidate(request, candidate_id):
    if cycle is not None:
        cycle = int(cycle)

-    election_full = request.GET.get('election_full', True)
-    election_full = bool(strtobool(str(election_full)))
+    election_full = bool(strtobool(request.GET.get('election_full', 'True')))


Minor - do we really need distutils.util.strtobool? Isn't the input controlled by us or specified in our API?

I agree it seems overly complex - I'm planning to address this here so I can test thoroughly.

vrajmohan · 2018-08-31T15:39:26Z

fec/data/views.py

@@ -105,32 +105,6 @@ def get_candidate(candidate_id, cycle, election_full):
        election_full=show_full_election,
    )

-    # cycle corresponds to the two-year period for which the committee has


Well done! Nice work on the commit message as well.

vrajmohan · 2018-08-31T15:42:47Z

fec/data/views.py

@@ -127,20 +127,6 @@ def get_candidate(candidate_id, cycle, election_full):
        if cycle_in_range(cycle_tmp)
    ]

-    # Annotate committees with most recent available cycle


The commit message could say more about why this complex calculation was not needed, other than the fact that max was throwing errors.

apburnes

Looks good and runs well.

lbeaufort and others added 6 commits August 28, 2018 14:56

Keep cycle consistent for base case

2aa0828

Reformat the code

bf61f0c

Fix line lengths, whitespace, remove statements that do nothing

Move statement of candidacy receipt_date formatting to jinja template

70be68e

Use @library.filter filters.date()

Replace max_cycle with cycle

c430f4a

max_cycle and cycle will always be the same value

Rename min_cycle to cycle_start_year

c0279aa

Simplify get_candidate logic

e341891

lbeaufort mentioned this pull request Aug 29, 2018

Refactor candidate view, fix 404's for future candidates #2316

Closed

lbeaufort changed the title ~~Feature/refactor candidate view~~ Refactor candidate view, fix 404's for future candidates Aug 29, 2018

lbeaufort requested review from vrajmohan, apburnes and patphongs August 29, 2018 17:32

lbeaufort added Please review Before release labels Aug 29, 2018

patphongs changed the title ~~Refactor candidate view, fix 404's for future candidates~~ [WIP] Refactor candidate view, fix 404's for future candidates Aug 29, 2018

lbeaufort added 3 commits August 29, 2018 16:09

Replace committee.related_cycle with view cycle

f7fb4e9

The max() function in "Annotate committees with most recent available cycle" was throwing errors - this simplifies the logic

Simplify candidate/committee 2-year-select macros

32abdd3

Allow two_year_select to be passed election_full values for candidate pages

lbeaufort force-pushed the feature/refactor-candidate-view branch from a9c5787 to 32abdd3 Compare August 29, 2018 20:13

lbeaufort changed the title ~~[WIP] Refactor candidate view, fix 404's for future candidates~~ Refactor candidate view, fix 404's for future candidates Aug 29, 2018

lbeaufort commented Aug 29, 2018

View reviewed changes

lbeaufort requested a review from johnnyporkchops August 31, 2018 15:25

vrajmohan reviewed Aug 31, 2018

View reviewed changes

apburnes approved these changes Aug 31, 2018

View reviewed changes

lbeaufort mentioned this pull request Aug 31, 2018

Look into getting rid of 'strtobool' #2326

Open

vrajmohan merged commit d1cc4f8 into develop Aug 31, 2018

vrajmohan deleted the feature/refactor-candidate-view branch August 31, 2018 19:36

johnnyporkchops mentioned this pull request Sep 1, 2018

Feature/candidate filings tab new #2310

Merged

johnnyporkchops mentioned this pull request Sep 10, 2018

Test candidate filings tab by 9/20/2018 #2339

Closed

6 tasks

patphongs mentioned this pull request Sep 10, 2018

404 on past candidates #2343

Closed

patphongs mentioned this pull request Sep 11, 2018

Refactor and bug fix candidate profile page logic #2349

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor candidate view, fix 404's for future candidates #2320

Refactor candidate view, fix 404's for future candidates #2320

lbeaufort commented Aug 29, 2018

codecov-io commented Aug 29, 2018

lbeaufort Aug 29, 2018 •

edited

Loading

vrajmohan left a comment

vrajmohan Aug 31, 2018

vrajmohan Aug 31, 2018

vrajmohan Aug 31, 2018

vrajmohan Aug 31, 2018

lbeaufort Aug 31, 2018

vrajmohan Aug 31, 2018

vrajmohan Aug 31, 2018

apburnes left a comment

Refactor candidate view, fix 404's for future candidates #2320

Refactor candidate view, fix 404's for future candidates #2320

Conversation

lbeaufort commented Aug 29, 2018

Summary (required)

Impacted areas of the application

Screenshots:

Before

After

Related PRs

codecov-io commented Aug 29, 2018

Codecov Report

lbeaufort Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

vrajmohan left a comment

Choose a reason for hiding this comment

vrajmohan Aug 31, 2018

Choose a reason for hiding this comment

vrajmohan Aug 31, 2018

Choose a reason for hiding this comment

vrajmohan Aug 31, 2018

Choose a reason for hiding this comment

vrajmohan Aug 31, 2018

Choose a reason for hiding this comment

lbeaufort Aug 31, 2018

Choose a reason for hiding this comment

vrajmohan Aug 31, 2018

Choose a reason for hiding this comment

vrajmohan Aug 31, 2018

Choose a reason for hiding this comment

apburnes left a comment

Choose a reason for hiding this comment

lbeaufort Aug 29, 2018 •

edited

Loading