Skip to content
This repository was archived by the owner on Aug 26, 2022. It is now read-only.

Conversation

@schalkneethling
Copy link

@jwhitlock This should be sufficient to expose the needed URLs to the JS in kuma and, make available a postMessage target call for the editor to get its parent URL.

Anything here that concern you security wise? Anything you would do different? Thanks!

@codecov-io
Copy link

codecov-io commented Sep 4, 2017

Codecov Report

Merging #4404 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4404      +/-   ##
==========================================
+ Coverage   94.73%   94.73%   +<.01%     
==========================================
  Files         254      254              
  Lines       22620    22621       +1     
  Branches     1666     1666              
==========================================
+ Hits        21428    21429       +1     
  Misses        963      963              
  Partials      229      229
Impacted Files Coverage Δ
kuma/settings/common.py 94.29% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ab1304...2fb93cb. Read the comment docs.

@escattone
Copy link
Contributor

escattone commented Sep 5, 2017

@jwhitlock @schalkneethling

Since @schalkneethling needs access to the editor URL (and the survey URL, which is not yet part of this PR) within kumascript macros as well, and since we'd prefer to have those settings in one place (here in kuma) rather than also having them somewhere in the kumascript settings, I'm proposing we should do the following (either in this PR or a related one):

  • make the INTERACTIVE_EXAMPLES_BASE setting configurable from the environment, like INTERACTIVE_EXAMPLES_BASE = config('INTERACTIVE_EXAMPLES_BASE', default='https://interactive-examples.mdn.mozilla.net') so we can set it from docker-compose.yml for local development
  • add an INTERACTIVE_EXAMPLES_SURVEY = config('INTERACTIVE_EXAMPLES_SURVEY', default='https://www.surveygizmo.com/s3/3780959/MDN-Interactive-Editor')
  • add these two key/value pairs to the env vars passed to kumascript both here and here, which adds them to the env global variable available within all kumascript macros (which is what @schalkneethling needs). For example, maybe we could add them like so:
env_vars = {
    ...
    'interactiveExamplesBaseURL': settings.INTERACTIVE_EXAMPLES_BASE,
    'interactiveExamplesSurveyURL': settings.INTERACTIVE_EXAMPLES_SURVEY,
}

which could be accessed within the kumascript macros like this:

env.interactiveExamplesBaseURL
  • add the proper local development setting for INTERACTIVE_EXAMPLES_BASE in the docker-compose.yml worker environment

What do you guys think?

@jwhitlock
Copy link
Contributor

@escattone that works for me. It is a little strange to pass this value, but it is also trivial to make it a KumaScript setting if that makes more sense in the future.

For speed, I think a backend dev should make those changes. @escattone, do you want to make these changes in a follow-on commit, or should I?

I think https://interactive-examples.mdn.mozilla.net is the right default for development, until we add a container for interactive examples development to the docker dev environment.

Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, with one nit. @escattone has ideas for making it configurable from the environment, and for extending it to KumaScript, which could be done in the context of this PR or a new one.

STAGING_DOMAIN = 'developer.allizom.org'
STAGING_URL = PROTOCOL + STAGING_DOMAIN

INTERACTIVE_EXAMPLES_BASE = 'https://interactive-examples.mdn.mozilla.net'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use config so it can be set from the environment, like:

INTERACTIVE_EXAMPLES_BASE = config('https://interactive-examples.mdn.mozilla.net', default='https://interactive-examples.mdn.mozilla.net')

@escattone
Copy link
Contributor

@jwhitlock It is a little strange. Maybe my desire not to repeat code is leading me down an un-natural path. I'm happy to just add it as a setting in kumascript as well. Maybe that's more natural.

@escattone
Copy link
Contributor

@jwhitlock I'll make the changes in a new kumascript PR, and I think I'll just add the settings to kumascript.

@stephaniehobson
Copy link
Contributor

Do we need to include the survey URL in this? It's going away in a few weeks and there isn't a staging version vs a production version.

@stephaniehobson
Copy link
Contributor

Data objects on the HTML tag feel like the wrong way to pass these to javascript. Can we make them part of our global MDN object with the other javascript config stuff in config.html

@schalkneethling
Copy link
Author

Can we make them part of our global MDN object with the other javascript config stuff in config.html

Most definitely, I was not aware of this file. Let me move it there and make the relevant updates.


INTERACTIVE_EXAMPLES_BASE = 'https://interactive-examples.mdn.mozilla.net'
INTERACTIVE_EXAMPLES_BASE = config(
'interactive-examples.mdn.mozilla.net',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be https://interactive-examples.mdn.mozilla.net?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crud, indeed. Copy/paste fail. Let me update it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed up.

@jwhitlock
Copy link
Contributor

This change worked locally, after I made some further changes to the Docker configuration. Your branch doesn't allow commits from maintainers, so I'll open a new PR for the changes:

https://github.com/mdn/interactive-examples/blob/b05330eac3bb104ff23022220f132c477f558c1d/js/editor-libs/analytics.js#L12

https://github.com/mdn/interactive-examples/blob/b05330eac3bb104ff23022220f132c477f558c1d/js/editor-libs/events.js#L92

  • Run interactive-examples locally
  • make up, and maybe docker-compose restart redis.

@jwhitlock jwhitlock merged commit 60a356c into mdn:master Sep 6, 2017
schalkneethling pushed a commit that referenced this pull request Aug 17, 2018
…4936)

This addresses some issues when reviewing PR #4935 and getting my Kuma dev environment to talk to a local checkout of https://github.com/mdn/interactive-examples/

* I gave @schalkneethling the wrong command in #4404 (comment) to override ``INTERACTIVE_EXAMPLES_BASE`` from the environment, this corrects it.
* The code that looks for the interactive example iframe events was always looking for the production domain, this changes it to look for ``INTERACTIVE_EXAMPLES_BASE`` via ``mdn.interactiveEditor.editorUrl``.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants