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

Included amended F3* records. #1289

Merged
merged 2 commits into from
Oct 22, 2015

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Oct 22, 2015

  • Include F3* records with non-null expire_date
  • Add "amended" filter to committee reports views
  • Index expire_date

Note: this requires a quick patch to the frontend so that only current
reports are requested for the committee detail view. This will be
submitted shortly.

[Resolves https://github.com/fecgov/openFEC-web-app/issues/874]

* Include F3* records with non-null `expire_date`
* Add "amended" filter to committee reports views
* Index `expire_date`

Note: this requires a quick patch to the frontend so that only current
reports are requested for the committee detail view. This will be
submitted shortly.

[Resolves fecgov/openFEC-web-app#874]
@jmcarp
Copy link
Contributor Author

jmcarp commented Oct 22, 2015

One point about this: we're adding a new query parameter to the filings endpoint called "amended". If "true", only show amended records; if "false", only show non-amended records; if not specified, show all records. This is a concise way to describe the filtering rules, but it occurs to me that it might be confusing. Happy to revise if folks have better ideas, but this does solve the immediate problem.

cc @LindsayYoung

@LindsayYoung
Copy link
Contributor

Yea the language on that is a bit confusing.

How about:
is_amended=true only records that have been amended
is_amended=false only records that have not been amended

@noahmanger
Copy link

Will we also still have the amendment_indicator parameter? This could be confusing to have both.

@LindsayYoung
Copy link
Contributor

How about keeping this for now, so that there isn't breaking changes and doing this as part of some systematic clean up later?

@noahmanger
Copy link

Could we keep it on the API but remove the filter on the /filings page and
replace it with the new one?

On Thu, Oct 22, 2015 at 12:00 PM, Lindsay Young [email protected]
wrote:

How about keeping this for now, so that there isn't breaking changes and
doing this as part of some systematic clean up later?


Reply to this email directly or view it on GitHub
https://github.com/18F/openFEC/pull/1289#issuecomment-150323346.

Noah Manger
18F http://18f.gsa.gov | Work: 415-696-6146

@LindsayYoung
Copy link
Contributor

Absolutely, @noahmanger!

@jmcarp
Copy link
Contributor Author

jmcarp commented Oct 22, 2015

Could we keep it on the API but remove the filter on the /filings page and
replace it with the new one?

Well, no. The new filter ("amended", potentially to be renamed to "is_amended") applies to the reports view, not the filings view. We don't currently have that information for filings.

@LindsayYoung
Copy link
Contributor

I forgot we don't have that view. Thanks Josh

@noahmanger
Copy link

Ah. Ok. So... this stays?
image

How will the new filter be applied?

@jmcarp
Copy link
Contributor Author

jmcarp commented Oct 22, 2015

@noahmanger this patch makes zero changes to the filings view, so that filter doesn't need to change. I'm not sure if it's clear or useful, but that's a separate question.

@noahmanger
Copy link

Gotcha. So amended reports will still be distinguished by amendment_indicator=N and amendments will have amendment_indicator=A?

@LindsayYoung
Copy link
Contributor

We don't have a good way to show what has been amended in the filings, which is what is listed and filterable on that page.

Once you click on this to look at the panel, we have the reports view, which will have the true/false for amendments.

is_amended true/false form the reports and N, A or null from filings don't correspond to each other because one table only knows if something has been amended and the other only knows if the record is an amendment.

@LindsayYoung
Copy link
Contributor

LGTM will merge when tests pass.

LindsayYoung added a commit that referenced this pull request Oct 22, 2015
@LindsayYoung LindsayYoung merged commit dc46558 into fecgov:develop Oct 22, 2015
LindsayYoung added a commit that referenced this pull request Oct 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants