This repository has been archived by the owner. It is now read-only.

Use Finder for metadata #81

Merged
merged 6 commits into from Feb 3, 2015

Conversation

Projects
None yet
3 participants
@tommyp
Contributor

tommyp commented Jan 30, 2015

As we now store all the Facets in the ContentStore, we can remove all of the code in this App for specifying each Document format.

The commits go as follows:

  • removing Presenters for each format
  • removing tests for each format
  • expose the Facets and the format name from the Finder model
  • add a new DocumentPresenter class which we pass the Finder into class to get the format name and the facets of the Finder. These then describe all the Metadata that a document format may have. We then use the keys of these facets to define methods which call document.details.#{method}. We also use these facets to get a hash of the extra_date_metadata, filterable_metadata and extra_metadata.
  • refactor SpecialistDocumentsController to get the Finder content_item from the first part of the path. We then pass this into the DocumentPresenter to use. This allows us to get rid of all the code checking the slug and initialising a format Presenter class.

@tommyp tommyp changed the title from Use finder for metadata to Use Finder for metadata Jan 30, 2015

@dsingleton

This comment has been minimized.

Show comment
Hide comment
@dsingleton

dsingleton Jan 30, 2015

Don't know finders well enough to review this fully, but diff stats are awesome.

Don't know finders well enough to review this fully, but diff stats are awesome.

@tommyp

This comment has been minimized.

Show comment
Hide comment
@tommyp

tommyp Jan 30, 2015

Contributor

So it turns out there's a few edge cases with regards to dates that I hadn't considered. Hold off merging this until I fix it, but feel free to comment.

Contributor

tommyp commented Jan 30, 2015

So it turns out there's a few edge cases with regards to dates that I hadn't considered. Hold off merging this until I fix it, but feel free to comment.

@tommyp tommyp changed the title from Use Finder for metadata to DO NOT MERGE Use Finder for metadata Jan 30, 2015

@@ -5,9 +5,10 @@ class SpecialistDocumentsController < ApplicationController
def show
artefact = content_api.artefact(params[:path])
+ finder = Finder.new(content_store.content_item("/#{params[:path].split('/')[0]}"))

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

What about leaving this in the finder method and just drop the slug argument, rather than inlining it?

@evilstreak

evilstreak Jan 30, 2015

Contributor

What about leaving this in the finder method and just drop the slug argument, rather than inlining it?

app/presenters/document_presenter.rb
def initialize(finder, document)
@finder = finder
@document = document
end
+ def respond_to?(method)
+ if finder.facets.map(&:key).include?(method.to_s)

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

Is it worth extracting this condition and the one on line 27 into a method so you can call if facet_keys.include?(method.to_s) or if has_facet?(method.to_s)

@evilstreak

evilstreak Jan 30, 2015

Contributor

Is it worth extracting this condition and the one on line 27 into a method so you can call if facet_keys.include?(method.to_s) or if has_facet?(method.to_s)

app/presenters/document_presenter.rb
+
+ def method_missing(method, *args)
+ if finder.facets.map(&:key).include?(method.to_s)
+ document.details.try(method)

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

If you've checked that the facet is included already, do you need to use try?

@evilstreak

evilstreak Jan 30, 2015

Contributor

If you've checked that the facet is included already, do you need to use try?

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

Actually, my previous comment makes no sense – try is to guard against details being nil here. Will that ever be the case?

@evilstreak

evilstreak Jan 30, 2015

Contributor

Actually, my previous comment makes no sense – try is to guard against details being nil here. Will that ever be the case?

This comment has been minimized.

@tommyp

tommyp Jan 30, 2015

Contributor

I thought try was a way to call a method if it's name is dynamic? Or am I after send?

@tommyp

tommyp Jan 30, 2015

Contributor

I thought try was a way to call a method if it's name is dynamic? Or am I after send?

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

Ah, yep. Use Object#send to dynamically call a method. Object#try is a Rails extension to make avoid having to do nil checking.

@evilstreak

evilstreak Jan 30, 2015

Contributor

Ah, yep. Use Object#send to dynamically call a method. Object#try is a Rails extension to make avoid having to do nil checking.

app/presenters/document_presenter.rb
@@ -41,7 +59,8 @@ def organisations
end
def extra_date_metadata
- {}
+ finder.facets.select{ |f| f.type == 'date' }

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

What about pushing this into Finder as a date_facets method?

@evilstreak

evilstreak Jan 30, 2015

Contributor

What about pushing this into Finder as a date_facets method?

app/presenters/document_presenter.rb
@@ -41,7 +59,8 @@ def organisations
end
def extra_date_metadata
- {}
+ finder.facets.select{ |f| f.type == 'date' }
+ .each_with_object({}) { |facet, hash| hash[facet.name] = self.try(facet.key) }

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

I think this could just be self.send(facet.key) instead of self.try(facet.key)

@evilstreak

evilstreak Jan 30, 2015

Contributor

I think this could just be self.send(facet.key) instead of self.try(facet.key)

app/presenters/document_presenter.rb
@@ -8,13 +8,31 @@ class DocumentPresenter
:bulk_published,
to: :"document.details"
+ attr_reader :finder

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

Why does this need to be public? I can't see anywhere in this PR where it's called

@evilstreak

evilstreak Jan 30, 2015

Contributor

Why does this need to be public? I can't see anywhere in this PR where it's called

app/presenters/document_presenter.rb
@@ -122,10 +141,12 @@ def first_edition?
end
def filterable_metadata
- {}
+ finder.facets.select{ |f| f.type == 'text' && f.filterable }

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

See above re pushing methods into Finder. Not sure what this one would be though. filterable_text_facets? A bit clunky. Maybe finder.text_facets.select(&:filterable?).

@evilstreak

evilstreak Jan 30, 2015

Contributor

See above re pushing methods into Finder. Not sure what this one would be though. filterable_text_facets? A bit clunky. Maybe finder.text_facets.select(&:filterable?).

app/presenters/document_presenter.rb
end
def extra_metadata
- {}
+ finder.facets.select{ |f| f.type == "text" && !f.filterable }

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

As above

@evilstreak

evilstreak Jan 30, 2015

Contributor

As above

This comment has been minimized.

@tommyp

tommyp Feb 2, 2015

Contributor

Does the (&:method) format accept ! before it?

@tommyp

tommyp Feb 2, 2015

Contributor

Does the (&:method) format accept ! before it?

This comment has been minimized.

@evilstreak

evilstreak Feb 2, 2015

Contributor

No, but you can use reject instead

@evilstreak

evilstreak Feb 2, 2015

Contributor

No, but you can use reject instead

features/document-viewing.feature
@@ -0,0 +1,14 @@
+Feature: Document viewing
+ As a specialist member of the public

This comment has been minimized.

@evilstreak

evilstreak Jan 30, 2015

Contributor

Since you're changing it, this is as good a time as any to update it. We've generally been referring to users with specialist needs rather than specialist users. So "As a member of the public with specialist needs"

@evilstreak

evilstreak Jan 30, 2015

Contributor

Since you're changing it, this is as good a time as any to update it. We've generally been referring to users with specialist needs rather than specialist users. So "As a member of the public with specialist needs"

@evilstreak

This comment has been minimized.

Show comment
Hide comment
@evilstreak

evilstreak Jan 30, 2015

Contributor

👍 This is really good work.

Contributor

evilstreak commented Jan 30, 2015

👍 This is really good work.

@tommyp tommyp changed the title from DO NOT MERGE Use Finder for metadata to Use Finder for metadata Feb 2, 2015

@tommyp

This comment has been minimized.

Show comment
Hide comment
@tommyp

tommyp Feb 2, 2015

Contributor

This is good to go now. All works and there's a Presenter to handle the special case for DSU.

