first pass at changing JS html insertion into helper instead

asanghi · Jan 26, 2011 · a5163c7 · a5163c7
1 parent 6790b36
commit a5163c7
Show file tree

Hide file tree

Showing 8 changed files with 65 additions and 111 deletions.
diff --git a/README.md b/README.md
@@ -398,15 +398,25 @@ not working properly.
 Javascript Notifer
 ------------------
 
-To automatically include the Javascript node on every page, set the
-:js_notifier to true:
+To automatically include the Javascript node on every page, use this helper method from your layouts:
 
-  HoptoadNotifier.configure do |config|
-    config.js_notifier = true
-  end
+    <%= hoptoad_javascript_notifier %>
 
-It automatically uses the API key, host, and port specified in the
-configuration.
+It's important to insert this very high in the markup, above all other javascript.  Example:
+
+    <!DOCTYPE html>
+    <html>
+      <head>
+        <meta charset="utf8">
+        <%= hoptoad_javascript_notifier %>
+        <!-- more javascript -->
+      </head>
+      <body>
+        ...
+      </body>
+    </html>
+
+This helper will automatically use the API key, host, and port specified in the configuration.
 
 Credits
 -------

diff --git a/features/rails_with_js_notifier.feature b/features/rails_with_js_notifier.feature
@@ -10,11 +10,10 @@ Feature: Install the Gem in a Rails application and enable the JavaScript notifi
     When I configure the notifier to use the following configuration lines:
       """
       config.api_key     = "myapikey"
-      config.js_notifier = true
       """
     And I define a response for "TestController#index":
       """
-        render :text => '<html><head profile="http://example.com"></head><body></body></html>'
+        render :inline => '<html><head profile="http://example.com"><%= hoptoad_javascript_notifier %></head><body></body></html>'
       """
     And I route "/test/index" to "test#index"
     And I perform a request to "http://example.com:123/test/index"
@@ -24,10 +23,6 @@ Feature: Install the Gem in a Rails application and enable the JavaScript notifi
     And the notifier JavaScript should provide the following errorDefaults:
       | url                           | component | action |
       | http://example.com:123/test/index | test      | index  |
-    And I should see the following value as the html head:
-      """
-        <head profile="http://example.com">
-      """
 
   Scenario: Include the Javascript notifier when enabled using custom configuration settings
     When I generate a new Rails application
@@ -38,23 +33,18 @@ Feature: Install the Gem in a Rails application and enable the JavaScript notifi
       config.api_key     = "myapikey!"
       config.host        = "myhoptoad.com"
       config.port        = 3001
-      config.js_notifier = true
       """
     And I define a response for "TestController#index":
       """
-        render :text => "<html><head></head><body></body></html>"
+        render :inline => '<html><head><%= hoptoad_javascript_notifier %></head><body></body></html>'
       """
     And I route "/test/index" to "test#index"
     And I perform a request to "http://example.com:123/test/index"
     Then I should see the notifier JavaScript for the following:
       | api_key   | environment | host               |
       | myapikey! | production  | myhoptoad.com:3001 |
-      And I should see the following value as the html head:
-        """
-          <head>
-        """
 
-  Scenario: Don't include the Javascript notifier by default
+  Scenario: Dont include the Javascript notifier by default
     When I generate a new Rails application
     And I configure the Hoptoad shim
     And I configure my application to require the "hoptoad_notifier" gem
@@ -64,25 +54,24 @@ Feature: Install the Gem in a Rails application and enable the JavaScript notifi
       """
     And I define a response for "TestController#index":
       """
-        render :text => "<html><head></head><body></body></html>"
+        render :inline => "<html><head></head><body></body></html>"
       """
     And I route "/test/index" to "test#index"
     And I perform a request to "http://example.com:123/test/index"
     Then I should not see notifier JavaScript
 
