Prevent duplicate subscribers

I would very much like to see this! Duplicate subscribers cause a real issue, especially when you consider GDPR/other privacy initiatives as people unsubscribe, but the duplicate is left active.

The alternative to making the database use a unique constraint is to add a field constraint, as User does in core. That way any form that attempts to change email addresses will hit that as part of entity validation and show an error.

@andrewbelcher yes this is a good discussion to have.

The alternative to...

I don't see them as alternatives - we can and maybe should do both. Look at the issue summary I have identified 3 steps. Step 3 is about user experience, step 2 is about enforcement/avoidance of bugs.

add a field constraint

Yes, but it's more complex than the user mail constraint.

• The scenario/constraint spans two entity types: user and subscriber.
• We might need to be more flexible. Suppose that I create a user account with email address A and subscribe. Later I have a new email address B and subscribe without making a user account. Then I want to change my user account to address B. It's an entirely reasonable thing to do, but a field constraint might block it. The user can't actually delete their anonymous subscription - even if they unsubscribe, the subscription entity remains.

Scenario 1

We can add a constraint on the subscriber:

    $fields['mail'] = BaseFieldDefinition::create('email')
      ->addConstraint('UniqueField', [])

This blocks the scenario giving a clear error message. It seems reasonable to enforce that the admin cannot make 2 anonymous subscribers with the same email.

Scenario 2

We could add a constraint on the user by means of hook_entity_base_field_info_alter(). It would presumably need to be a new custom validator but fairly easy to write copying from UniqueFieldValueValidator but looking into the subscriber entity type rather than the current entity type.

However as mentioned in #5 this is not necessarily an acceptable restriction. Perhaps we could try to make the constraint ignore unsubscribed subscribers?? Then print a message telling the user they can unsubscribe their other account then try again?

Scenario 2

I propose that when editing a user email address, if there is an existing subscriber then that subscriber should be deleted. Then at least we avoid corrupted a database with duplicates.

As a separate matter, it is obviously desirable when using Simplenews to verify user change of email. There is a core patch #85494: Use email verification when changing user email addresses or a module email_confirm.

Re #7

Scenario 2
We could add a constraint on the user

It seems unacceptable to block the user from changing their email in this way.

Re #10:

I propose that when editing a user email address, if there is an existing subscriber then that subscriber should be deleted.

+1. I can't see a better solution.

The fix here gets tangled up with #3049356: Anon user can alter any anonymous subscription. However that one is blocked on the complex fix #3035367: Track history of subscribe/unsubscribe and proof of consent. Many issues are tangled up in each other and we need to be practical or nothing will get committed. So I've deleted some pretty broken code about setting the subscriber status, with a @todo to replace it with correct code as part of the other issue.

1. +++ b/simplenews.module
   @@ -434,52 +434,46 @@ function simplenews_user_create(UserInterface $account) {
   +  // new account is created with unconfirmed subscriptions. Confirm these when
   

   AFAICS the subscriber status will always be TRUE because simplenews_user_update always calls $subscriber->fillFromAccount().

2. +++ b/simplenews.module
   @@ -434,52 +434,46 @@ function simplenews_user_create(UserInterface $account) {
   +  $subscriber = Subscriber::loadByUid($account->id());
   +  $new_subscriber = Subscriber::loadByMail($account->getEmail());
   +
   +  if ($subscriber) {
   +    if ($new_subscriber && ($new_subscriber->id() != $subscriber->id())) {
   

   It might be worth a comment that what we're really concerned with here is the possibility that the email has changed.

3. +++ b/src/Entity/Subscriber.php
   @@ -305,6 +305,19 @@ class Subscriber extends ContentEntityBase implements SubscriberInterface {
   +  public function preSave(EntityStorageInterface $storage) {
   +    parent::preSave($storage);
   +
   +    // If there is not already a linked user, fill from an account with
   +    // matching uid or email.
   

   As I remember, this will solve an unrelated need identified in my project. Yay.

Yay finally! The syncing code is extremely delicate - hopefully won't have to change it again for a long while. We still need new tests for the scenarios that we are fixing.

Re #27 thanks for the review

AFAICS the subscriber status will always be TRUE because simplenews_user_update always calls $subscriber->fillFromAccount().

You are correct. However the comment will be correct after the @todo has been completed and #3049356: Anon user can alter any anonymous subscription has been fixed. Right now the state of that function is rather arbitrary, but I thought I might as well leave the function as close to the final correct code as I could.

It might be worth a comment that what we're really concerned with here is the possibility that the email has changed.

Thanks I added several comments.

Plan to commit after adding tests.

New patch with tests.

The code moved from SubscriptionsFormBase to SubscriptionsBlockForm was necessary to allow the second bullet in this test to work. It's OK to move that code because since #3037307: Improve experience for anonymous subscribers to modify existing subscriptions, that's the only form that allows anonymous users.

    // - Create a registered user C with no subscriptions.
    // - Admin changes email of subscriber A to C.
    // - Should link subscriptions of A to C.
    $user_c = $this->drupalCreateUser(['subscribe to newsletters']);
    $this->submitForm(['mail[0][value]' => $user_c->getEmail()], 'Save');
    $sub_c = Subscriber::loadByUid($user_c->id());
    $this->assertEquals($sub_a->id(), $sub_c->id());
    $this->assertEquals(['a'], $sub_c->getSubscribedNewsletterIds());
    $this->assertEquals(2, $this->countSubscribers());

@jonathanshaw Ready to commit if it looks good to you.

Great thanks - another stable release blocker fixed many thanks for your help