Conversation
|
@NNN @marla-singer @frenchbread One of you, please volunteer to review by assigning yourself |
|
@jykae When I started application at first time and created admin user, I have a message about e-mail verification although the SMTP settings didn't exist. |
|
@jykae After creating new user, it redirects to home page and user is already logined. Also user, who doesn't verify e-mail, can go to Catalogue, add new api and do some manipulations with this api. But user don't have permission to add documentation or upload api logo. When he/she is clicking on button, nothing happens. It's just only console error, which can't be seen by typical users. And it looks very strange for them. |
|
This is what we talked today in daily. I wanted to get initial review how this looks like. In daily I asked about how the user flow should go, as I didn't have clear understanding how we want it to be. And first user is one special case, that I did not even thought yet, we should allow first admin user to access platform and make settings. Thanks for review! |
|
The ideas to improve an e-mail verification:
|
|
@brylie requested changes done, ready for review |
| const branding = Branding.findOne(); | ||
|
|
||
| // Check branding exists | ||
| if( branding ) { |
There was a problem hiding this comment.
We need to check if (branding && branding.siteTitle), since site title is optional.
| // Check settings exists | ||
| if( settings && settings.mail && settings.mail.fromEmail ) { | ||
| // Return fromEmail | ||
| return '<'+settings.mail.fromEmail+'>'; |
There was a problem hiding this comment.
Move these < and > out of this function. Follow the single responsible principle here. In effect this function should only get the email, not get the email and transform it.
There was a problem hiding this comment.
Each variable, each line of code, each function, each class, each project should Do One Thing.
settings/collection/schema.js
Outdated
| return 'required'; | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Make sure you are using ESLint while developing. We have an ESLint rule that requires trailing commas now, based on the Airbnb rules.
settings/collection/schema.js
Outdated
| return 'required'; | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Make sure you are using ESLint while developing. We have an ESLint rule that requires trailing commas now, based on the Airbnb rules.
|
@brylie requested trailing commas added automagically :) |
|
@brylie Fixed conflict with eslint |
|
@brylie Do you agree my lintings and latest fixes? |
5 participants
Closes #1123
Closes #1514
Proposed changes
Other
Testing