fecgov / openFEC-web-app

DEPRECATED See https://github.com/18F/fec-cms for fec.gov's code

Other

43 stars 31 forks source link

Extend efiling window + refactor #2258

Closed noahmanger closed 6 years ago

noahmanger commented 7 years ago

This extends the window for showing efilings on a committee page from two days to three, for all the reasons laid out here.

It also makes this a little easier to configure by making the previous two_days_ago filter into n_days_ago and then setting a constant for EFILING_WINDOW.

Resolves https://github.com/18F/openFEC-web-app/issues/2179

codecov-io commented 7 years ago

Codecov Report

Merging #2258 into develop will increase coverage by 0.02%. The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2258      +/-   ##
===========================================
+ Coverage    55.53%   55.55%   +0.02%     
===========================================
  Files           54       54              
  Lines         3299     3301       +2     
  Branches       398      398              
===========================================
+ Hits          1832     1834       +2     
  Misses        1435     1435              
  Partials        32       32

Impacted Files	Coverage Δ
openfecwebapp/app.py	`72.22% <ø> (ø)`	:arrow_up:
openfecwebapp/constants.py	`100% <100%> (ø)`	:arrow_up:
openfecwebapp/utils.py	`51.45% <100%> (ø)`	:arrow_up:
openfecwebapp/views.py	`39.85% <100%> (+0.43%)`	:arrow_up:

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 9c27862...4c25024. Read the comment docs.

jenniferthibault commented 6 years ago

@LindsayYoung how should we handle this PR now that we've merged repos?

jenniferthibault commented 6 years ago

@LindsayYoung @ccostino this is the last PR/issue in the openFEC-web-app repo. Can you help figure out how we should handle it? The issue Noah pointed to above that this addresses has been moved to fec-cms in https://github.com/18F/fec-cms/issues/1539, which was closed because there was no response to prompts in the comment thread (but I'm not sure that means it was no longer relevant?)

ccostino commented 6 years ago

It looks like based on the conversation history this work will handle the display of things on the front end, but there may not be a noticeable difference as the API doesn't return the information being requested at the moment (it only returns the 2-day period if I understand this correctly).

We could probably go ahead and move this work over to the CMS and merge it without an issue based on what I see here, then circle back on it and see if we still need to make an API adjustment. Does that sound good, @LindsayYoung?

LindsayYoung commented 6 years ago

I think we did this let me double check

LindsayYoung commented 6 years ago

Ok it is not implemented, but it seems like that is because it hasn't been a priority. We thought it would fix a problem about latent reports, but it seems like the cases we had seen before, that wouldn't have helped anyway. I am going to close this for now, it is a simple change so we can just open a new ticket and do this in the future if we think it would be beneficial. (Feel free to re-open if you disagree)

Here is the current code: https://github.com/18F/openFEC-web-app/blob/e23ff54e822ed491bcd78b2ab407c544b39914ac/openfecwebapp/templates/macros/entity-pages.html#L43

Previous discussions: https://github.com/18F/openFEC-web-app/issues/2179 https://github.com/18F/fec-cms/issues/1539