handle multiple RCE requests

fixes CNVS-27951

also document which methods are not in the
public interface to be clear
which methods are ok for other modules to call
on the service loader

TEST PLAN:
 1) go to some RCE use case with the service
    enabled
 2) make sure only 1 "get_module" call happens
    in the network panel

Change-Id: I064436e2a9ea7aa1c340848a2bf3e91dfcd23bb5
Reviewed-on: https://gerrit.instructure.com/74292
Tested-by: Jenkins
Reviewed-by: Jacob Fugal <jacob@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Product-Review: Ethan Vizitei <evizitei@instructure.com>

app/jsx/shared/rce/serviceRCELoader.jsx

@@ -6,79 +6,10 @@ define([
   'jsx/shared/rce/loadEventListeners'
 ], function($, _, editorOptions, RCEStore, loadEventListeners){
   let RCELoader = {
-    cachedModule: null,
-
-    setCache(val) {
-      this.cachedModule = val;
-    },
-
     preload(host) {
       this.loadRCE(host, function(){})
     },
 
-    loadRCE(host, cb) {
-      if(this.cachedModule !== null){
-        cb(this.cachedModule)
-      } else {
-        // trim trailing slash if there is one, as we're going to add one below
-        host = host.replace(/\/$/, "")
-        var moduleUrl = '//'+ host +'/get_module'
-        if(host.indexOf("cloudfront") > -1){
-          moduleUrl = '//' + host + '/latest'
-        }
-        $.getScript(moduleUrl, (res) => {
-          loadEventListeners()
-          if(!this.cachedModule){ this.setCache(RceModule) }
-          cb(this.cachedModule);
-        })
-      }
-    },
-
-    getTargetTextarea(initialTarget) {
-      return $(initialTarget).get(0).type == "textarea" ?
-        $(initialTarget).get(0) :
-        $(initialTarget).find("textarea").get(0)
-    },
-
-    getRenderingTarget(textarea) {
-      return $(textarea).parent().get(0)
-    },
-
-    // anything that canvas needs to later find this
-    // editor/textarea should be mirrored here so we
-    // dont have to change too much canvas code
-    _attrsToMirror(textarea) {
-      let validAttrs = ["name"]
-      let attrs = _.reduce(textarea.attributes, (memo, attr) => {
-        memo[attr.name] = attr.value
-        return memo
-      }, {})
-
-      return _.pick(attrs, validAttrs)
-    },
-
-    createRCEProps(textarea, tinyMCEInitOptions) {
-      let textareaClassName = textarea.classList + " " + RCEStore.classKeyword
-      let width = textarea.offsetWidth
-      let height = textarea.offsetHeight
-
-      if (height){
-        tinyMCEInitOptions.tinyOptions = _.extend({},
-          {height: height},
-          (tinyMCEInitOptions.tinyOptions || {})
-        )
-      }
-
-      return {
-        editorOptions: editorOptions.bind(null, width, textarea.id, tinyMCEInitOptions, null),
-        defaultContent: textarea.value || tinyMCEInitOptions.defaultContent,
-        textareaId: textarea.id,
-        textareaClassName: textareaClassName,
-        language: ENV.LOCALE,
-        mirroredAttrs: this._attrsToMirror(textarea)
-      }
-    },
-
     loadOnTarget(target, tinyMCEInitOptions, host) {
       const textarea = this.getTargetTextarea(target)
       const getTargetFn = tinyMCEInitOptions.getRenderingTarget || this.getRenderingTarget
@@ -99,8 +30,150 @@ define([
       this.loadRCE(host, function (RCE) {
         RCE.renderSidebarIntoDiv(target, props, callback)
       })
-    }
+    },
 
+    /**
+    * properties for managing several requests to load
+    * the module from various pieces of canvas code.
+    *
+    * @private
+    */
+    cachedModule: null,
+    loadingFlag: false,
+    loadingCallbacks: [],
+
+    /**
+    * handle accepting new load requests depending on the current state
+    * of the load/cache cycle
+    *
+    * @private
+    */
+    loadRCE(host, cb) {
+      if(this.cachedModule !== null){
+        cb(this.cachedModule)
+      } else {
+        this.loadingCallbacks.push(cb)
+        if(!this.loadingFlag){
+          // we need to make sure we don't make this kinda expensive request
+          // multiple times, so anybody who wants the module can queue up
+          // a callback, but first one to this point performs the load
+          this.loadingFlag = true
+          let moduleUrl = this.buildModuleUrl(host)
+          $.getScript(moduleUrl, (res) => { this.onRemoteLoad() })
+        }
+      }
+    },
+
+    /**
+    * sometimes we get passed a container that has the
+    * textarea nested within it, we want to normalize to
+    * just using whatever textarea is going to be bound to
+    * the editor
+    *
+    * @private
+    * @return {Element} the textarea
+    */
+    getTargetTextarea(initialTarget) {
+      return $(initialTarget).get(0).type == "textarea" ?
+        $(initialTarget).get(0) :
+        $(initialTarget).find("textarea").get(0)
+    },
+
+    /**
+    * the immediate parent of the textarea is the container
+    * we want to render the remote react component inside, so it's
+    * a sibiling with the actual form element being populated.
+    *
+    * @private
+    * @return {Element} container element for rendering remote editor
+    */
+    getRenderingTarget(textarea) {
+      return $(textarea).parent().get(0)
+    },
+
+    /**
+    * anything that canvas needs to later find this
+    * editor/textarea should be mirrored here so we
+    * dont have to change too much canvas code
+    *
+    * @private
+    * @return {Hash}
+    */
+    _attrsToMirror(textarea) {
+      let validAttrs = ["name"]
+      let attrs = _.reduce(textarea.attributes, (memo, attr) => {
+        memo[attr.name] = attr.value
+        return memo
+      }, {})
+
+      return _.pick(attrs, validAttrs)
+    },
+
+    /**
+     * merges options provided by consuming code with some
+     * intelligent defaults so that simple use cases can not
+     * worry about providing repetitive options hashes.
+     *
+     * @private
+     * @return {Hash} ready-to-use options hash to use as react props
+     */
+    createRCEProps(textarea, tinyMCEInitOptions) {
+      let textareaClassName = textarea.classList + " " + RCEStore.classKeyword
+      let width = textarea.offsetWidth
+      let height = textarea.offsetHeight
+
+      if (height){
+        tinyMCEInitOptions.tinyOptions = _.extend({},
+          {height: height},
+          (tinyMCEInitOptions.tinyOptions || {})
+        )
+      }
+
+      return {
+        editorOptions: editorOptions.bind(null, width, textarea.id, tinyMCEInitOptions, null),
+        defaultContent: textarea.value || tinyMCEInitOptions.defaultContent,
+        textareaId: textarea.id,
+        textareaClassName: textareaClassName,
+        language: ENV.LOCALE,
+        mirroredAttrs: this._attrsToMirror(textarea)
+      }
+    },
+
+    /**
+     * helps with url construction which is different when using a CDN
+     * than when loading directly from an RCE server
+     *
+     * @private
+     * @return {String} ready-to-use URL for loading RCE remotely
+     */
+    buildModuleUrl(host) {
+      // trim trailing slash if there is one, as we're going to add one below
+      host = host.replace(/\/$/, "")
+      var moduleUrl = '//'+ host +'/get_module'
+      if(host.indexOf("cloudfront") > -1){
+        moduleUrl = '//' + host + '/latest'
+      }
+      return moduleUrl
+    },
+
+    /**
+     * called when remote module has finished loading
+     * so we can set the cache appropriately, un-set the loading
+     * flag, and deal with any callbacks that have been queueing up that
+     * need the module to execute.  Anything outside of this file using
+     * this function could damage state and make the remote module loading fail.
+     *
+     * @private
+     */
+    onRemoteLoad() {
+      loadEventListeners()
+      if(!this.cachedModule){ this.cachedModule = RceModule }
+      this.loadingFlag = false
+      this.loadingCallbacks.forEach((loadingCallback)=>{
+        loadingCallback(this.cachedModule)
+      })
+      this.loadingCallbacks = []
+    }
   }
 
   return RCELoader;

spec/coffeescripts/helpers/editorUtils.coffee

@@ -0,0 +1,9 @@
+define ['jsx/shared/rce/serviceRCELoader'], (RCELoader) ->
+  return {
+    resetRCE: ()=>
+      window.tinyrce = null
+      window.RceModule = null
+      RCELoader.cachedModule = null
+      RCELoader.loadingFlag = false
+      RCELoader.loadingCallbacks = []
+  }

spec/coffeescripts/jsx/shared/rce/IntegrationSpec.coffee

@@ -2,8 +2,9 @@ define [
   'jsx/shared/rce/RichContentEditor',
   'jsx/shared/rce/serviceRCELoader',
   'jquery',
-  'helpers/fakeENV'
-], (RichContentEditor, RCELoader, $, fakeENV) ->
+  'helpers/fakeENV',
+  'helpers/editorUtils'
+], (RichContentEditor, RCELoader, $, fakeENV, editorUtils) ->
 
   wikiSidebar = undefined
 
@@ -35,8 +36,7 @@ define [
       fakeENV.teardown()
       $('#fixtures').empty()
       $.getScript.restore()
-      # don't leave cached module hanging around, can break other specs
-      RCELoader.setCache(null)
+      editorUtils.resetRCE()
 
   test "instatiating a remote editor", ->
     ENV.RICH_CONTENT_SERVICE_ENABLED = true

spec/coffeescripts/jsx/shared/rce/RichContentEditorSpec.coffee

@@ -3,7 +3,8 @@ define [
   'jsx/shared/rce/serviceRCELoader',
   'jsx/shared/rce/rceStore',
   'helpers/fakeENV'
-], (RichContentEditor, serviceRCELoader, rceStore, fakeENV) ->
+  'helpers/editorUtils'
+], (RichContentEditor, serviceRCELoader, rceStore, fakeENV, editorUtils) ->
 
   wikiSidebar = undefined
 
@@ -16,6 +17,7 @@ define [
     teardown: ->
       fakeENV.teardown()
       serviceRCELoader.preload.restore()
+      editorUtils.resetRCE()
 
   test 'loads with CDN host if available', ->
     ENV.RICH_CONTENT_CDN_HOST = "cdn-host"
@@ -45,28 +47,28 @@ define [
       fakeENV.setup()
       ENV.RICH_CONTENT_SERVICE_ENABLED = true
       ENV.RICH_CONTENT_APP_HOST = "http://fakehost.com"
-      @originalLoadOnTarget = serviceRCELoader.loadOnTarget
       @target = {
         attr: (()-> "fakeTarget")
       }
       @fakeJquery = ()=>
         return @target # length is at least one
       @fakeJquery.extend = $.extend
-      serviceRCELoader.loadOnTarget = sinon.stub()
+      @loadOnTargetStub = sinon.stub(serviceRCELoader, "loadOnTarget")
+
     teardown: ->
-      serviceRCELoader.loadOnTarget = @originalLoadOnTarget
+      serviceRCELoader.loadOnTarget.restore()
       fakeENV.teardown()
 
   test 'calls serviceRCELoader.loadOnTarget with a target and host', ->
     richContentEditor = new RichContentEditor({riskLevel: 'basic', jQuery: @fakeJquery})
     richContentEditor.loadNewEditor(@target, {})
-    ok serviceRCELoader.loadOnTarget.calledWith(@target, {}, "http://fakehost.com")
+    ok @loadOnTargetStub.calledWith(@target, {}, "http://fakehost.com")
 
   test 'CDN host overrides app host', ->
     ENV.RICH_CONTENT_CDN_HOST = "http://fakecdn.net"
     richContentEditor = new RichContentEditor({riskLevel: 'basic', jQuery: @fakeJquery})
     richContentEditor.loadNewEditor(@target, {})
-    ok serviceRCELoader.loadOnTarget.calledWith(@target, {}, "http://fakecdn.net")
+    ok @loadOnTargetStub.calledWith(@target, {}, "http://fakecdn.net")
 
   test 'calls editorBox and set_code when feature flag off', ->
     ENV.RICH_CONTENT_SERVICE_ENABLED = false
@@ -88,7 +90,7 @@ define [
     notFoundJquery.extend = $.extend
     richContentEditor = new RichContentEditor({riskLevel: 'basic', jQuery: notFoundJquery})
     richContentEditor.loadNewEditor(".invalidTarget", {})
-    ok !serviceRCELoader.loadOnTarget.called
+    ok @loadOnTargetStub.notCalled
 
   module 'RichContentEditor - initSidebar',
     setup: ->
@@ -107,11 +109,11 @@ define [
         attachToEditor: (ed)->
           wikiSidebar.editor = ed
       }
-      @_loadSidebarOnTarget = serviceRCELoader.loadSidebarOnTarget
-      serviceRCELoader.loadSidebarOnTarget = (target, host, callback)->
+      sinon.stub serviceRCELoader, "loadSidebarOnTarget", (target, host, callback)->
         callback({is_a: 'remote_sidebar'})
+
     teardown: ->
-      serviceRCELoader.loadSidebarOnTarget = @_loadSidebarOnTarget
+      serviceRCELoader.loadSidebarOnTarget.restore()
       fakeENV.teardown()
 
   test 'uses wikiSidebar when feature flag off', ->
@@ -174,6 +176,7 @@ define [
     teardown: ->
       fakeENV.teardown()
       rceStore.callOnRCE.restore()
+      editorUtils.resetRCE()
 
   test 'with flag enabled, ultimately lets RCEStore handle the message', ->
     ENV.RICH_CONTENT_SERVICE_ENABLED = true

spec/coffeescripts/jsx/shared/rce/serviceRCELoaderSpec.coffee

@@ -1,33 +1,26 @@
 define [
   'jquery'
   'jsx/shared/rce/serviceRCELoader'
-], ($, RCELoader) ->
+  'helpers/editorUtils'
+], ($, RCELoader, editorUtils) ->
   module 'loadRCE',
     setup: ->
       # make sure we don't get a cached thing from other tests
-      RCELoader.setCache(null)
+      RCELoader.cachedModule = null
       @elementInFixtures = (type) ->
         newElement = document.createElement(type)
         fixtureDiv = document.getElementById("fixtures")
         fixtureDiv.appendChild(newElement)
         newElement
-      @modifiedJquery = $
-      @originalGetScript = @modifiedJquery.getScript
-      @modifiedJquery.getScript = (__host__, cb) ->
-        # spoofing getScript behavior
-        $.globalEval( "RceModule = 'fakeModule'" )
-        cb()
+      @getScriptSpy = sinon.stub $, "getScript", (__host__, callback)=>
+        window.RceModule = 'fakeModule'
+        callback()
 
-      @getScriptSpy = sinon.spy(@modifiedJquery, "getScript");
-
-      window.$ = @modifiedJquery
       window.tinyrce = {editorsListing: {}}
 
     teardown: ->
-      RCELoader.setCache(null)
-      @modifiedJquery.getScript.restore()
-      @modifiedJquery.getScript = @originalGetScript
-      window.tinyrce = null
+      $.getScript.restore()
+      editorUtils.resetRCE()
       document.getElementById("fixtures").innerHtml = ""
 
   # loading RCE
@@ -36,15 +29,13 @@ define [
     equal RCELoader.cachedModule, null
     RCELoader.loadRCE("somehost.com", (() ->))
     equal RCELoader.cachedModule, 'fakeModule'
-    ok @getScriptSpy.calledOnce
+    ok(@getScriptSpy.calledOnce)
 
   test 'does not call get_module once a response has been cached', ->
-    RCELoader.cachedModule = "something"
-    RCELoader.setCache("foo")
-
-    ok @getScriptSpy.notCalled
+    RCELoader.cachedModule = "foo"
+    ok(@getScriptSpy.notCalled)
     RCELoader.loadRCE("somehost.com", () -> "noop")
-    ok @getScriptSpy.notCalled
+    ok(@getScriptSpy.notCalled)
 
   test 'executes a callback when RCE is loaded', ->
     cb = sinon.spy();
@@ -52,9 +43,9 @@ define [
     ok cb.called
 
   test 'uses `latest` version when hitting a cloudfront host', ->
-    ok @getScriptSpy.notCalled
+    ok(@getScriptSpy.notCalled)
     RCELoader.loadRCE("xyz.cloudfront.net/some/path", () -> "noop")
-    ok @getScriptSpy.calledWith("//xyz.cloudfront.net/some/path/latest")
+    ok(@getScriptSpy.calledWith("//xyz.cloudfront.net/some/path/latest"))
 
   # target finding
 
@@ -125,3 +116,29 @@ define [
     ta.setAttribute("name", "elementName")
     props = RCELoader.createRCEProps(ta, {defaultContent: "default text"})
     equal props.mirroredAttrs.name, "elementName"
+
+  test 'only tries to load the module once', ->
+    RCELoader.preload("host")
+    RCELoader.preload("host")
+    RCELoader.preload("host")
+    ok(@getScriptSpy.calledOnce)
+
+
+  asyncTest 'handles callbacks once module is loaded', ->
+    expect(1)
+    resolveGetScript = null
+    $.getScript.restore()
+    sinon.stub $, "getScript", (__host__, callback)=>
+      resolveGetScript = ()->
+        window.RceModule = 'fakeModule'
+        callback()
+    # first make sure all the state is fixed so test call isn't
+    # making it's own getScript call
+    RCELoader.preload("host")
+    RCELoader.loadRCE("host", (()=>))
+
+    # now setup while-in-flight load request to check
+    RCELoader.loadRCE "host", (module)=>
+      start()
+      equal(module, "fakeModule")
+    resolveGetScript()