-  Scenario: Don't include the Javascript notifier when enabled in non-public environments
+  Scenario: Dont include the Javascript notifier when enabled in non-public environments
     When I generate a new Rails application
     And I configure the Hoptoad shim
     And I configure my application to require the "hoptoad_notifier" gem
     When I configure the notifier to use the following configuration lines:
       """
       config.api_key          = "myapikey!"
-      config.js_notifier      = true
       config.environment_name = 'test'
       """
     And I define a response for "TestController#index":
       """
-        render :text => "<html><head></head><body></body></html>"
+        render :inline => '<html><head><%= hoptoad_javascript_notifier %></head><body></body></html>'
       """
     And I route "/test/index" to "test#index"
     And I perform a request to "http://example.com:123/test/index" in the "test" environment

diff --git a/features/step_definitions/rails_application_steps.rb b/features/step_definitions/rails_application_steps.rb
@@ -362,11 +362,6 @@ def rails_non_initializer_hoptoad_config_file
   end
 end
 
-Given /^I should see the following value as the html head:$/ do |value|
-  document_body = '<html>' + @terminal.output.split('<html>').last
-  document_body.should include(value.strip)
-end
-
 Then "the notifier JavaScript should provide the following errorDefaults:" do |table|
   hash = table.hashes.first
 

diff --git a/lib/hoptoad_notifier/configuration.rb b/lib/hoptoad_notifier/configuration.rb
@@ -8,7 +8,7 @@ class Configuration
         :ignore_user_agent, :notifier_name, :notifier_url, :notifier_version,
         :params_filters, :project_root, :port, :protocol, :proxy_host,
         :proxy_pass, :proxy_port, :proxy_user, :secure, :framework,
-        :js_notifier, :user_information].freeze
+        :user_information].freeze
 
     # The API key for your project, found on the project edit form.
     attr_accessor :api_key
@@ -63,9 +63,6 @@ class Configuration
     # +true+ if you want to check for production errors matching development errors, +false+ otherwise.
     attr_accessor :development_lookup
 
-    # +true+ if you want to enable the JavaScript notifier in production environments
-    attr_accessor :js_notifier
-
     # The name of the environment the application is running in
     attr_accessor :environment_name
 
@@ -132,7 +129,6 @@ def initialize
       @ignore_user_agent        = []
       @development_environments = %w(development test cucumber)
       @development_lookup       = true
-      @js_notifier              = false
       @notifier_name            = 'Hoptoad Notifier'
       @notifier_version         = VERSION
       @notifier_url             = 'http://hoptoadapp.com'
@@ -222,6 +218,10 @@ def protocol
       end
     end
 
+    def js_notifier=(*args)
+      warn '[HOPTOAD] config.js_notifier has been deprecated and has no effect.  You should use <%= hoptoad_javascript_notifier %> directly at the top of your layouts.  Be sure to place it before all other javascript.'
+    end
+
     def environment_filters
       warn 'config.environment_filters has been deprecated and has no effect.'
       []

diff --git a/lib/hoptoad_notifier/rails/javascript_notifier.rb b/lib/hoptoad_notifier/rails/javascript_notifier.rb
@@ -2,44 +2,39 @@ module HoptoadNotifier
   module Rails
     module JavascriptNotifier
       def self.included(base) #:nodoc:
-        base.send(:after_filter, :insert_hoptoad_javascript_notifier)
+        base.send :helper_method, :hoptoad_javascript_notifier
       end
 
       private
 
-      def insert_hoptoad_javascript_notifier
+      def hoptoad_javascript_notifier
         return unless HoptoadNotifier.configuration.public?
-        return unless HoptoadNotifier.configuration.js_notifier
 
-        path = File.join(File.dirname(__FILE__), '..', '..', 'templates', 'javascript_notifier.erb')
+        path = File.join File.dirname(__FILE__), '..', '..', 'templates', 'javascript_notifier.erb'
         host = HoptoadNotifier.configuration.host.dup
         port = HoptoadNotifier.configuration.port
         host << ":#{port}" unless [80, 443].include?(port)
 
-        options = {
-          :file          => path,
-          :layout        => false,
-          :use_full_path => false,
-          :locals        => {
-            :host        => host,
-            :api_key     => HoptoadNotifier.configuration.api_key,
-            :environment => HoptoadNotifier.configuration.environment_name
+        options              = {
+          :file              => path,
+          :layout            => false,
+          :use_full_path     => false,
+          :locals            => {
+            :host            => host,
+            :api_key         => HoptoadNotifier.configuration.api_key,
+            :environment     => HoptoadNotifier.configuration.environment_name,
+            :action_name     => action_name,
+            :controller_name => controller_name,
+            :url             => request.url
           }
         }
 
         if @template
-          javascript = @template.render(options)
+          @template.render(options)
         else
-          javascript = render_to_string(options)
+          render_to_string(options)
         end
 
-        if response.body.respond_to?(:gsub)
-          response.body = insert_javascript_after_head response.body, javascript
-        end
-      end
-
-      def insert_javascript_after_head(body, javascript)
-        body.gsub /<(head.*?)>/i, "<\\1>\n#{javascript}\n"
       end
 
     end

diff --git a/lib/templates/javascript_notifier.erb b/lib/templates/javascript_notifier.erb
@@ -1,17 +1,13 @@
-<script type="text/javascript"> 
-//<![CDATA[
-var notifierJsScheme = (("https:" == document.location.protocol) ? "https://" : "http://");
-document.write(unescape("%3Cscript src='" + notifierJsScheme + "<%= host %>/javascripts/notifier.js' type='text/javascript'%3E%3C/script%3E"));
-//]]>
-</script> 
+<%= javascript_tag %Q{
+    var notifierJsScheme = (("https:" == document.location.protocol) ? "https://" : "http://");
+    document.write(unescape("%3Cscript src='" + notifierJsScheme + "#{host}/javascripts/notifier.js' type='text/javascript'%3E%3C/script%3E"));
+  }
+-%>
 
-<script type="text/javascript">
-  Hoptoad.setKey('<%= api_key %>');
-  Hoptoad.setHost('<%= host %>');
-  Hoptoad.setEnvironment('<%= environment %>');
-    Hoptoad.setErrorDefaults({
-      url: "<%= a = "#{controller.request.protocol}#{controller.request.host_with_port}#{controller.request.request_uri}"; p a; a %>",
-      component: "<%= controller.controller_name %>",
-      action: "<%= controller.action_name %>"
-  });
-</script>
+<%= javascript_tag %Q{
+    Hoptoad.setKey('#{api_key}');
+    Hoptoad.setHost('#{host}');
+    Hoptoad.setEnvironment('#{environment}');
+    Hoptoad.setErrorDefaults({ url: "#{escape_javascript url}", component: "#{controller_name}", action: "#{action_name}" });
+  } 
+-%>
diff --git a/test/configuration_test.rb b/test/configuration_test.rb
@@ -29,7 +29,6 @@ class ConfigurationTest < Test::Unit::TestCase
                           HoptoadNotifier::Configuration::IGNORE_DEFAULT
     assert_config_default :development_lookup, true
     assert_config_default :framework, 'Standalone'
-    assert_config_default :js_notifier, false
   end
 
   should "provide default values for secure connections" do
@@ -85,7 +84,7 @@ class ConfigurationTest < Test::Unit::TestCase
      :http_read_timeout, :ignore, :ignore_by_filters, :ignore_user_agent,
      :notifier_name, :notifier_url, :notifier_version, :params_filters,
      :project_root, :port, :protocol, :proxy_host, :proxy_pass, :proxy_port,
-     :proxy_user, :secure, :development_lookup, :js_notifier].each do |option|
+     :proxy_user, :secure, :development_lookup].each do |option|
       assert_equal config[option], hash[option], "Wrong value for #{option}"
     end
   end
@@ -108,6 +107,14 @@ class ConfigurationTest < Test::Unit::TestCase
     assert_equal [], config.environment_filters
   end
 
+  should "warn when attempting to write js_notifier" do
+    config = HoptoadNotifier::Configuration.new
+    config.
+      expects(:warn).
+      with(regexp_matches(/deprecated/i))
+    config.js_notifier = true
+  end
+
   should "allow ignored user agents to be appended" do
     assert_appends_value :ignore_user_agent
   end

diff --git a/test/javascript_notifier_test.rb b/test/javascript_notifier_test.rb