-
Notifications
You must be signed in to change notification settings - Fork 902
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 email confirmation #5301
Add email confirmation #5301
Conversation
[ci skip] [skip ci]
[ci skip] [skip ci]
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 love it! Well done @pxpm ! I've gone through all of the code thoroughly, but haven't tested this yet. I only have one concern - see in the inline comments.
When that is resolved, I think it's ready for testing and launch.
/gpt can you summarize the issue? |
@tabacitu The issue is a pull request that adds email confirmation functionality to the Laravel Backpack CRUD package. The pull request aims to make it easier to implement email verification for registered users by adding necessary middleware and controllers. The proposed changes include updating the |
Co-authored-by: Cristian Tăbăcitu <[email protected]>
Co-authored-by: Cristian Tăbăcitu <[email protected]>
…ackpack/CRUD into add-email-confirmation
Heads-up, I added a PR to demo too - Laravel-Backpack/demo#571 - since I already had the code on my machine, following your docs. |
[ci skip] [skip ci]
It makes sense. I've updted the PR's to account for that |
…ackpack/CRUD into add-email-confirmation # Conflicts: # src/app/Http/Controllers/Auth/VerifyEmailController.php
[ci skip] [skip ci]
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've given this a good hard look, how we can clean it up. Let me know.
Co-authored-by: Cristian Tăbăcitu <[email protected]>
[ci skip] [skip ci]
Co-authored-by: Cristian Tăbăcitu <[email protected]>
…ackpack/CRUD into add-email-confirmation
[ci skip] [skip ci]
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've given this another look. And another test. Looks & works good to me! WE HAVE IT. Let's do it, let's merge! 🎉🎉🎉
WHY
BEFORE - What was wrong? What was happening before this PR?
Long requested feature in Backpack Laravel-Backpack/community-forum#53
It's finally happening 🎉 🥳 🎈
It was possible, but difficult to implement email verification for registered users.
AFTER - What is happening after this PR?
It's dead simple as changing a config value, and all your admin routes will now require the verified email. 👍
HOW
How did you achieve that, in technical terms?
I've followed Laravel documentation and adapted to our use-case.
Is it a breaking change?
I don't think so,no. It's behind a config that's false by default.
How can we test the before & after?
Find the docs here: Laravel-Backpack/docs#503
This PR requires:
Laravel-Backpack/theme-tabler#142
Laravel-Backpack/theme-coreuiv2#32
Laravel-Backpack/theme-coreuiv4#48
NOTE TO SELF: update composer.json with CRUD version in those PR's before merging.