Feature/4718 pac snapshot #4767
Conversation
rfultz
requested review from
johnnyporkchops,
patphongs,
lbeaufort and
JonellaCulmer
Codecov Report
@@ Coverage Diff @@
## develop #4767 +/- ##
===========================================
- Coverage 75.77% 75.24% -0.54%
===========================================
Files 125 125
Lines 7690 7771 +81
Branches 622 622
===========================================
+ Hits 5827 5847 +20
- Misses 1863 1924 +61
Continue to review full report at Codecov.
|
@rfultz I think the approach is good and it's also clear as to what committee types are relevant. In fact, you'll need it for more than just the glossary, we'll need it to determine whether or not to even show the snapshot on the page, since we're only doing this for certain committees right now. |
only showing the rest of the sentence if there are values to put in it
| {% else %} | ||
| {% set rec_ind_l = '!' %} | ||
| {% set rec_ind_ring = '0 100' %} | ||
| {% set rec_ind_class = '' %} |
There was a problem hiding this comment.
@rfultz I think this applies to if it's 0 then give the "!" error state? Do we really want to do that anytime it's 0? Are there currently any errors thrown from the API right now when it can't be calculated?
There was a problem hiding this comment.
New code just committed includes == 0 values in the outer if statement
There was a problem hiding this comment.
Looks good @rfultz, thanks for making those quick fixes. My comment about the error state isn't a blocker to getting this merged in, but I would like to have more error handling logic worked out when we're able to fine tune it.
|
@rfultz Here are my comments on changes. Nothing serious that blocks a merge under the feature flag, but I'd really like to take a deeper dive into the math to make sure it works for parties, SSFs and other committees that may not fit the original design, like we discussed during demo. I would like these changes before launch, in that sense these would be blockers. But not today.
|
|
New updates on the way! There's a spreadsheet of committees with outlier values |
|
@rfultz Everything looks really great. The only thing we missing is that some committees are SSFs and require two glossary terms. I've highlighted them in the spreadsheet in green. https://docs.google.com/spreadsheets/d/1WkNHCI-vXqxM1ZOSK0d-HZp9Sbx3Tj0CWnoKiC9rjbE/edit#gid=0. I know we're still waiting on some definitions. So if you're planning to do all the remaining glossary terms at the same time I'm fine with that. |
The rest of the glossary changes will be handled in this ticket: #4793. I'll merge in the current code so that we have this snapshot feature under the feature flag. |
Summary
Building the PAC snapshot
Required reviewers
UX, front-end (x2?), back-end to check the Python (I don't love this proposal)
Impacted areas of the application
Adds the snapshot to the committees-single template, adds a couple new variables to get_committee in views.py
Screenshots
(Include a screenshot of the new/updated features in context (“in the wild”). If it is an interface change, include both before and after screenshots)
Related PRs
None
How to test
npm run build./manage.py runserverKnown issues