MBS-11507: Remove useless resend-verification code

[reosarevok]

Fix MBS-11507

The whole resend-verification code was added in 2011 (MBS-3188) apparently under the incorrect assumption that we store the email a user has entered before it gets verified. In fact, we only store it as part of the verification process - so there's no possible case where we would have an email stored that still needs verifying.
The entirety of the resend-verification code is as such completely useless, since there's no reason to re-verify an already verified email address (even though that's what the Reverify test was testing).

[yvanzo]

Two cases that needs to be checked:

• What happens when a user changes their email address?
• What happens when an admin changes user’s email address?

[reosarevok]

What happens when a user changes their email address?

I'm fairly sure the old address is kept until they verify the new one, unless they have just removed their email completely:

musicbrainz-server/lib/MusicBrainz/Server/Controller/Account.pm

Lines 394 to 401 in 6d61d02

	if ($old_email ne $new_email) {
	if ($new_email) {
	$self->_send_confirmation_email($c, $editor, $new_email);
	$verification_sent = 1;
	} else {
	$c->model('Editor')->update_email($editor, undef);
	}
	}

.

What happens when an admin changes user’s email address?

Similarly, old address is kept until they verify, unless the admin explicitly skips the verification process (which sets email_confirm_date)

musicbrainz-server/lib/MusicBrainz/Server/Controller/Admin.pm

Lines 59 to 75 in 6d61d02

	if ($old_email ne $new_email) {
	if ($new_email) {
	if ($form->field('skip_verification')->value) {
	$c->model('Editor')->update_email($user, $new_email);
	$user->email($new_email);
	$c->forward('/discourse/sync_sso', [$user]);
	} else {
	$c->controller('Account')->_send_confirmation_email($c, $user, $new_email);
	$args{email} = $new_email;
	}
	}
	else {
	$c->model('Editor')->update_email($user, undef);
	$user->email('editor-' . $user->id . '@musicbrainz.invalid');
	$c->forward('/discourse/sync_sso', [$user]);
	}
	}

[mwiencek]

Something we should think about before removing this is if we should just start storing the emails before they're verified. I don't see any reason not to.

[reosarevok]

What's the benefit of storing the unverified email? I'm not against it per se, just not sure if there's enough of a use case to actually make the effort to change it.

[mwiencek]

What's the benefit of storing the unverified email? I'm not against it per se, just not sure if there's enough of a use case to actually make the effort to change it.

Well, we could allow them to re-request a verification email without having to re-enter the email. :) (Which may only be useful if our email-sending actually broke, or they manage their own spam filters, but.)

I think it's also slightly confusing how entering a new email doesn't update it right away. If you reload the form it's either still empty or still using the old email until you re-verify.

I imagine it'd also improve support if someone didn't get the verification mail, since you can verify they entered it correctly? I mean, you can already enter it yourself and skip verification, but it's a useful bit of information to know where the problem was.

[reosarevok]

I guess I can see that. Dunno how hard it would be to change the logic - probably not a lot, if everything stays the same except for saving the email?

My only worry is that if someone already has a verified email, try to change it and don't get the verification email for some reason, right now they can still edit with the old email. With this they could not, AFAICT.

[yvanzo]

To summarize, if I understood correctly:

• The bug report rightfully states that "Resend verification" code is useless.
• The current patch removes this code.
• The above conversation suggests to fix it and make it useful instead of removing it.

Therefore, what are the next steps?
Still reviewing and merging this pull request, or closing it and reopening the ticket?

[mwiencek]

Since @reosarevok deals with the support mails regarding this I'll defer to him on what the best course of action is. The code is currently useless, so I don't see much issue in removing it for now -- if we decide to fix it later, it's still in the git history.

[reosarevok]

I still think it's fine to keep as it is, but I'm also used to it :)

We could specify that verification expires and how to request a new verification email from the verification email itself (MBS-12476) and it'd probably be sensible to also include how to request a new one in Controller::Account where we currently just say the somewhat infuriating "Sorry, this email verification link has expired"