Contributor

tommyp commented Feb 2, 2015

This is good to go now. All works and there's a Presenter to handle the special case for DSU.

tommyp added some commits Jan 30, 2015

Remove format specific Presenters
As we're moving to just having a Document model, we don't need all of
these Presenters. Apart from for DrugSafetyUpdate because it's a
special ❄️

DSU is unique in that they don't display any date metadata in the footer
if it's the first edition as we would then get a duplicate Published
(which is really `first_published_at`) and Published, which will change to
Updated when the document is updated. When it's not the first edition we
see Published (which is really `first_published_at`) and Updated, so
it's not a problem in that state.

For the default_date_metadata method, we return nothing if it's bulk
published or the first edition.
Remove format specific tests
Don't need all of these tests either! 🎉

Again apart from Drug Safety Update because it's a special ❄️
I've altered it's feature test to make sure that we test this
date behaviour that makes it unique too.
- Finder.new(content_store.content_item("/#{slug}"))
+ def document(finder, artefact)
+ case finder_slug
+ when "drug-safety-update"

This comment has been minimized.

@evilstreak

evilstreak Feb 3, 2015

Contributor

Can we use the document format here instead? It feels more robust to special case drug_safety_update documents than to special case the finder they belong to.

@evilstreak

evilstreak Feb 3, 2015

Contributor

Can we use the document format here instead? It feels more robust to special case drug_safety_update documents than to special case the finder they belong to.

app/models/finder.rb
+ def date_facets
+ facets.select{ |f| f.type == 'date' }
+ end
+

This comment has been minimized.

@evilstreak

evilstreak Feb 3, 2015

Contributor

Spare whitespace

@evilstreak

evilstreak Feb 3, 2015

Contributor

Spare whitespace

@evilstreak

This comment has been minimized.

Show comment
Hide comment
@evilstreak

evilstreak Feb 3, 2015

Contributor

Apart from the finder slug vs document type thing, this is good to go

Contributor

evilstreak commented Feb 3, 2015

Apart from the finder slug vs document type thing, this is good to go

tommyp added some commits Jan 30, 2015

Expose facets and format name from the Finder
We're going to use the facets to get the metadata labels so we need to
expose them and we can also specify the format name in the Finder
schema. Also expose the document_type to check for DSU.
New DocumentPresenter
This new DocumentPresenter is now the Presenter for all document
formats.

The major change is now we pass the Finder to the class to get the
format name and the facets of the Finder, which now describe all the
Metadata that a document format may have. We then use the keys of these
facets to define methods which call `document.details.#{method}`.

We also use these facets to get a hash of the `extra_date_metadata`,
`filterable_metadata` and `extra_metadata`.
@tommyp

This comment has been minimized.

Show comment
Hide comment
@tommyp

tommyp Feb 3, 2015

Contributor

Fixed those commits based on @evilstreak's comments.

Contributor

tommyp commented Feb 3, 2015

Fixed those commits based on @evilstreak's comments.

tommyp added some commits Jan 30, 2015

Refactor SpecialistDocumentsController
First, we get the Finder content_item from the first part of the path.
We then pass this into the DocumentPresenter to use. This allows us to
get rid of all the code checking the slug and initialising a format
Presenter class, apart from DSU (again).
Add new Document Viewing feature
After we deleted all our features, adding in a generic one for a Document
Format made sense. This test uses a Document which contains the 3
current types of Metdata; linkable text, unlinkable text and a date.

Also updating the Drug Safety Update feature to use the Finder
correctly.

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

@evilstreak evilstreak merged commit f5930a4 into master Feb 3, 2015

1 check passed

default Build #274 succeeded on Jenkins
Details

@evilstreak evilstreak deleted the use-finder-for-metadata branch Feb 3, 2015

@evilstreak evilstreak referenced this pull request in alphagov/finder-frontend Feb 3, 2015

Merged

Use facets for metadata #148

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.