Merge pull request #809 from projectblacklight/track_click

Change the click tracking route to be CatalogController#track.

Author: Jessie Keck

app/assets/javascripts/blacklight/search_context.js

@@ -1,28 +1,42 @@
 //= require blacklight/core
 (function($) {
   Blacklight.do_search_context_behavior = function() {
-      $('a[data-counter]').click(function(event) {
-      var f = document.createElement('form'); f.style.display = 'none'; 
-      this.parentNode.appendChild(f); 
-      f.method = 'POST'; 
-      f.action = $(this).attr('href');
-      if(event.metaKey || event.ctrlKey){f.target = '_blank';};
-      var d = document.createElement('input'); d.setAttribute('type', 'hidden'); 
-      d.setAttribute('name', 'counter'); d.setAttribute('value', $(this).data('counter')); f.appendChild(d);
-      var id = document.createElement('input'); id.setAttribute('type', 'hidden');
-      id.setAttribute('name', 'search_id'); id.setAttribute('value', $(this).data('search_id')); f.appendChild(id);
-      var m = document.createElement('input'); m.setAttribute('type', 'hidden'); 
-      m.setAttribute('name', '_method'); m.setAttribute('value', 'put'); f.appendChild(m);
-      var m = document.createElement('input'); m.setAttribute('type', 'hidden'); 
-      m.setAttribute('name', $('meta[name="csrf-param"]').attr('content')); m.setAttribute('value', $('meta[name="csrf-token"]').attr('content')); f.appendChild(m);
-
-      f.submit();
-        
-      return false;
-      });
-
-    };  
-Blacklight.onLoad(function() {
-  Blacklight.do_search_context_behavior();  
-});
+    $('a[data-context-href]').on('click.search-context', Blacklight.handleSearchContextMethod);
+  };
+
+  // this is the $.rails.handleMethod with a couple adjustments, described inline:
+  // first, we're attaching this directly to the event handler, so we can check for meta-keys
+  Blacklight.handleSearchContextMethod = function(event) {
+    var link = $(this);
+
+    // instead of using the normal href, we need to use the context href instead
+    var href = link.data('context-href'),
+      method = 'post',
+      target = link.attr('target'),
+      csrfToken = $('meta[name=csrf-token]').attr('content'),
+      csrfParam = $('meta[name=csrf-param]').attr('content'),
+      form = $('<form method="post" action="' + href + '"></form>'),
+      metadataInput = '<input name="_method" value="' + method + '" type="hidden" />',
+      redirectHref = '<input name="redirect" value="' + link.attr('href') + '" type="hidden" />';
+
+    // check for meta keys.. if set, we should open in a new tab
+    if(event.metaKey || event.ctrlKey) {
+      target = '_blank';
+    }
+
+    if (csrfParam !== undefined && csrfToken !== undefined) {
+      metadataInput += '<input name="' + csrfParam + '" value="' + csrfToken + '" type="hidden" />';
+    }
+
+    if (target) { form.attr('target', target); }
+
+    form.hide().append(metadataInput).append(redirectHref).appendTo('body');
+    form.submit();
+
+    return false;
+  };
+
+  Blacklight.onLoad(function() {
+    Blacklight.do_search_context_behavior();
+  });
 })(jQuery);

app/helpers/blacklight/url_helper_behavior.rb

@@ -1,11 +1,19 @@
+require 'deprecation'
 module Blacklight::UrlHelperBehavior
+  extend Deprecation
+  self.deprecation_horizon = 'blacklight 6.0'
 
   ##
   # Extension point for downstream applications
-  # to provide more interesting routing to 
+  # to provide more interesting routing to
   # documents
   def url_for_document doc
-    doc
+    if controller.is_a? Blacklight::Catalog and doc.is_a? SolrDocument and
+        (!doc.respond_to?(:to_model) or doc.to_model.is_a? SolrDocument)
+      { controller: controller_name, action: :show, id: doc }
+    else
+      doc
+    end
   end
 
   # link_to_document(doc, :label=>'VIEW', :counter => 3)
@@ -15,30 +23,55 @@ def url_for_document doc
   def link_to_document(doc, opts={:label=>nil, :counter => nil})
     opts[:label] ||= document_show_link_field(doc)
     label = render_document_index_label doc, opts
-    link_to label, url_for_document(doc), search_session_params(opts[:counter]).merge(opts.reject { |k,v| [:label, :counter].include? k  })
+    link_to label, url_for_document(doc), document_link_params(doc, opts)
   end
 
+  def document_link_params(doc, opts)
+    session_tracking_params(doc, opts[:counter]).merge(opts.reject { |k,v| [:label, :counter].include? k  })
+  end
+  protected :document_link_params
+
   ##
   # Link to the previous document in the current search context
   def link_to_previous_document(previous_document)
-    link_to_unless previous_document.nil?, raw(t('views.pagination.previous')), url_for_document(previous_document), search_session_params(search_session['counter'].to_i - 1).merge(:class => "previous", :rel => 'prev')  do
+    link_opts = session_tracking_params(previous_document, search_session['counter'].to_i - 1).merge(:class => "previous", :rel => 'prev')
+    link_to_unless previous_document.nil?, raw(t('views.pagination.previous')), url_for_document(previous_document), link_opts do
       content_tag :span, raw(t('views.pagination.previous')), :class => 'previous'
     end
   end
 
   ##
   # Link to the next document in the current search context
   def link_to_next_document(next_document)
-    link_to_unless next_document.nil?, raw(t('views.pagination.next')), url_for_document(next_document), search_session_params(search_session['counter'].to_i + 1).merge(:class => "next", :rel => 'next') do
+    link_opts = session_tracking_params(next_document, search_session['counter'].to_i + 1).merge(:class => "next", :rel => 'next')
+    link_to_unless next_document.nil?, raw(t('views.pagination.next')), url_for_document(next_document), link_opts do
       content_tag :span, raw(t('views.pagination.next')), :class => 'next'
     end
   end
 
   ##
   # Current search context parameters
-  def search_session_params counter
+  def search_session_params counter 
     { :'data-counter' => counter, :'data-search_id' => current_search_session.try(:id) }
   end
+  deprecation_deprecate :search_session_params
+
+  ##
+  # Attributes for a link that gives a URL we can use to track clicks for the current search session
+  # @param [SolrDocument] document
+  # @param [Integer] counter
+  # @example
+  #   session_tracking_params(SolrDocument.new(id: 123), 7)
+  #   => { data: { :'tracker-href' => '/catalog/123/track?counter=7&search_id=999' } }
+  def session_tracking_params document, counter
+    if document.nil?
+      return {}
+    end
+  
+    { :data => {:'context-href' => track_solr_document_path(document, counter: counter, search_id: current_search_session.try(:id))}}
+  end
+  protected :session_tracking_params
+  
 
   #
   # link based helpers ->

lib/blacklight/catalog.rb

@@ -57,11 +57,16 @@ def show
     end
 
     # updates the search counter (allows the show view to paginate)
-    def update
+    def track
       search_session['counter'] = params[:counter]
-      redirect_to :action => "show", :status => 303
+      path = if params[:redirect] and (params[:redirect].starts_with?("/") or params[:redirect] =~ URI::regexp)
+        URI.parse(params[:redirect]).path
+      else
+        { action: 'show' }
+      end
+      redirect_to path, :status => 303
     end
-    
+
     # displays values and pagination links for a single facet field
     def facet
       @facet = blacklight_config.facet_fields[params[:id]]

lib/blacklight/catalog/search_context.rb

@@ -94,7 +94,7 @@ def blacklisted_search_session_params
   def setup_next_and_previous_documents
     if search_session['counter'] and current_search_session
       index = search_session['counter'].to_i - 1
-      response, documents = get_previous_and_next_documents_for_search index, current_search_session.query_params
+      response, documents = get_previous_and_next_documents_for_search index, current_search_session.query_params.with_indifferent_access
 
       search_session['total'] = response.total
       @search_context_response = response

lib/blacklight/routes.rb

@@ -108,10 +108,14 @@ def export(primary_resource)
       def solr_document(primary_resource)
         add_routes do |options|
 
-          args = {only: [:show, :update]}
+          args = {only: [:show]}
           args[:constraints] = options[:constraints] if options[:constraints]
 
-          resources :solr_document, args.merge(path: primary_resource, controller: primary_resource)
+          resources :solr_document, args.merge(path: primary_resource, controller: primary_resource) do
+            member do
+              post "track"
+            end
+          end
 
           # :show and :update are for backwards-compatibility with catalog_url named routes
           resources primary_resource, args

spec/controllers/catalog_controller_spec.rb

@@ -195,23 +195,33 @@ def assigns_response
 
   end # describe index action
 
-  describe "update action" do
+  describe "track action" do
     doc_id = '2007020969'
 
     it "should set counter value into session[:search]" do
-      put :update, :id => doc_id, :counter => 3
+      put :track, :id => doc_id, :counter => 3
       expect(session[:search]['counter']).to eq "3"
     end
 
     it "should redirect to show action for doc id" do
-      put :update, :id => doc_id, :counter => 3
+      put :track, :id => doc_id, :counter => 3
       assert_redirected_to(catalog_path(doc_id))
     end
 
     it "HTTP status code for redirect should be 303" do
-      put :update, :id => doc_id, :counter => 3
+      put :track, :id => doc_id, :counter => 3
       expect(response.status).to eq 303
     end
+
+    it "should redirect to the path given in the redirect param" do
+      put :track, :id => doc_id, :counter => 3, redirect: '/xyz'
+      assert_redirected_to("/xyz")
+    end
+
+    it "should redirect to the path of the uri given in the redirect param" do
+      put :track, :id => doc_id, :counter => 3, redirect: 'http://localhost:3000/xyz'
+      assert_redirected_to("/xyz")
+    end
   end
 
   # SHOW ACTION

spec/features/alternate_controller_spec.rb

@@ -29,7 +29,7 @@
     fill_in "q", :with=>"history"
     click_button 'search'
     expect(page).to have_selector ".document-thumbnail"
