Finder as presenter #125

Merged
merged 7 commits into from Nov 21, 2014

Conversation

Projects
None yet
3 participants
@tommyp
Contributor

tommyp commented Nov 18, 2014

As part of the work to reduce Tech Debt in Finders, I decided that I should refactor how the Finder is displayed by sending the schema to the Content Store as part of the details hash and introducing a Presenter to simplify parsing the response. This allowed me to get rid of the Finder model (as that was basically all it was doing) along with it's horrible Finder#get method and a bunch of Parsers which we don't need now.

With this, I also refactored the Facets as we no longer need to parse hashes and build them into OpenStructs. Doing this, I discovered that there was lots of code still around that we don't need. Mostly from before we built the custom UI components so that was removed, along with the Radio Facet which we don't use anymore.

All this work has the added benefit of being able to remove the dependancy on Finder API for this project.

This is a big refactor, so it's best to leave merging this a couple of days so we can get everyone's opinions on this.

@kalleth

View changes

app/controllers/finders_controller.rb
- @finder ||= Finder.get(finder_slug).tap { |finder|
+ @finder ||= FinderPresenter.new(
+ content_store.content_item("/#{finder_slug}")
+ ).tap { |finder|

This comment has been minimized.

@kalleth

kalleth Nov 19, 2014

Contributor

Could we pass the facet params and keywords in as arguments or keywords to the FinderPresenter here and simplifiy the controller to remove the tap?

@kalleth

kalleth Nov 19, 2014

Contributor

Could we pass the facet params and keywords in as arguments or keywords to the FinderPresenter here and simplifiy the controller to remove the tap?

@kalleth

View changes

features/support/case_helper.rb
- "name": "Competition and Markets Authority cases",
- "document_noun": "case",
- "facets": [
+ def cma_cases_content_item

This comment has been minimized.

@kalleth

kalleth Nov 19, 2014

Contributor

This is used here, a subset is used in the email_alert_subscriptions_controllers_spec (below) and could be used to build the OpenStruct used in facet_parser_spec.rb.

I still think we'd be better off with this in a fixture somewhere then altering the other tests; however this isn't something that will stop me merging this PR.

@kalleth

kalleth Nov 19, 2014

Contributor

This is used here, a subset is used in the email_alert_subscriptions_controllers_spec (below) and could be used to build the OpenStruct used in facet_parser_spec.rb.

I still think we'd be better off with this in a fixture somewhere then altering the other tests; however this isn't something that will stop me merging this PR.

@kalleth

This comment has been minimized.

Show comment
Hide comment
@kalleth

kalleth Nov 19, 2014

Contributor

I really like this PR. I think it lowers the complexity of finder-frontend significantly and will make it far nicer to work with for the publishing team after handover.

Great work, @tommyp :)

Contributor

kalleth commented Nov 19, 2014

I really like this PR. I think it lowers the complexity of finder-frontend significantly and will make it far nicer to work with for the publishing team after handover.

Great work, @tommyp :)

tommyp added some commits Nov 18, 2014

Refactor Finder to be a Presenter
As we move away from using Finder API, we want to store all the
information for a Finder inlcuding the schema in the Content Store.

This commit is the start of the work that will allow this to happen.
By moving the presentation logic of a Finder from the model to a
Presenter, we follow a more standard Rails pattern. This also allows to
get rid of the horrible `Finder#get` method which fetches the schema
from Finder API.
Refactor facets to use ContentStore response
Now that Finder Schemas are inside the details hash in the response
returned from the ContentStore, we can refactor the facets used and the
Parser.

This also means we can remove the OpenStruct option generation from the
parsers, since all the options are OpenStructs.

While doing this work, I discovered a lot of leftover code that isn't
used anymore such as the `SelectFacet#include_blank` and the methods
related to building native Select boxes so I removed it also.

This commit also contains the updates to the tests.
Remove FacetCollectionParser and Spec
As this is now an OpenStruct, we don't need a seperate class to return
us Facet objects. This logic now happens in the Presenter and talks
directly to the FacetCollection.
Remove RadioFacet and Spec
After Finders were developed we found out through User Research that
Radio Facets were confusing for searching, so we phased them out and
used smaller select facets.

This commit removes the radio facet and the spec.
Remove Finder API from FinderFrontend
This commit removes the call to the Finder API for the schema and
removes the schema beind passed around in Finder Frontend, as it's no
longer used.
Replace Finder API in tests
This commit removes the stubbed response from Finder API and instead
adds the schema to the stubbed ContentStore response.
@tommyp

This comment has been minimized.

Show comment
Hide comment
@tommyp

tommyp Nov 20, 2014

Contributor

This needs alphagov/finder-api#89 merged first.

Contributor

tommyp commented Nov 20, 2014

This needs alphagov/finder-api#89 merged first.

@tommyp

This comment has been minimized.

Show comment
Hide comment
@tommyp

tommyp Nov 20, 2014

Contributor

Fixed the code based on @kalleth's suggestions.

Contributor

tommyp commented Nov 20, 2014

Fixed the code based on @kalleth's suggestions.

@kalleth

View changes

spec/controllers/email_alert_subscriptions_controller_spec.rb
allow(controller).to receive(:email_alert_api).and_return(delivery_api)
- allow(Finder).to receive(:get).with('cma-cases').and_return(finder)
+ # allow(FinderPresenter).to receive(:new).with({}).and_return(finder)

This comment has been minimized.

@kalleth

kalleth Nov 20, 2014

Contributor

Should be removed rather than commented out :)

@kalleth

kalleth Nov 20, 2014

Contributor

Should be removed rather than commented out :)

This comment has been minimized.

@tommyp

tommyp Nov 20, 2014

Contributor

Woops. Meant to delete that.

@tommyp

tommyp Nov 20, 2014

Contributor

Woops. Meant to delete that.

- ]
- }
- |
+ def fixtures_path

This comment has been minimized.

@kalleth

kalleth Nov 20, 2014

Contributor

Could this use the FixturesHelper now that's been introduced?

@kalleth

kalleth Nov 20, 2014

Contributor

Could this use the FixturesHelper now that's been introduced?

Use FinderPresenter in Email Subs Controller
As we now use a Presenter, we can call this instead in the email alert
subscriptions controller. This commit also updates the test and includes
webmock to stub out the API in RSpec.
@kalleth

This comment has been minimized.

Show comment
Hide comment
@kalleth

kalleth Nov 20, 2014

Contributor

I'm happy for this to be merged now -- all my comments above have either been addressed or discussed offline. Great work Tommy 👍

Contributor

kalleth commented Nov 20, 2014

I'm happy for this to be merged now -- all my comments above have either been addressed or discussed offline. Great work Tommy 👍

@Themitchell

This comment has been minimized.

Show comment
Hide comment
@Themitchell

Themitchell Nov 20, 2014

Contributor

Good work. This is much cleaner now! 👍

Contributor

Themitchell commented Nov 20, 2014

Good work. This is much cleaner now! 👍

kalleth added a commit that referenced this pull request Nov 21, 2014

@kalleth kalleth merged commit c18a8d9 into master Nov 21, 2014

1 check passed

default Build #488 succeeded on Jenkins
Details

@kalleth kalleth deleted the finder-as-presenter branch Nov 21, 2014

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