Skip to content
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

Add allowemailregister option #317

Merged
merged 1 commit into from Jan 12, 2017
Merged

Add allowemailregister option #317

merged 1 commit into from Jan 12, 2017

Conversation

SISheogorath
Copy link
Contributor

A small contribution to solve some "paper cut issues" ;)

#297

@jackycute
Copy link
Member

@SISheogorath You are so nice!
Could you also add description to README.md?

@SISheogorath
Copy link
Contributor Author

SISheogorath commented Jan 12, 2017

Done. I have to verify everything works fine as we have no CI right now >.>

@jackycute
Copy link
Member

I know, we should write some tests #22

@jackycute
Copy link
Member

Thank you @SISheogorath !

@jackycute jackycute merged commit 4851098 into hackmdio:master Jan 12, 2017
@SISheogorath
Copy link
Contributor Author

Ehhh >.> did you test that it works? My last test ended up in a setting HMD_ALLOW_EMAIL_REGISTER and not showing the button.

@jackycute
Copy link
Member

You mean there might have so bugs?
I could revert it if needed, and you can patch it up.
I might need some time to test it.

@@ -132,6 +132,7 @@ if (process.env.HMD_LDAP_PROVIDERNAME) {
}
var imgur = process.env.HMD_IMGUR_CLIENTID || config.imgur || false;
var email = process.env.HMD_EMAIL ? (process.env.HMD_EMAIL === 'true') : !!config.email;
var allowemailregister = process.env.HMD_ALLOW_EMAIL_REGISTER ? (process.env.HMD_HMD_ALLOW_EMAIL_REGISTER === 'true') : !!config.allowemailregister;
Copy link
Member

@jackycute jackycute Jan 12, 2017

Choose a reason for hiding this comment

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

Well, here is the bug.
This should be:

var allowemailregister = process.env.HMD_ALLOW_EMAIL_REGISTER ? (process.env.HMD_ALLOW_EMAIL_REGISTER === 'true') : ((typeof config.allowemailregister === 'boolean') ? config.allowemailregister : true);

Or it will default to be false.

Copy link
Member

@jackycute jackycute Jan 12, 2017

Choose a reason for hiding this comment

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

And there is a typo HMD_HMD_ALLOW_EMAIL_REGISTER.

Copy link
Member

@jackycute jackycute Jan 12, 2017

Choose a reason for hiding this comment

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

Both of above have been fixed in 6be8752

@jackycute
Copy link
Member

jackycute commented Jan 12, 2017

I just made a patch, please help to test @SISheogorath.

@SISheogorath
Copy link
Contributor Author

Cool. I'll test it in a few seconds ;)

@SISheogorath
Copy link
Contributor Author

SISheogorath commented Jan 12, 2017

@jackycute Works great! Thanks!

(Tested with the lite image as the stable wasn't ready)

@SISheogorath SISheogorath deleted the master+allowEmailRegister branch January 12, 2017 16:38
@youngcraft
Copy link

how to use it in dockerfile version? I use docker-compose to build my Codimd. In Readme.md , there is not reference issues to talk about it.

pandy1988 pushed a commit to pandy1988/codimd that referenced this pull request Sep 13, 2019
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.

None yet

3 participants