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

Send confirmation email for registration #249

Open
wants to merge 11 commits into
base: devel
Choose a base branch
from

Conversation

guilhermesiqueira
Copy link

@guilhermesiqueira guilhermesiqueira commented Oct 24, 2019

Proposed Changes

Upon registration, the system sends an email to the user to confirm their registration.

Type of Change

What kind of changes this Pull Request introduces to Falko?

  • Bugfix
  • New Feature

Checklist

  • Pull Request's name is self-explanatory and its in English.
  • There are no Rubocop issues.
  • Travis build has success (tests, code climate and codacy checks).
  • Pull Request is linked to an existing issue.
  • Seeds have been created to the new User Stories.

Other Comments

When creating a new version of the user address, email confirmation is required, so a new version has been created as V1/users. The versioning of the whole API is something that is beyond the scope of this issue, so we opted for the above.

Close #214

@alaxalves
Copy link
Collaborator

Is this a WIP PR? Because there's still some tests breaking in the CI tool.

@guilhermesiqueira
Copy link
Author

Is this a WIP PR? Because there's still some tests breaking in the CI tool.

Now the PR is complete. Now there is the versioning of endpoint /users, where in v1/users email confirmation is required to register new users.

Copy link
Collaborator

@alaxalves alaxalves left a comment

Choose a reason for hiding this comment

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

In general, this look ok to me, I just didn't get why there are so many templates that apparently do the same thing. Might be just a technicality tho ¯_(ツ)_/¯

@guilhermesiqueira
Copy link
Author

In general, this look ok to me, I just didn't get why there are so many templates that apparently do the same thing. Might be just a technicality tho ¯_(ツ)_/¯

For some reason when I delete templates from the user_mailer folder I get a missing template error. The same happens when I remove templates from the layouts folder. Keeping the two template folders all works beautifully ¯_(ツ)_/¯
Captura de Tela 2019-10-24 às 22 43 42

By the way, sending emails is using Sendgrid. It would be interesting for a project maintainer to create a Sendgrid account and add the key to the environment.rb file on deploy.

alaxalves
alaxalves previously approved these changes Oct 25, 2019
@guilhermesiqueira
Copy link
Author

Hey @alaxalves, I added some exceptions, could you review again?

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.

Enviar email de confirmação para cadastro
2 participants