-    expect(page).to have_selector ".document-thumbnail a[data-counter]"
+    expect(page).to have_selector ".document-thumbnail a[data-context-href]"
     expect(page).to have_selector ".document-thumbnail a img"
 
   end

spec/features/search_results_spec.rb

@@ -55,14 +55,19 @@
     click_on 'Pluvial nectar of blessings'
     expect(page).to have_content "« Previous | 10 of 30 | Next »"
     prev = page.find("#previousNextDocument .previous")
-    expect(prev['data-counter']).to eq "9"
-    expect(prev['data-search_id']).to eq search_id
+    expect(prev['data-context-href']).to eq "/catalog/2003546302/track?counter=9&search_id=#{search_id}"
 
     click_on "« Previous"
 
     prev = page.find("#previousNextDocument .previous")
-    expect(prev['data-counter']).to eq "8"
-    expect(prev['data-search_id']).to eq search_id
+    expect(prev['data-context-href']).to eq "/catalog/2004310986/track?counter=8&search_id=#{search_id}"
+  end
+  
+  it "should redirect context urls to the original url", :js => true do
+    search_for ''
+    first('.index_title a').click
+    expect(page).to have_content "« Previous | 1 of 30 | Next »"
+    expect(page.current_url).to_not have_content "/track"
   end
 
 end

spec/helpers/url_helper_spec.rb

@@ -18,6 +18,41 @@
     helper.stub(current_search_session: nil)
   end
 
+  describe "url_for_document" do
+    let(:controller_class) { ::CatalogController.new }
+
+    before do
+      helper.stub(controller: controller_class)
+      helper.stub(controller_name: controller_class.controller_name)
+    end
+
+    it "should be a polymorphic routing-ready object" do
+      doc = double
+      expect(helper.url_for_document(doc)).to eq doc
+    end
+
+    it "should be a catalog controller-specific route" do
+      doc = SolrDocument.new
+      expect(helper.url_for_document(doc)).to eq({controller: 'catalog', action: :show, id: doc})
+    end
+
+    context "within an alternative catalog controller" do
+      let(:controller_class) { ::AlternateController.new }
+
+      it "should be a catalog controller-specific route" do
+        doc = SolrDocument.new
+        expect(helper.url_for_document(doc)).to eq({controller: 'alternate', action: :show, id: doc})
+      end
+    end
+
+    it "should be a polymorphic route if the solr document responds to #to_model with a non-SolrDocument" do
+      some_model = double
+      doc = SolrDocument.new
+      doc.stub(to_model: some_model)
+      expect(helper.url_for_document(doc)).to eq doc
+    end
+  end
+
   describe "link_back_to_catalog" do
     let(:query_params)  {{:q => "query", :f => "facets", :per_page => "10", :page => "2", :controller=>'catalog'}}
     let(:bookmarks_query_params) {{ :page => "2", :controller=>'bookmarks'}}
@@ -210,7 +245,7 @@
     it "should convert the counter parameter into a data- attribute" do
       data = {'id'=>'123456','title_display'=>['654321']}
       @document = SolrDocument.new(data)
-      expect(helper.link_to_document(@document, { :label => :title_display, :counter => 5  })).to match /data-counter="5"/
+      expect(helper.link_to_document(@document, { :label => :title_display, :counter => 5  })).to match /\/catalog\/123456\/track\?counter=5/
     end
 
     it "passes on the title attribute to the link_to_with_data method" do
@@ -352,4 +387,4 @@
       expect(added_facet_params_from_facet_action).to eq added_facet_params.except(Blacklight::Solr::FacetPaginator.request_keys[:page], Blacklight::Solr::FacetPaginator.request_keys[:sort])     
     end
   end
-end
+end

spec/lib/blacklight/routes_spec.rb

@@ -7,17 +7,17 @@
     describe "without constraints" do
       let(:options) { Hash.new }
       it "should define the resources" do
-        router.should_receive(:resources).with(:solr_document, {:path=>:records, :controller=>:records, :only=>[:show, :update]})
-        router.should_receive(:resources).with(:records, :only=>[:show, :update])
+        router.should_receive(:resources).with(:solr_document, {:path=>:records, :controller=>:records, :only=>[:show]})
+        router.should_receive(:resources).with(:records, :only=>[:show])
         subject.solr_document(:records)
       end
     end
 
     describe "with constraints" do
       let(:options) { { :constraints => {id: /[a-z]+/, format: false } } }
       it "should define the resources" do
-        router.should_receive(:resources).with(:solr_document, {:path=>:records, :controller=>:records, :only=>[:show, :update], :constraints=>{:id=>/[a-z]+/, :format=>false} })
-        router.should_receive(:resources).with(:records, :only=>[:show, :update], :constraints=>{:id=>/[a-z]+/, :format=>false})
+        router.should_receive(:resources).with(:solr_document, {:path=>:records, :controller=>:records, :only=>[:show], :constraints=>{:id=>/[a-z]+/, :format=>false} })
+        router.should_receive(:resources).with(:records, :only=>[:show], :constraints=>{:id=>/[a-z]+/, :format=>false})
         subject.solr_document(:records)
       end
     end