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

Official Documents Finder #1154

Merged
merged 2 commits into from Jun 5, 2019

Conversation

Projects
None yet
4 participants

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 28, 2019 Inactive

@hannako hannako force-pushed the official_doc_finder branch from 2c859df to c9ff01f May 28, 2019

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 28, 2019 Inactive

@hannako hannako force-pushed the official_doc_finder branch from c9ff01f to 1707820 May 29, 2019

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 29, 2019 Inactive

@hannako hannako force-pushed the official_doc_finder branch from 1707820 to fd6e1b5 May 29, 2019

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 29, 2019 Inactive

@hannako hannako force-pushed the official_doc_finder branch from fd6e1b5 to 0701e5d May 29, 2019

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 29, 2019 Inactive

@hannako hannako marked this pull request as ready for review May 29, 2019

@hannako hannako force-pushed the official_doc_finder branch from 0701e5d to 34dcfc6 May 29, 2019

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 29, 2019 Inactive

@hannako hannako force-pushed the official_doc_finder branch from 34dcfc6 to 30391db May 29, 2019

@benthorner benthorner temporarily deployed to finder-frontend-pr-1154 May 29, 2019 Inactive

@bilbof bilbof self-requested a review May 30, 2019

@bilbof
Copy link
Contributor

left a comment

I think we can make this code more generic. Could we define a single class (for filter and for facet) and pass in the difference as arguments?

It looks like OfficialDocumentsFacet and ResearchAndStatisticsFacet, and OfficialDocumentsFilter and ResearchAndStatisticsFilter have a fair amount of duplicated code.

@sihugh

sihugh approved these changes May 30, 2019

Copy link
Contributor

left a comment

Looks good to me 👍 (For some reason I didn't see what Bill said at all)

@hannako

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@bilbof thanks bill. My thought was that the duplication was fine until a third custom facet and filter is required and then I should refactor. But am happy to do so now.

@bilbof

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I'd remove the duplicated code now, while we're very familiar with it. Future us might forget how this stuff works.

If we can extract the duplicated stuff into additional classes that will make future changes easier.

For example, since the only difference between OfficialDocumentsFacet and ResearchAndStatisticsFacet is the filter_hashes, you could have a method called filter_hashes in both of these classes, and have them inherit from RadioFacetWithRequiredValue (or something like that), a new class that uses those. If you can think of a way to do this with composition rather than inheritance that would be even better.

I would also see - this might be out of scope for this PR - if filter_hashes could give an array of classes rather than hashes. This would make the code less dependent on mutable filter_hashes, and instead on a collection of immutable (e.g.) Filter classes.

Official Documents Finder
This commit allows finder frontend to render a new official
documents finder. Official documents are any publication
with an attachment that is a command paper or an act paper.
The JSON has not yet been defined in Search API and
published to the content store. So an official_doc JSON fixture
is added for development. It will be removed in a later commit
once search-api/config/finders/official_documents_finder.yml
has been published.

Trello:
https://trello.com/c/6xFJqfAw/724-spin-up-finder-for-content-that-has-official-doc-status-s

@hannako hannako force-pushed the official_doc_finder branch from 30391db to 783571e Jun 3, 2019

@sihugh

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@hannako there are some linting failures on the build :-(

@hannako hannako force-pushed the official_doc_finder branch from c15ba83 to c7258cb Jun 4, 2019

Refactor of custom radio button facets and filters
The existing radio button facet and filter classes
assume search results will be filtered on a single field.
As the need has arisen to filter on multiple fields, this commit
refactors to use inheritance to create two more abstract superclasses,
with custom code moved into the subclasses.
@bilbof

bilbof approved these changes Jun 5, 2019

Copy link
Contributor

left a comment

Nice work! 🎉

@hannako hannako merged commit 25addb6 into master Jun 5, 2019

3 checks passed

continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
continuous-integration/jenkins/security No security issues found
Details

@hannako hannako deleted the official_doc_finder branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.