Skip to content
This repository has been archived by the owner on Dec 23, 2017. It is now read-only.

Changed FEC PDF links to be built by a filter #194

Merged
merged 2 commits into from
May 26, 2015
Merged

Conversation

arowla
Copy link
Contributor

@arowla arowla commented May 21, 2015

  • Added check in template to make sure we don't try building a
    non-existent link

- Added check in template to make sure we don't try building a
  non-existent link
</div>
<div class="table__row--note">
Source: <a href="http://docquery.fec.gov/pdf/{{ reports.beginning_image_number|last_n_characters }}/{{ reports.beginning_image_number }}/{{ reports.beginning_image_number }}.pdf">{{reports.report_type_full|fmt_report_desc|default("unavailable")}} {{ reports.report_year }}</a>
{% if report %}
Source: <a href="{{ report|url_to_fec_pdf }}">{{report.report_type_full|fmt_report_desc|default("unavailable")}} {{ report.report_year }}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this code lead to a case where we generate HTML like <a href="unavailable">...</a>? Disregard previous comment--this is the question I meant to ask 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the behavior on /candidate/H2OH06086... It takes the "else" route and says "Source: None".

Copy link
Contributor

Choose a reason for hiding this comment

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

Good. But if that's the case, then is the default("unavailable") logic on this line unreachable? If so, seems like we don't need it, and if not, this could lead to an edge case where we get "unavailable" in the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well in this case, the "unavailable" is only in the case that we don't have a full report description. We ought to have that if we have a report, but I'm not 100% certain if that will always be the case. So what if we replace the default link text "unavailable" with something like default("Committee Report")?

- Went with just "Report" to be totally generic.
</div>
<div class="table__row--note">
Source: <a href="http://docquery.fec.gov/pdf/{{ reports.beginning_image_number|last_n_characters }}/{{ reports.beginning_image_number }}/{{ reports.beginning_image_number }}.pdf">{{reports.report_type_full|fmt_report_desc|default("unavailable")}} {{ reports.report_year }}</a>
{% if report %}
Source: <a href="{{ report|url_to_fec_pdf }}">{{report.report_type_full|fmt_report_desc|default("Report")}} {{ report.report_year }}</a>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmcarp I went ahead and changed the default report text to read just "Report" in the case that we have a report but for some reason the report_type_full is not filled in. Does that work, do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jmcarp jmcarp merged commit 1252631 into develop May 26, 2015
@noahmanger noahmanger deleted the feature/fec-pdf-links branch August 3, 2015 22:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants