bug 760240: provide interactive-examples settings to macros #306

escattone · 2017-09-06T00:19:57Z

This PR:

adds the interactive-example settings to kumascript, and makes them available in macros via the env.interactive_examples object
changes the EmbedInteractiveExample.ejs macro to use the new setting, as well as provides tests for the macro
is meant to replace the approach outlined in Fix issue #255, exposes site url and editor url to JS and editor kuma#4404 (comment), which although DRY seemed like an unnecessarily complicated way to provide the setting to kumascript.
does not provide the survey URL as part of the settings, since that seems only temporary per Fix issue #255, exposes site url and editor url to JS and editor kuma#4404 (comment)

escattone · 2017-09-06T00:22:14Z

By the way, I'm sorry for the lack of camel-case. Python's practices are so embedded that I sometimes forget when I'm writing JavaScript. Feel free to suggest otherwise, and I'd be happy to change.

schalkneethling · 2017-09-06T09:15:47Z

r+ from my side. One question though, in a situation(and I am not at all certain how regular this would happen) where you wanted to run Kuma, Kumascript and the interactive editor locally. Is there than a way to override the URL using a local.conf or something of the sort?

jwhitlock · 2017-09-06T14:43:25Z

Yes, you can change the configuration by setting environment variables. There's a small section in the docs that mentions it.

With this change, I think the method would be:

Copy .env-dist.dev to .env, if you haven't already
Add a line:

INTERACTIVE_EXAMPLES_BASE=http://localhost:8003

Restart the services with make up

We'd need to update docker-compose.yml to set the container environment based on that variables.

In the worker section:

  - INTERACTIVE_EXAMPLES_BASE=${INTERACTIVE_EXAMPLES_BASE:-https://interactive-examples.mdn.mozilla.net}

In the kumascript section:

interactive_examples__base_url=${INTERACTIVE_EXAMPLES_BASE:-https://interactive-examples.mdn.mozilla.net}

If there's going to be a lot of development around these, then we may want to add an interactive examples container to docker-compose.yml, or provide an alternate configuration.

escattone · 2017-09-06T15:22:16Z

Thanks @jwhitlock !

jwhitlock · 2017-09-06T16:46:39Z

I'm going to try adding a commit updating docker-compose.yml, etc to mdn/kuma#4404

jwhitlock · 2017-09-06T17:08:03Z

macros/EmbedInteractiveExample.ejs

 let surveyClass = "interactive-editor-survey";
 let surveyLink = "https://www.surveygizmo.com/s3/3780959/MDN-Interactive-Editor";
-let url = "https://interactive-examples.mdn.mozilla.net/" + $0;
+let url = kuma.url.resolve(env.interactive_examples.base_url, $0);


I'm getting an error:

Cannot read property 'base_url' of undefined at eval (eval at compile (/node_modules/ejs/lib/ejs.js:491:12), <anonymous>:21:52) at returnedFn (/node_modules/ejs/lib/ejs.js:520:17) at /app/lib/kumascript/templates.js:71:26

Adding some debug statements to see what is going on...

@jwhitlock Do you get this when you run make test-macros? Hmm, my first guess is that you may need to rebuild your kumascript image cd .. && make build-kumascript?

No, the macros tests explictly set env.interactive_examples, so the error doesn't occur. It only occurs when using the value set by server.js

jwhitlock

After the change in server.js, this works locally, and allows the iframe source to be http://127.0.0.1:8080. Kuma needs to whitelist this as a domain for iframes as well, or it gets filtered out during sanitizing.

jwhitlock · 2017-09-06T18:44:17Z

macros/EmbedInteractiveExample.ejs

 let surveyClass = "interactive-editor-survey";
 let surveyLink = "https://www.surveygizmo.com/s3/3780959/MDN-Interactive-Editor";
-let url = "https://interactive-examples.mdn.mozilla.net/" + $0;
+let url = kuma.url.resolve(env.interactive_examples.base_url, $0);


No, the macros tests explictly set env.interactive_examples, so the error doesn't occur. It only occurs when using the value set by server.js

jwhitlock · 2017-09-06T18:44:50Z

lib/kumascript/server.js

            }

+            // Add a clone of the interactive-examples settings to "env".
+            env.interactive_examples = _.clone(


This should be:

env.interactive_examples = ks_conf.nconf.get('interactive_examples');

Oh man, good catch, sorry about that! Fix coming soon...

stephaniehobson

I'm not sure I understand this well enough to review it.

jwhitlock · 2017-09-06T19:08:10Z

Also, I think bug 760240 is the wrong one. Maybe bug 1308630?

escattone · 2017-09-06T19:08:36Z

@jwhitlock I amended my original commit to address your requested fix. Good catch, I should have tried it manually. 😞 I think it's good to go now?

escattone · 2017-09-06T19:17:21Z

@jwhitlock Ugh, you're right, that bugzilla issue is not the right one, sorry! I'm not sure where I got that one from now that I think of it. Yes, I think bug 1308630 will work fine, thanks. I'll amend the commit again to use that one.

* add interactive-examples settings to conf.js * add interactive-examples settings to env object of each macro's APIContext * add tests for EmbedInteractiveExample.ejs

jwhitlock

👍 this works now, thanks @escattone!

escattone requested review from jwhitlock, schalkneethling and stephaniehobson September 6, 2017 00:20

jwhitlock reviewed Sep 6, 2017

View reviewed changes

jwhitlock suggested changes Sep 6, 2017

View reviewed changes

stephaniehobson reviewed Sep 6, 2017

View reviewed changes

jwhitlock mentioned this pull request Sep 6, 2017

Fix issue #255, exposes site url and editor url to JS and editor mdn/kuma#4404

Merged

bug 1308630: provide interactive-examples settings to macros

372a82c

* add interactive-examples settings to conf.js * add interactive-examples settings to env object of each macro's APIContext * add tests for EmbedInteractiveExample.ejs

jwhitlock mentioned this pull request Sep 6, 2017

bug 1308630: Set Interactive Examples URL from env mdn/kuma#4413

Merged

jwhitlock approved these changes Sep 6, 2017

View reviewed changes

jwhitlock merged commit 7bd5390 into mdn:master Sep 6, 2017

schalkneethling mentioned this pull request Sep 7, 2017

KumaScript: Expose environment URL to macro mdn/interactive-examples#256

Closed

escattone deleted the interactive-settings-in-macros-760240 branch October 31, 2017 17:55

bug 760240: provide interactive-examples settings to macros #306

bug 760240: provide interactive-examples settings to macros #306

Uh oh!

Conversation

escattone commented Sep 6, 2017

Uh oh!

escattone commented Sep 6, 2017

Uh oh!

schalkneethling commented Sep 6, 2017

Uh oh!

jwhitlock commented Sep 6, 2017

Uh oh!

escattone commented Sep 6, 2017

Uh oh!

jwhitlock commented Sep 6, 2017

Uh oh!

jwhitlock Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

escattone Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

jwhitlock Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

jwhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

jwhitlock Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

jwhitlock Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

escattone Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

stephaniehobson left a comment

Choose a reason for hiding this comment

Uh oh!

jwhitlock commented Sep 6, 2017

Uh oh!

escattone commented Sep 6, 2017

Uh oh!

escattone commented Sep 6, 2017

Uh oh!

jwhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants