Email validation enhancement

[msamgan]

The email:rfc,dns validation rule in Laravel ensures that the email address is both correctly formatted and belongs to an actual domain that is capable of receiving emails, resulting in more reliable, secure, and accurate email handling.

[msamgan]

@nunomaduro @MrPunyapal

[msamgan]

@MrPunyapal thanks for letting me know about free email faker. it will be a great help in future.

[CamKem]

I think that on some of the tests we are just testing the happy path. It might be a good idea to assert the count of error messages.

For example, the CreateTest there is multiple tests where 'terms' => true, should be in the payload, but it's not, so effectively we have a stray message in the bag we are not accounting for.

We could strengthen the tests, adding something this, ensuring the count of errors are expecting for the tests were we have failing messages.

    expect(collect(session('errors')->getBags())
        ->flatMap(fn ($bag) => $bag->all())
        ->count())->toBe(1);

Alternatively, you could use:

        ->assertValid(['name', 'username', 'password', 'password_confirmation', 'terms'])

However, if we miss adding a value to the array or later fields are added in code, a validation message could be thrown which is not picked up, or expected.

Not a huge deal & could be a seperate PR as it doesn't effect this, I just feel we need to ensure that we are making our tests more robust. With @MrPunyapal requested changes, everything else looks good.

[msamgan]

@MrPunyapal can you please approve this. @steven-fox whats your opinion on this ?

[steven-fox]

Only code adjustments we may need: the dns validation requires the 'intl' extension to be installed with PHP, so we may want to add that requirement to our composer file. The production server already has this installed, so no issue there, but including it in composer would be more explicit for other devs locally.

This seems OK and I suppose it would reduce a minimal amount of spam user registrations. So I don't see a big reason to not merge this;

It's just not suuuper important because we require email verification to actually sign in. If someone registers with a dud email, they won't be able to verify->login, so it's a mute point. And we already delete unverified users after 24 hours.

[msamgan]

Only code adjustments we may need: the dns validation requires the 'intl' extension to be installed with PHP, so we may want to add that requirement to our composer file. The production server already has this installed, so no issue there, but including it in composer would be more explicit for other devs locally.

This seems OK and I suppose it would reduce a minimal amount of spam user registrations. So I don't see a big reason to not merge this;

It's just not suuuper important because we require email verification to actually sign in. If someone registers with a dud email, they won't be able to verify->login, so it's a mute point. And we already delete unverified users after 24 hours.

As per the request I have updated the composer.json.

[msamgan]

where are we on this one ?

[MrPunyapal]

It has conflicts

What do you say about adding a description to it? 🤔

[msamgan]

It has conflicts

What do you say about adding a description to it? 🤔

conflict resolved.
And i guess everyone know what the PR is about but still if you want, i'll definitely add a description.

[MrPunyapal]

It's for devs who are gonna see this PR in future.

[msamgan]

It's for devs who are gonna see this PR in future.

Make sense. Thanks for the reminder.