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

Feature/fix typeahead sort #178

Merged
merged 2 commits into from
May 14, 2015
Merged

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented May 14, 2015

Fix grouping in main typeahead search.

The actual issue here turned out to be that we were checking the
candidate and committee IDs against null but were getting back a
different falsy value of ''. This patch uses a more general check,
ignoring results with all falsy ID values, and revises the filtering
logic to use a more explicit filter and map, rather than the implicit
filtering in $.map.

[Resolves https://github.com/18F/openFEC/issues/741]

The actual issue here turned out to be that we were checking the
candidate and committee IDs against `null` but were getting back a
different falsy value of `''`. This patch uses a more general check,
ignoring results with all falsy ID values, and revises the filtering
logic to use a more explicit filter and map, rather than the implicit
filtering in `$.map`.

[Resolves https://github.com/18F/openFEC/issues/741]
@@ -10,6 +11,10 @@

from openfecwebapp.sauce import SauceClient

# Silence Selenium logs
remote_logger = logging.getLogger('selenium.webdriver.remote.remote_connection')
remote_logger.setLevel(logging.WARN)
Copy link
Contributor

Choose a reason for hiding this comment

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

A++++++++++

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Does this mean we'll just see warnings and failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this logger, we'll only see messaged logged at WARN, ERROR, or CRITICAL. All other loggers are unaffected.

@arowla arowla self-assigned this May 14, 2015
arowla added a commit that referenced this pull request May 14, 2015
@arowla arowla merged commit eefb76e into fecgov:develop May 14, 2015
@jmcarp
Copy link
Contributor Author

jmcarp commented May 14, 2015

By the way, in the spirit of disclosure, I now realize that I broke this in the first place on the API side. We're currently using the latest stable version of marshmallow for serialization, and null values are serialized as empty strings. I actually brought this up with the marshmallow developer a few weeks ago, and he fixed that behavior such that nulls serialize to nulls, but I didn't notice that we needed to upgrade to the dev version of marshmallow to get this change. I'll do that later today.

Shorter version: sorry @noahmanger!

@noahmanger
Copy link
Contributor

No worries! Thanks for the transparency!

On Thu, May 14, 2015 at 9:38 AM, Joshua Carp notifications@github.com
wrote:

By the way, in the spirit of disclosure, I now realize that I broke this
in the first place on the API side. We're currently using the latest stable
version of marshmallow for serialization, and null values are serialized as
empty strings. I actually brought this up with the marshmallow developer a
few weeks ago, and he fixed that behavior such that nulls serialize to
nulls, but I didn't notice that we needed to upgrade to the dev version of
marshmallow to get this change. I'll do that later today.

Shorter version: sorry @noahmanger https://github.com/noahmanger!


Reply to this email directly or view it on GitHub
#178 (comment).

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

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.

None yet

3 participants