Skip to content

Add new setting to cms project - #645

Merged
chrisndodge merged 3 commits into
masterfrom
fix/cdodge/add-tech-support-email-config
Aug 12, 2013
Merged

Add new setting to cms project#645
chrisndodge merged 3 commits into
masterfrom
fix/cdodge/add-tech-support-email-config

Conversation

@chrisndodge

Copy link
Copy Markdown
Contributor

@adampalay @jzoldak can you review. We'll need to get this into this week's release.

Comment thread cms/envs/aws.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TECH_SUPPORT_EMAIL isn't defined in cms.envs.aws or cms.envs.common; it's in lms.envs.common. I would add TECH_SUPPORT_EMAIL to the things you already import from lms.envs.common into cms.envs.common

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's what I'm not clearly understanding. In cms/envs/common.py we have:

import lms.envs.common

Shouldn't that pick up the TECH_SUPPORT_EMAIL setting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added explicit definition in cms/envs/common.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actually not, you'd either have to specify "lms.envs.common.TECH_SUPPORT_EMAIL" or say "from lms.envs.common import TECH_SUPPORT_EMAIL". Importing a module doesn't add its namespace to your current one; you do that with the "from import " syntax.

Of course explicit definitions work too!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably cleanest to change cms/envs/dev line 29 to "from lms.envs.common import USE_TZ, TECH_SUPPORT_EMAIL".

@chrisndodge

Copy link
Copy Markdown
Contributor Author

@adampalay is this what you had in mind? I kept the override in aws.py in case there wanted to be a configurable means to change this in prod.

@adampalay

Copy link
Copy Markdown
Contributor

yep! 👍

chrisndodge pushed a commit that referenced this pull request Aug 12, 2013
@chrisndodge
chrisndodge merged commit bb644bb into master Aug 12, 2013
@chrisndodge
chrisndodge deleted the fix/cdodge/add-tech-support-email-config branch August 12, 2013 18:11
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
itsjeyd referenced this pull request in open-craft/openedx-platform Mar 24, 2016
itsjeyd referenced this pull request in open-craft/openedx-platform Mar 24, 2016
Merge pull request #645 from edx-solutions/rc/2016-03-09
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
dfrojas pushed a commit to eduNEXT/edx-platform that referenced this pull request Aug 31, 2017
DanielVZ96 referenced this pull request in open-craft/openedx-platform Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants