Skip to content

Conversation

@pkulkark
Copy link
Contributor

@pkulkark pkulkark commented Sep 20, 2020

One of our clients require to be able to send customizable values to their custom tinymce plugin that they want to use in studio. This PR adds the ability to share additional node environment variables that are set in django settings file with any js plugins. They can then be accessed via process.env.<variable-name>.

JIRA tickets: OSPR-4988

Sandbox URL: TBD - sandbox is being provisioned

Testing Instructions:

  1. Set any required environment variables inside the ADDITIONAL_NODE_ENV_VARS variable in cms/envs/common.py file.
  2. Verify that you can access those environment variables from within js code (can try within any of the active tinymce plugins). You can get the value of the environment variables by parsing process.env.ADDITIONAL_NODE_ENV_VARS.

Reviewers:

@openedx-webhooks
Copy link

openedx-webhooks commented Sep 20, 2020

Thanks for the pull request, @pkulkark! I've created OSPR-4988 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 20, 2020
@pkulkark pkulkark force-pushed the pooja/allow-custom-node-env-variables branch 2 times, most recently from 4a7b6e8 to 560d4cd Compare September 20, 2020 13:06
@natabene
Copy link
Contributor

@pkulkark Thank you for your contribution. Please let me know once this is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Sep 21, 2020
Copy link
Contributor

@mavidser mavidser left a comment

Choose a reason for hiding this comment

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

@pkulkark Small nit - else LGTM 👍

  • I tested this: Accessed process.env.ADDITIONAL_NODE_ENV_VARS from some compiled js files and was able to access defined variables.
  • I read through the code
  • I checked for accessibility issues

@pkulkark pkulkark force-pushed the pooja/allow-custom-node-env-variables branch from 560d4cd to 0f7d63b Compare September 24, 2020 06:55
This adds a way for tinymce plugins to use
custom environment variables.
@pkulkark pkulkark force-pushed the pooja/allow-custom-node-env-variables branch from 0f7d63b to e28ed7e Compare September 24, 2020 06:58
@pkulkark pkulkark force-pushed the pooja/allow-custom-node-env-variables branch from f54e0b1 to c4be8c7 Compare September 24, 2020 14:14
@natabene
Copy link
Contributor

@pkulkark Is this good for our review?

@edx-status-bot
Copy link

Your PR has finished running tests. The following contexts failed:

  • jenkins/python-3.8/a11y
  • jenkins/python-3.8/quality
  • jenkins/python-3.8/python
  • jenkins/a11y
  • jenkins/quality
  • jenkins/python

@gabor-boros
Copy link
Contributor

@pkulkark Could you please rebase this PR and ensure this is ready for review? Or if the feature is not needed anymore close the PR?

@nizarmah
Copy link
Contributor

nizarmah commented May 7, 2021

@gabor-boros @pkulkark I believe this pull request should have been closed because it was addressed in other pull requests with a different approach (https://github.com/edx/configuration/pull/6179, https://github.com/edx/edx-platform/pull/25695, https://github.com/edx/edx-platform/pull/25324).

@pkulkark pkulkark closed this May 10, 2021
@openedx-webhooks
Copy link

@pkulkark Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@openedx-webhooks openedx-webhooks added rejected and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants