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

Admin panel allow all emails option #41

Closed
wants to merge 2 commits into from

Conversation

jeremy-melnyk
Copy link
Contributor

@jeremy-melnyk jeremy-melnyk commented Oct 27, 2017

Issue #15
Closes #15

This PR is dependent on #40, since it builds on code in that PR.

Making another contribution for Hacktoberfest, which completes #15.

I added a new checkbox called 'Allow all emails' to the 'Additional Options' section on the admin settings page.

Toggling this will update a new boolean property called allowAllEmails in the server's Settings schema,
and update the settings client side. A 'put' endpoint was added to the server api.js to allow changes to the allowAllEmails setting.

Upon new user registration, the UserController validation that occurs server side now checks the settings for allowAllEmails flag after validating that the email is a valid email type. If allowAllEmails is set to true, then the whitelist will not be checked for valid educational emails.

If the allowAllEmails flag is set to false, then the whitelist validation proceeds as normal.

I did my best to remain consistent with the codebase, and added as little as possible to get this working. Please don't hesitate to let me know if you'd like this done differently. Thanks!

Example Screenshots:

Additional options section.
Allow all emails set to disallowed.

image

This is what it looks like when toggled to 'allowed'.

image

This is the confirmation that pops up after you toggle the checkbox.
image

Add allow/dissallow all emails button in the admin settings panel.
The canRegister function now checks the allowAllEmails flag
in the settings model when a user registers.
A false allowAllEmails flag continues to check the whitelisted emails
while a true allowAllEmails flag will allow the user to register with any
valid email.

Moved the email validator before the settings check.
@jlin816
Copy link
Contributor

jlin816 commented Oct 30, 2017

Awesome! Can we start a place to document all the new settings in the admin panel (either in the README.md, in the code, or both -- open to thoughts on which would be the most appropriate place)? e.g. it might not be immediately apparent that disabling Allow all emails checks for .edu emails.

@ehzhang
Copy link
Contributor

ehzhang commented Oct 30, 2017

Huh - I just realized that we had a change that we made for blueprint to allow for all emails that didn't get propagated up to the main repo. There, we added a special case for the '*' selector in whitelisted emails, and we can accomplish all of this behavior server-side.

UX-wise, I don't like the idea of having both the toggle and whitelisted emails available as an option, since it can become unclear as to which option has precedence.

@jeremy-melnyk
Copy link
Contributor Author

@ehzhang
In light of the server side '*' selector, would you scrap the client side option? I don't mind either way.

If would like to keep the 'allow all emails' toggle, I can hide/disable the whitelist section once it's toggled, to indicate that they are mutually exclusive.

Let me know!

@jlin816
If the option is to remain, then I will update the PR to add documentation concerning the toggle and the whitelist.

@jlin816
Copy link
Contributor

jlin816 commented Nov 14, 2017

Hey Jeremy -- sorry for the delay!
What are your thoughts on labeling the toggle on both sides ("Allow all emails" vs. "Allow whitelisted emails only"). If the "whitelisted" is toggled, it seems to make sense that the whitelist email text box, which was previously a separate section from "Additional Options," instead folds open within the section, right below the toggle.

Let me know whether this is an intuitive UI!

@jeremy-melnyk
Copy link
Contributor Author

@jlin816

Sounds great! I think that does make it more clear.
I'll see if I can update the PR within the next week.

I will update the documentation to reflect this new setting as well.
Thanks for the feedback!

@jlin816
Copy link
Contributor

jlin816 commented Dec 31, 2017

@Jeremy091 Bumping this! Are you still planning to finish up this PR?

@lgg
Copy link

lgg commented Mar 12, 2018

@Jeremy091 any updates with this PR?

@jeremy-melnyk
Copy link
Contributor Author

Revisiting old PRs I had open. Closing since it is very stale.

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.

Add more customizable options in admin panel
5 participants