Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Refactor to reduce search-related duplication

- Expose ActiveModel callbacks for document publishing/archiving
- Trigger document search indexing with callbacks
- Extract mixin for defining search_index methods and hooking
  indexing/unindexing into record lifecycle
  • Loading branch information...
commit fb64eddf7e696d8246dcc9e05352046bc97f24a4 2 parents 0b26a39 + eeac4df
@tomstuart tomstuart authored
View
2  Gemfile.lock
@@ -242,7 +242,7 @@ GEM
thor (0.14.6)
tilt (1.3.3)
timecop (0.3.5)
- transitions (0.0.10)
+ transitions (0.0.13)
treetop (1.4.10)
polyglot
polyglot (>= 0.3.1)
View
11 app/models/document.rb
@@ -7,6 +7,10 @@ class Document < ActiveRecord::Base
include Document::Organisations
include Document::Publishing
+ include Rails.application.routes.url_helpers
+ include PublicDocumentRoutesHelper
+ include Searchable
+
has_many :editorial_remarks, dependent: :destroy
has_many :document_authors, dependent: :destroy
@@ -34,6 +38,9 @@ def significant_changed_attributes
changed - %w(state updated_at featured carrierwave_featuring_image)
end
+ searchable title: :title, link: -> d { d.public_document_path(d) }, content: :body_without_markup, format: -> d { d.type.underscore },
+ only: :published, index_after: :publish, unindex_after: :archive
+
def creator
document_authors.first && document_authors.first.user
end
@@ -128,6 +135,10 @@ def sluggable_title
title
end
+ def body_without_markup
+ Govspeak::Document.new(body).to_text
+ end
+
class << self
def authored_by(user)
joins(:document_authors).where(document_authors: {user_id: user}).group(:document_id)
View
41 app/models/document/workflow.rb
@@ -4,8 +4,6 @@ module Document::Workflow
included do
include ::Transitions
include ActiveRecord::Transitions
- include Rails.application.routes.url_helpers
- include PublicDocumentRoutesHelper
default_scope where(%{documents.state <> "deleted"})
scope :draft, where(state: "draft")
@@ -13,6 +11,9 @@ module Document::Workflow
scope :rejected, where(state: "rejected")
scope :published, where(state: "published")
+ define_model_callbacks :publish, :archive, only: :after
+ after_publish :archive_previous_documents
+
state_machine do
state :draft
state :submitted
@@ -33,11 +34,11 @@ module Document::Workflow
transitions from: :submitted, to: :rejected
end
- event :publish, success: :on_publish_success do
+ event :publish, success: -> document { document.run_callbacks(:publish) } do
transitions from: [:draft, :submitted], to: :published
end
- event :archive, success: :on_archive_success do
+ event :archive, success: -> document { document.run_callbacks(:archive) } do
transitions from: :published, to: :archived
end
end
@@ -46,44 +47,12 @@ module Document::Workflow
validates_with DocumentHasNoOtherPublishedDocumentsValidator, on: :create
end
- def on_publish_success
- archive_previous_documents
- update_in_search_index
- end
-
- def on_archive_success
- remove_from_search_index
- end
-
def archive_previous_documents
document_identity.documents.published.each do |document|
document.archive! unless document == self
end
end
- def update_in_search_index
- Rummageable.index(search_index)
- end
-
- def remove_from_search_index
- Rummageable.delete(public_document_path(self))
- end
-
- def search_index
- { "title" => title, "link" => public_document_path(self),
- "indexable_content" => body_without_markup, "format" => type.underscore }
- end
-
- def body_without_markup
- Govspeak::Document.new(body).to_text
- end
-
- module ClassMethods
- def search_index_published
- published.map(&:search_index)
- end
- end
-
class DocumentHasNoUnpublishedDocumentsValidator < ActiveModel::Validator
def validate(record)
if record.document_identity && (existing_edition = record.document_identity.unpublished_edition)
View
24 app/models/ministerial_role.rb
@@ -1,12 +1,12 @@
class MinisterialRole < Role
+ include Searchable
include Rails.application.routes.url_helpers
has_many :document_ministerial_roles
has_many :documents, through: :document_ministerial_roles
has_many :speeches, through: :current_role_appointments
- after_save :update_in_search_index
- after_destroy :remove_from_search_index
+ searchable title: :to_s, link: :search_link, content: :current_person_biography, format: 'minister'
def self.cabinet
name = arel_table[:name]
@@ -24,26 +24,10 @@ def destroyable?
super && documents.empty?
end
- def search_index
+ def search_link
# This should be ministerial_role_path(self), but we can't use that because friendly_id's #to_param returns
# the old value of the slug (e.g. nil for a new record) if the record is dirty, and apparently the record
# is still marked as dirty during after_save callbacks.
- link = ministerial_role_path(slug)
-
- { 'title' => to_s, 'link' => link, 'indexable_content' => current_person_biography, 'format' => 'minister' }
- end
-
- def self.search_index
- all.map(&:search_index)
- end
-
- private
-
- def update_in_search_index
- Rummageable.index(search_index)
- end
-
- def remove_from_search_index
- Rummageable.delete(ministerial_role_path(self))
+ ministerial_role_path(slug)
end
end
View
26 app/models/organisation.rb
@@ -1,4 +1,5 @@
class Organisation < ActiveRecord::Base
+ include Searchable
include Rails.application.routes.url_helpers
belongs_to :organisation_type
@@ -34,12 +35,11 @@ class Organisation < ActiveRecord::Base
default_scope order(:name)
+ searchable title: :name, link: :search_link, content: :description, format: 'organisation'
+
extend FriendlyId
friendly_id :name, use: :slugged
- after_save :update_in_search_index
- after_destroy :remove_from_search_index
-
def should_generate_new_friendly_id?
new_record?
end
@@ -65,26 +65,10 @@ def normalize_friendly_id(value)
super value
end
- def search_index
+ def search_link
# This should be organisation_path(self), but we can't use that because friendly_id's #to_param returns
# the old value of the slug (e.g. nil for a new record) if the record is dirty, and apparently the record
# is still marked as dirty during after_save callbacks.
- link = organisation_path(slug)
-
- { 'title' => name, 'link' => link, 'indexable_content' => description, 'format' => 'organisation' }
- end
-
- private
-
- def update_in_search_index
- Rummageable.index(search_index)
- end
-
- def remove_from_search_index
- Rummageable.delete(organisation_path(self))
- end
-
- def self.search_index
- all.map(&:search_index)
+ organisation_path(slug)
end
end
View
63 app/models/searchable.rb
@@ -0,0 +1,63 @@
+module Searchable
+ extend ActiveSupport::Concern
+
+ included do
+ class_attribute :searchable_options
+ end
+
+ module ClassMethods
+ def searchable(options)
+ include Searchable::Mixin
+
+ self.searchable_options = options.reverse_merge(index_after: :save, unindex_after: :destroy, only: :scoped)
+
+ [:title, :link, :content, :format, :only].each do |name|
+ value = searchable_options[name]
+ searchable_options[name] =
+ if value.respond_to?(:call)
+ # use procs verbatim
+ value
+ elsif value.respond_to?(:to_proc)
+ # turn willing objects (e.g. symbols) into procs
+ value.to_proc
+ else
+ # treat other objects (e.g. strings) as constants
+ -> _ { value }
+ end
+ end
+
+ set_callback searchable_options[:index_after], :after, :update_in_search_index
+ set_callback searchable_options[:unindex_after], :after, :remove_from_search_index
+ end
+ end
+
+ module Mixin
+ extend ActiveSupport::Concern
+
+ def search_index
+ title, link, content, format =
+ [:title, :link, :content, :format].map { |name| searchable_options[name].call(self) }
+
+ {
+ 'title' => title,
+ 'link' => link,
+ 'indexable_content' => content,
+ 'format' => format
+ }
+ end
+
+ def update_in_search_index
+ Rummageable.index(search_index)
+ end
+
+ def remove_from_search_index
+ Rummageable.delete(searchable_options[:link].call(self))
+ end
+
+ module ClassMethods
+ def search_index
+ searchable_options[:only].call(self).all.map(&:search_index)
+ end
+ end
+ end
+end
View
2  lib/whitehall.rb
@@ -41,7 +41,7 @@ def use_s3?
end
def search_index
- [Document.search_index_published, Organisation.search_index, MinisterialRole.search_index].sum([])
+ [Document, MinisterialRole, Organisation].map(&:search_index).sum([])
end
private
View
2  test/integration/search_index_test.rb
@@ -2,7 +2,7 @@
class SearchIndexTest < ActiveSupport::TestCase
test "Whitehall.search_index includes documents" do
- Document.stubs(:search_index_published).returns([:documents])
+ Document.stubs(:search_index).returns([:documents])
assert Whitehall.search_index.include?(:documents)
end
View
3  test/unit/document_test.rb
@@ -713,8 +713,9 @@ class DocumentTest < ActiveSupport::TestCase
test "should return search index data for all published documents" do
create(:published_policy, title: "policy-title", body: "this and that")
create(:published_publication, title: "publication-title", body: "stuff and things")
+ create(:draft_publication, title: "draft-publication-title", body: "bits and bobs")
- results = Document.search_index_published
+ results = Document.search_index
assert_equal 2, results.length
assert_equal({"title"=>"policy-title", "link"=>"/government/policies/policy-title",
Please sign in to comment.
Something went wrong with that request. Please try again.