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 email confirmation #5301

Merged
merged 32 commits into from
Aug 31, 2023
Merged

Add email confirmation #5301

merged 32 commits into from
Aug 31, 2023

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Aug 28, 2023

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.

src/config/backpack/base.php Outdated Show resolved Hide resolved
Copy link
Member

@tabacitu tabacitu left a 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.

src/resources/lang/en/base.php Outdated Show resolved Hide resolved
src/resources/lang/en/base.php Outdated Show resolved Hide resolved
src/BackpackServiceProvider.php Outdated Show resolved Hide resolved
src/app/Http/Middleware/EnsureEmailVerification.php Outdated Show resolved Hide resolved
@tabacitu tabacitu removed their assignment Aug 29, 2023
@tabacitu
Copy link
Member

/gpt can you summarize the issue?

@github-actions
Copy link

@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 BackpackServiceProvider to register the new middleware, updating the RegisterController to handle email verification, and adding a new VerifyEmailController to handle the verification process. There are discussions about whether this should be a responsibility of the main application or the package itself.

@tabacitu
Copy link
Member

In a freshly set up demo, I've followed the steps in the docs, then registered a user. After registration, I've been sent to this page:
CleanShot 2023-08-30 at 12 46 54

Note that:

(1) I am using the proper branches for all themes:
CleanShot 2023-08-30 at 12 48 37

(2) The error page has the admin panel menu 😱 So I think it has already logged me in - but I haven't verified the email yet. Shouldn't we login only AFTER verification?

(3) Granted, what I did wrong was that I messed up a middleware name - I added it as verify instead of verified. If I hadn't done that, I probably wouldn't have seen this error page difference, so I wouldn't have a sneak peek of the menu without actually having the email address verified. But even if I trigger other errors while in this status (registered but not verified) I see the menu. For example if I trigger the email verification too many attempts. So I think the safest is to completely log in the user upon verification. NOT upon registration.

CleanShot 2023-08-30 at 12 57 41

@tabacitu
Copy link
Member

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.

@pxpm
Copy link
Contributor Author

pxpm commented Aug 30, 2023

It makes sense. I've updted the PR's to account for that

pxpm and others added 3 commits August 30, 2023 18:31
…ackpack/CRUD into add-email-confirmation

# Conflicts:
#	src/app/Http/Controllers/Auth/VerifyEmailController.php
[ci skip] [skip ci]
src/app/Library/Auth/AuthenticatesUsers.php Outdated Show resolved Hide resolved
src/app/Http/Controllers/Auth/VerifyEmailController.php Outdated Show resolved Hide resolved
src/app/Http/Requests/EmailVerificationRequest.php Outdated Show resolved Hide resolved
src/config/backpack/base.php Outdated Show resolved Hide resolved
Copy link
Member

@tabacitu tabacitu left a 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.

src/app/Http/Controllers/Auth/VerifyEmailController.php Outdated Show resolved Hide resolved
src/app/Http/Controllers/Auth/VerifyEmailController.php Outdated Show resolved Hide resolved
src/app/Http/Controllers/Auth/VerifyEmailController.php Outdated Show resolved Hide resolved
src/app/Http/Controllers/Auth/VerifyEmailController.php Outdated Show resolved Hide resolved
src/app/Library/Auth/AuthenticatesUsers.php Outdated Show resolved Hide resolved
src/config/backpack/base.php Outdated Show resolved Hide resolved
src/config/backpack/base.php Outdated Show resolved Hide resolved
src/config/backpack/base.php Outdated Show resolved Hide resolved
src/BackpackServiceProvider.php Outdated Show resolved Hide resolved
Copy link
Member

@tabacitu tabacitu left a 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! 🎉🎉🎉

@tabacitu tabacitu merged commit 4cd8e5d into main Aug 31, 2023
3 checks passed
@tabacitu tabacitu deleted the add-email-confirmation branch August 31, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants