-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make help email address configurable instead of hard coding #3384
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! Thank you @sjanssen2. Just a couple of questions.
qiita_core/configuration_manager.py
Outdated
self.help_email = config.get('main', 'HELP_EMAIL') | ||
if not self.help_email: | ||
self.help_email = 'qiita.help@gmail.com' | ||
|
||
self.sysadmin_email = config.get('main', 'SYSADMIN_EMAIL') | ||
if not self.sysadmin_email: | ||
self.sysadmin_email = 'jdereus@health.ucsd.edu' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about this being better than actually raising an error if those are not defined; what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree! Thus, we remove "personalized" emails completely. What type of error do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess something like: raise ValueError('You need to have a value in the HELP_EMAIL to continue')
but anything that lets the person configuring/starting the system that they are missing that value should work fine. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I was wondering about the class of the error, not the message ;-) Sorry for being unspecific.
BTW just noticed that this PR was issued against master vs. dev; I just changed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments.
qiita_core/configuration_manager.py
Outdated
@@ -117,6 +117,12 @@ class ConfigurationManager(object): | |||
The script used to start the plugins | |||
plugin_dir : str | |||
The path to the directory containing the plugin configuration files | |||
help_email : str | |||
The email address a user should write to when asking for help | |||
Defaults to qiita.help@gmail.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer true, right? Could you remove this and the one below?
# Address a user should write to when asking for help | ||
HELP_EMAIL = qiita.help@gmail.com | ||
|
||
# The email address, Qiita sends internal notifications to a sys admin | ||
SYSADMIN_EMAIL = jdereus@health.ucsd.edu | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these match the ones used in the actual test?
# Address a user should write to when asking for help
HELP_EMAIL = foo@bar.com
# The email address, Qiita sends internal notifications to a sys admin
SYSADMIN_EMAIL = jeff@bar.com
and perhaps changing the error (which should never happen now) to a warning when the emails match those.
@@ -234,6 +240,22 @@ def _get_main(self, config): | |||
self.key_file = join(install_dir, 'qiita_core', 'support_files', | |||
'ci_server.key') | |||
|
|||
self.help_email = config.get('main', 'HELP_EMAIL') | |||
if not self.help_email: | |||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change this to something like:
if self.help_email == 'foo@bar.com':
warnings.warn("Using the github fake email for HELP_EMAIL, are you sure this is OK?")
@@ -247,6 +245,11 @@ def _get_main(self, config): | |||
"section of Qiita's config file. This address is essential " | |||
"for users to ask for help as it is displayed at various " | |||
"location throughout Qiita's web pages.") | |||
if (self.help_email == 'foo@bar.com') and \ | |||
(self.test_environment is False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Good idea!
Addresses issue #3375
When we want to run multiple Qiita instances, it would make sense to use different email addresses a user can get help from. Therefore, I extended the configuration manager to parse two new strings for the help_email address and Jeff's sys-admin address.
Once you merge this PR, don't forget to update your configuration files!