Use facets for metadata #148

tommyp · 2015-01-21T17:51:13Z

This PR contains the work necessary to reduce specificity in FinderFrontend. To do this we need to remove each Document's Model and use the Finder to figure out what fields we are asking to be returned from Rummager and what metadata values should we show for each document. The way I've done this is adding the concept of a FilterableFacet and changing Facet to represent information about a set of metadata key/value pairs and if this should be filterable we then suggest a type of {multi-select|date} and it becomes a FilterableFacet.

The commits roughly go as follows:

Use the Finder's facets to tell Rummager what values we want returned
Remove document Models and Parser
Refactor the facets to become Facet and FilterableFacet and expose Finder#metadata and Finder#filters
Allow metadata labels for when the name of the facet isn't what we want to label the metadata with
Set a summaries flag on the Finder to decide if we should show them
Refactor AbstractDocument to Document and use the facets to convert metadata values to labels
Pass the FinderPresenter object down to the Document
Cleanup the features by updating the fixture content item with the metadata flags and the closed_date facet

After some discussion with @evilstreak I took a bit of a different approach and added extra commits which do the following:

Set if the facet should be filterable and if it should be displayed as result metadata by having every type of facet be a Facet class or one of it's child classes.
Remove filterable being hardcoded into the Filterable Facet class, but we still use this class so we can set the values of the facets.
FacetParser now uses this new way of initialising the Facets.
Use these new type of facets to specify tag and date metadata for the document
Add the updated schema attrs to the CMA Cases content item fixture.

The additional commits were made to handle edge cases in:

International Development Funding where we have metadata which is filterable but not displayed under the results.
AAIB Reports which has free text fields which aren't filterable, though these are only displayed in SpecialistFrontend.

KushalP · 2015-01-26T14:38:58Z

app/presenters/finder_presenter.rb

@@ -64,8 +76,7 @@ def related

  def results
    @results ||= ResultSet.get(
-      slug,
-      document_type,
+      self,


I'm really unsure about this. It makes the owning object unclear.

My gut says to start by removing calls to the some_finder_presenter.results method here with direct uses of ResultSet, but I'm not entirely sure.

tommyp · 2015-02-02T16:26:02Z

Hold off merging this while I do some investigation. Think I've found a small issue with DSU.

tommyp · 2015-02-02T17:10:56Z

This is good to go now. I noticed some weirdness with the metadata labels not being changed to short_name so I fixed that and cleaned up one of the commits which was changed later.

First pass at a doc for what the Finder Content Item should contain. Leaving the facets for now until #148 is merged.

evilstreak · 2015-02-03T16:22:31Z

app/models/document.rb

+    description = description.gsub(/\.\s[A-Z].*/, '.') if description.present? && finder.show_summaries?
+  end
+
+  def date_metadata_keys


I think all these metadata key and mapping methods can be private

evilstreak · 2015-02-03T17:00:31Z

Review still in progress! TBC

evilstreak · 2015-02-04T17:24:14Z

app/models/document.rb

+
+    # This truncates the description at the end of the first sentence
+    description = description.gsub(/\.\s[A-Z].*/, '.') if description.present? && finder.show_summaries?
+  end


What about something like this to break it down a bit?

def summary if finder.show_summaries? && description.present? description.gsub(/\.\s[A-Z].*/, '.') end end private # or in the initializer def description attrs.fetch(:description, nil) end

evilstreak · 2015-02-05T13:47:32Z

app/models/document.rb

-
-  def initialize(attrs)
+  def initialize(attrs, finder)
+    attrs = attrs.with_indifferent_access


What prompted the change to turn this into a HashWithIndifferentAccess?

All the test data is JSON, but the hash returned in the real world is symbols so all these setters in the initialize method fail in the tests. Should I change all the data?

We should make the test data reflect the real world data – otherwise someone could remove this line and replace all the fetch arguments with strings, and the tests would indicate everything is fine

For reference for anyone else this was discussed offline. This line was originally in https://github.com/alphagov/finder-frontend/blob/master/app/parsers/document_parser.rb#L3 but because we don't have a DocumentParser anymore I moved this line to the initialize method of Document.

evilstreak · 2015-02-05T13:59:48Z

Sorry for adding another round of comments. This really is good work.

I'll come over and work with you to get it merged today.

This commit removes the document models which defined what attrs to be returned as metadata and if a summary should be shown.

As we no longer need to parse documents, we can remove this file and it's spec.

We can now use the facets to generate the metadata apart from certain metadata keys which not have a corresponding filter so we need to introduce a distinction. This commit does a fair bit of refactoring around this as well as introducing a `filterable?` method to the existing facet types. I've also exposed methods for `Finder#filters` and `Finder#metadata` to access these more easily. I've also replace the existing Facet class with one which is unfilterable and all the other facets are subclasses from FilterableFacet which subclasses from Facet itself.

This commit alows us to see if the Finder should show summaries.

Now that we don't need this Abstract class we can rename it to Document. This commit also refactors it to recieve the Finder which it uses to see if it should have a summary along with the facets which are used to do metadata value to label conversion.

As we use the Finder and it's Facets we need to pass it down through to the Document. This commit updates the ResultSet model, the ResultSetParser and the latter's spec.

This class still exists so we can set the values, but now we can define filterable using the facet attrs set in the schema.

This commit updates the FacetParser to handle unfilterable facets properly and updates it's spec.

With the new facet structure, we can use this to figure out what are dates and what are text based metadata.

This commit adds the short_names to the facets in the CMA Case Finder fixture along with the closed date facet.

This new method is a bit cleaner than what was there previously.

This new method removes an unnecessary method and moves the short_name call to the Finder.

Use facets for metadata

evilstreak · 2015-02-06T09:04:37Z

🎆

tommyp · 2015-02-06T09:05:29Z

🎉🎉

KushalP reviewed Jan 26, 2015
View reviewed changes

tommyp force-pushed the use-facets-for-metadata branch 3 times, most recently from 7357fd9 to a743bae Compare January 30, 2015 11:35

tommyp mentioned this pull request Feb 2, 2015

Add doc for explaining Finder Content Item #155

Closed

tommyp changed the title ~~Use facets for metadata~~ DO NOT MERGE Use facets for metadata Feb 2, 2015

tommyp force-pushed the use-facets-for-metadata branch 4 times, most recently from 917d9ec to 4ca0b5e Compare February 2, 2015 17:08

tommyp changed the title ~~DO NOT MERGE Use facets for metadata~~ Use facets for metadata Feb 2, 2015

tommyp added a commit that referenced this pull request Feb 3, 2015

Add doc for explaining Finder Content Item

7917eb1

First pass at a doc for what the Finder Content Item should contain. Leaving the facets for now until #148 is merged.

tommyp mentioned this pull request Feb 3, 2015

Add doc for explaining Finder Content Item #156

Merged

evilstreak reviewed Feb 3, 2015
View reviewed changes

tommyp force-pushed the use-facets-for-metadata branch from 4ca0b5e to 5952134 Compare February 3, 2015 18:26

evilstreak reviewed Feb 4, 2015
View reviewed changes

tommyp force-pushed the use-facets-for-metadata branch 2 times, most recently from e61d488 to 00ce854 Compare February 5, 2015 11:01

tommyp force-pushed the use-facets-for-metadata branch 2 times, most recently from 04506c1 to 8778e01 Compare February 5, 2015 11:33

evilstreak reviewed Feb 5, 2015
View reviewed changes

tommyp added 14 commits February 5, 2015 14:15

Use Finder for Rummager return fields

5d006bd

Remove format specific models

00dbbec

This commit removes the document models which defined what attrs to be returned as metadata and if a summary should be shown.

Remove document parser

e6d13a2

As we no longer need to parse documents, we can remove this file and it's spec.

Set a summaries flag on the Finder

7df8260

This commit alows us to see if the Finder should show summaries.

AbstractDocument => Document

27f5eb4

Now that we don't need this Abstract class we can rename it to Document. This commit also refactors it to recieve the Finder which it uses to see if it should have a summary along with the facets which are used to do metadata value to label conversion.

Pass the Finder object down to the document

ccf4767

As we use the Finder and it's Facets we need to pass it down through to the Document. This commit updates the ResultSet model, the ResultSetParser and the latter's spec.

Don't set filterable in the filterable facet class

6e673ae

This class still exists so we can set the values, but now we can define filterable using the facet attrs set in the schema.

Set filterable? and metadata? from schema

6793526

FacetParser now handles unfilterable facets

b71af8b

This commit updates the FacetParser to handle unfilterable facets properly and updates it's spec.

Use new facet structure for date and metadata

badd4b4

With the new facet structure, we can use this to figure out what are dates and what are text based metadata.

Add new schema stuff to CMA cases fixture

685bc29

This commit adds the short_names to the facets in the CMA Case Finder fixture along with the closed date facet.

Refactor summaries method

773029c

This new method is a bit cleaner than what was there previously.

Refactor Document#metadata_name_mappings

17fd23d

This new method removes an unnecessary method and moves the short_name call to the Finder.

tommyp force-pushed the use-facets-for-metadata branch from 8778e01 to 17fd23d Compare February 5, 2015 14:15

tommyp mentioned this pull request Feb 5, 2015

Use new facet structure alphagov/contacts-admin#190

Merged

evilstreak added a commit that referenced this pull request Feb 6, 2015

Merge pull request #148 from alphagov/use-facets-for-metadata

21ea795

Use facets for metadata

evilstreak merged commit 21ea795 into master Feb 6, 2015

evilstreak deleted the use-facets-for-metadata branch February 6, 2015 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use facets for metadata #148

Use facets for metadata #148

tommyp commented Jan 21, 2015

KushalP Jan 26, 2015

tommyp commented Feb 2, 2015

tommyp commented Feb 2, 2015

evilstreak Feb 3, 2015

evilstreak commented Feb 3, 2015

evilstreak Feb 4, 2015

evilstreak Feb 5, 2015

tommyp Feb 5, 2015

evilstreak Feb 5, 2015

tommyp Feb 6, 2015

evilstreak commented Feb 5, 2015

evilstreak commented Feb 6, 2015

tommyp commented Feb 6, 2015

Use facets for metadata #148

Use facets for metadata #148

Conversation

tommyp commented Jan 21, 2015

KushalP Jan 26, 2015

Choose a reason for hiding this comment

tommyp commented Feb 2, 2015

tommyp commented Feb 2, 2015

evilstreak Feb 3, 2015

Choose a reason for hiding this comment

evilstreak commented Feb 3, 2015

evilstreak Feb 4, 2015

Choose a reason for hiding this comment

evilstreak Feb 5, 2015

Choose a reason for hiding this comment

tommyp Feb 5, 2015

Choose a reason for hiding this comment

evilstreak Feb 5, 2015

Choose a reason for hiding this comment

tommyp Feb 6, 2015

Choose a reason for hiding this comment

evilstreak commented Feb 5, 2015

evilstreak commented Feb 6, 2015

tommyp commented Feb 6, 2015