Fix onebox loading on every keystroke after a request fails.

app/assets/javascripts/discourse/components/onebox.js

@@ -1,89 +1,106 @@
 /**
   A helper for looking up oneboxes and displaying them
 
-  For now it only stores in a var, in future we can change it so it uses localStorage.
+  For now it only stores in a local Javascript Object, in future we can change it so it uses localStorage
+  or some other mechanism.
 
-  @class Notification
-  @extends Discourse.Model
+  @class Onebox
   @namespace Discourse
   @module Discourse
 **/
-Discourse.Onebox = (function() {
+Discourse.Onebox = {
 
-  var cache, load, localCache, lookup, lookupCache;
-  localCache = {};
+  // The cache is just a JS Object
+  localCache: {},
 
-  cache = function(url, contents) {
-    localCache[url] = contents;
-    return null;
-  };
+  // A cache of failed URLs
+  failedCache: {},
 
-  lookupCache = function(url) {
-    var cached;
-    cached = localCache[url];
-    if (cached && cached.then) {
-      return null;
-    } else {
-      return cached;
+  /**
+    Perform a lookup of a onebox based an anchor element. It will insert a loading
+    indicator and remove it when the loading is complete or fails.
+
+    @method load
+    @param {HTMLElement} e the anchor element whose onebox we want to look up
+    @param {Boolean} refresh true if we want to force a refresh of the onebox
+  **/
+  load: function(e, refresh) {
+
+    var $elem = $(e);
+
+    // If the onebox has loaded, return
+    if ($elem.data('onebox-loaded')) return;
+    if ($elem.hasClass('loading-onebox')) return;
+
+    var url = e.href;
+
+    // Unless we're forcing a refresh...
+    if (!refresh) {
+      // If we have it in our cache, return it.
+      var cached = this.localCache[url];
+      if (cached) return cached;
+
+      // If the request failed, don't do anything
+      var failed = this.failedCache[url];
+      if (failed) return;
     }
-  };
 
-  lookup = function(url, refresh, callback) {
-    var cached;
-    cached = localCache[url];
-    if (refresh && cached && !cached.then) {
-      cached = null;
-    }
-    if (cached) {
-      if (cached.then) {
-        cached.then(callback(lookupCache(url)));
-      } else {
-        callback(cached);
-      }
-      return false;
-    } else {
-      cache(url, jQuery.get("/onebox", {
-        url: url,
-        refresh: refresh
-      }, function(html) {
-        cache(url, html);
-        return callback(html);
-      }));
-      return true;
-    }
-  };
+    // Add the loading CSS class
+    $elem.addClass('loading-onebox');
 
-  load = function(e, refresh) {
-    var $elem, loading, url;
-    if (!refresh) refresh = false;
+    // Retrieve the onebox
+    var promise = $.ajax({
+      type: 'GET',
+      url: "/onebox",
+      data: { url: url, refresh: refresh }
+    });
 
-    url = e.href;
-    $elem = $(e);
-    if ($elem.data('onebox-loaded')) {
-      return;
-    }
-    loading = lookup(url, refresh, function(html) {
+    // We can call this when loading is complete
+    var loadingFinished = function() {
       $elem.removeClass('loading-onebox');
       $elem.data('onebox-loaded');
-      if (!html) {
-        return;
     }
-      if (html.trim().length === 0) {
-        return;
-      }
-      return $elem.replaceWith(html);
+
+    var onebox = this;
+    promise.then(function(html) {
+
+      // successfully loaded onebox
+      loadingFinished();
+
+      onebox.localCache[url] = html;
+      $elem.replaceWith(html);
+
+    }, function() {
+      // If the request failed log it as such
+      onebox.failedCache[url] = true;
+      loadingFinished();
     });
-    if (loading) {
-      return $elem.addClass('loading-onebox');
+
+  },
+
+  /**
+    Return the cached contents of a Onebox
+
+    @method lookupCache
+    @param {String} url the url of the onebox
+    @return {String} the cached contents of the onebox or null if not found
+  **/
+  lookupCache: function(url) {
+    return this.localCache[url];
+  },
+
+  /**
+    Store the contents of a Onebox in our local cache.
+
+    @method cache
+    @private
+    @param {String} url the url of the onebox we crawled
+    @param {String} contents the contents we want to cache
+  **/
+  cache: function(url, contents) {
+    this.localCache[url] = contents;
   }
+
 };
 
-  return {
-    load: load,
-    lookup: lookup,
-    lookupCache: lookupCache
-  };
-
-})();
-
 

app/assets/javascripts/discourse/views/composer_view.js

@@ -152,19 +152,27 @@ Discourse.ComposerView = Discourse.View.extend({
 
   // Called after the preview renders. Debounced for performance
   afterRender: Discourse.debounce(function() {
-    var $wmdPreview, refresh,
-      _this = this;
-    $wmdPreview = $('#wmd-preview');
-    if ($wmdPreview.length === 0) {
-      return;
-    }
+    var $wmdPreview = $('#wmd-preview');
+    if ($wmdPreview.length === 0) return;
+
     Discourse.SyntaxHighlighting.apply($wmdPreview);
-    refresh = this.get('controller.content.post.id') !== void 0;
+
+    var post = this.get('controller.content.post');
+    var refresh = false;
+
+    // If we are editing a post, we'll refresh its contents once. This is a feature that
+    // allows a user to refresh its contents once.
+    if (post && post.blank('refreshedPost')) {
+      refresh = true
+      post.set('refreshedPost', true);
+    }
+
+    // Load the post processing effects
     $('a.onebox', $wmdPreview).each(function(i, e) {
-      return Discourse.Onebox.load(e, refresh);
+      Discourse.Onebox.load(e, refresh);
     });
-    return $('span.mention', $wmdPreview).each(function(i, e) {
-      return Discourse.Mention.load(e, refresh);
+    $('span.mention', $wmdPreview).each(function(i, e) {
+      Discourse.Mention.load(e, refresh);
     });
   }, 100),
 

app/controllers/onebox_controller.rb

@@ -4,7 +4,16 @@ class OneboxController < ApplicationController
 
   def show
     Oneboxer.invalidate(params[:url]) if params[:refresh].present?
-    render text: Oneboxer.preview(params[:url])
+
+    result = Oneboxer.preview(params[:url])
+    result.strip! if result.present?
+
+    # If there is no result, return a 404
+    if result.blank?
+      render nothing: true, status: 404
+    else
+      render text: result
+    end
   end
 
 end

spec/controllers/onebox_controller_spec.rb

@@ -2,14 +2,46 @@ require 'spec_helper'
 
 describe OneboxController do
 
-  it 'asks the oneboxer for the preview' do
-    Oneboxer.expects(:preview).with('http://google.com')
-    xhr :get, :show, url: 'http://google.com'
-  end
+  let(:url) { "http://google.com" }
 
   it 'invalidates the cache if refresh is passed' do
-    Oneboxer.expects(:invalidate).with('http://google.com')
-    xhr :get, :show, url: 'http://google.com', refresh: true
+    Oneboxer.expects(:invalidate).with(url)
+    xhr :get, :show, url: url, refresh: true
+  end
+
+  describe "found onebox" do
+
+    let(:body) { "this is the onebox body"}
+
+    before do
+      Oneboxer.expects(:preview).with(url).returns(body)
+      xhr :get, :show, url: url
+    end
+
+    it 'returns success' do
+      response.should be_success
+    end
+
+    it 'returns the onebox response in the body' do
+      response.body.should == body
+    end
+
+  end
+
+  describe "missing onebox" do
+
+    it "returns 404 if the onebox is nil" do
+      Oneboxer.expects(:preview).with(url).returns(nil)
+      xhr :get, :show, url: url
+      response.response_code.should == 404
+    end
+
+    it "returns 404 if the onebox is an empty string" do
+      Oneboxer.expects(:preview).with(url).returns(" \t ")
+      xhr :get, :show, url: url
+      response.response_code.should == 404
+    end
+
   end
 
 end

spec/javascripts/components/onebox_spec.js

@@ -2,19 +2,22 @@
 
 describe("Discourse.Onebox", function() {
 
+  var anchor;
+
   beforeEach(function() {
     spyOn(jQuery, 'ajax').andCallThrough();
+    anchor = $("<a href='http://bla.com'></a>")[0];
   });
 
   it("Stops rapid calls with cache true", function() {
-    Discourse.Onebox.lookup('http://bla.com', true, function(c) { return c; });
-    Discourse.Onebox.lookup('http://bla.com', true, function(c) { return c; });
+    Discourse.Onebox.load(anchor, true);
+    Discourse.Onebox.load(anchor, true);
     expect(jQuery.ajax.calls.length).toBe(1);
   });
 
   it("Stops rapid calls with cache false", function() {
-    Discourse.Onebox.lookup('http://bla.com/a', false, function(c) { return c; });
-    Discourse.Onebox.lookup('http://bla.com/a', false, function(c) { return c; });
+    Discourse.Onebox.load(anchor, false);
+    Discourse.Onebox.load(anchor, false);
     expect(jQuery.ajax.calls.length).toBe(1);
   });