Sites are likely to need subscription history in case of disputes about whether a user was legitimately subscribed with their consent. In some countries this is required by law, including GDPR in EU.
This module already tracks some history for each subscription: when it was last changed and the "source" (which roughly indicates who/how it was changed). However this information is not really adequate for the purpose:
Subscriber::postSave(), by comparing with the ->original. The "source" can be calculated based on the current route.Subscriber::isUnsubscribed() status can be calculated with an SQL query on the new table.Subscriber::subscribe() and unsubscribe() to take just a single $newsletter_id parameter.SubscriptionManager->[un]subscribe() become a simple wrapper to the subscriber functions that should only be used in cases where the code doesn't have a Subscriber.Advantages:
isUnsubscribed() status can be calculated with an entity query on past revisions.SubscriptionManager->[un]subscribe() become a simple wrapper to saveNewRevision() that should only be used in cases where the code doesn't have a Subscriber. The signature is almost correct - change $source to be $log_message. They must not be called more than once for the same subscriber because that leads to unnecessary revisions.SubscriberInterface::saveNewRevision() (see below). Change subscribe() and unsubscribe() to take just a single $newsletter_id parameter. /**
* Saves the subscriber tracking a new revision.
*
* @param string $log_message
* (optional) Log message for the new revision. Note that the URI, time and
* uid are already saved, so only set a message for something extra.
* @param bool $save
* (optional) Whether to save the subscriber. Pass FALSE if the calling
* code will save, for example in form code.
*/
public function saveNewRevision(string $log_message = NULL, bool $save = TRUE);
Disadvantages: quite complex and likely to increase database usage significantly.
Create a new SubscriptionItem for every subscription and unsubscription so it keeps expanding. The outdated ones can have different status values like SIMPLENEWS_SUBSCRIPTION_STATUS_PREVIOUSLY_SUBSCRIBED.
This is the cheapest fix, however it's a bit hacky and unintuitive.
Integrate with an existing consent module, in particular gdpr_consent: there is currently a sandbox project.
Comments
Comment #2
adamps commentedComment #3
adamps commentedComment #4
adamps commentedComment #5
PhilippVerpoortI think the solution outlined in the IS sounds good. A few comments:
confirmed(orunconfirmedif you prefer) would be sufficient. Making use of workflow states from the Workflow module would in my view add too many dependencies and unnecessary complexity.confirmedrevision (e.g. through a query using latestRevision()), and check if an entry in thesubscriptionsfield exists for that newsletter. When the subscription gets modified by the user or the admin, we should always load the latest revision, no matter if that one isconfirmedor not.confirmed.confirmedand the existing entity-reference field calledsubscriptionsare obvious candidates. What about themail,uid, andlangcodefields? This brings me back to something that I currently do not understand about this module: if a user changes their email address, do we update the existing subscriber entity or do we create a new one? All other fields added through the UI should be revisionable I guess, partly to resolve #3152102: Anonymous user can alter fields of any subscriber.statusfield on the subscriber entity can definitely be removed. We can replace the current behaviour in the 'mass subscribe' admin interface, which is dependent on that field, by something more powerful using the revision history: we can give admins the option to mass subscribe (1) everybody, (2) everybody except existing subscribers who never subscribed to this newsletter type, or (3) everybody except those who have previously opted out from this newsletter type.gdpr_consent? This is confusing because the sandbox project mentioned in the IS is a 8.x module but depends on this 7.x module... I guess we have to wait a little for this to make progress.Comment #6
adamps commentedGood questions. I will reply briefly now because this issue needs to wait for #3066314: Subscriber updates should be confirmed atomically anyway. We don't want to make this issue try to fix everything. This issue probably should not add any config settings or change the module behavior. I think this issue is about changing the data model only.
3. I believe you are anticipating #3037307: Improve experience for anonymous subscribers to modify existing subscriptions which should be a separate issue.
4. Clean-up on unconfirmed already exists, so it should continue to exist after this issue. The existing UI seems fine.
5. Tracking IP address can be a separate feature.
6. If a subscription corresponds to a user then the subscriber mail field changes automatically to match the value in the user entity. An anonymous subscriber cannot change their own email address so would have to create a new one. An admin can change it.
7. The status field is used for the subscription corresponding to a blocked user. It could also used by an admin to disable a subscription without deleting it. This feature should not try to give new options on mass subscribe/unsubscribe.
8. An "unsubscribed" field (= never pester me again) would be a separate issue.
9. I suggest that "Right to be forgotten" should be separate.
10. Seems like separate.
Comment #7
PhilippVerpoortYou are absolutely right, most of the points I made above are probably separate issues.
Nonetheless, I think it make sense to think about these now as we're designing the new data model. I think it'd be good to get this right now so we don't end up having to make further changes to the data model later on as we attempt to address other issues. It won't be possible to foresee everything, but let's not blindly make changes here and then reverse them all in the next issue.
In particular (with regards to point 3 from above), I'm wondering if there's any point in updating the confirmation controller in this issue if we are perhaps considering to remove it entirely in a later issue (namely #3066314: Subscriber updates should be confirmed atomically). As we apply the required changes for the data model (make entity revisionable, add
confirmedfield, changesubscriptionsfield, etc), we'll have to change the way that confirmations are made. This will result in quite a bit of rewriting of the confirmation form and corresponding controller. Is it worth making all these changes only to end up removing the whole thing in the next issue?Comment #8
adamps commentedI think the order has to be the other way round - #3066314: Subscriber updates should be confirmed atomically first then this one. The idea of this issue is to have one revision per update to a subscriber. That's not really possible until the updates are confirmed atomically. So let's solve the other issue first and then we can come back and discuss this one in detail.
I still believe that this issue should not change the way that confirmations are made - that would be #3037307: Improve experience for anonymous subscribers to modify existing subscriptions.
I don't see that we will remove the ConfirmationController or even expect that it will change very much. We will still use
confirmCombined()for a new subscriber using the form andconfirmSubscription()when someone clicks an unsubscribe link in a newsletter.Comment #9
PhilippVerpoortOh no, sorry... I referenced the wrong issue. This caused confusion, my apologies.
This one has to come first of course, you're absolutely right about that: #3066314: Subscriber updates should be confirmed atomically
It is the following issue which I believe would be easier to resolve alongside this one: #3037307: Improve experience for anonymous subscribers to modify existing subscriptions
My point is this: If we move away from keeping track of requested "changes", and move towards the revision API, we can essentially remove the entire "confirmation" machinery from the code. All you need is one entity form that updates the subscriber entity, and either creates revisions that have the
confirmedset toTRUEorFALSE, depending on the authentication status of the user. You don't need anything else, such as confirmation forms/controllers or specific code that handles the confirmation.But I guess I should just get started with all the changes required, and then we can have another chat.
Comment #10
adamps commentedAh yes that makes much more sense. But I don't entirely understand 'resolving alongside' - Drupal.org requires each patch to work on its own. Perhaps even to resolve the other issue before this one?
I'm not sure let's wait and discuss that later. (I'm still expecting confirmations as per the last para of #8.)
Comment #11
jonathanshawIt may be worth doing some performance tests here, as revisions may have an impact of sending an issue to 100,000 subscribers.
Comment #12
PhilippVerpoort@jonathanshaw: why do you think so? During sending it should really just add one modifier to the query, which isn't going to take much longer. I believe the bottle neck in performance on the web server side is probably with building all those HTML emails — which again is probably dwarfed by the performance of the actual mail server...
Or maybe there's something I misunderstood?
Comment #13
adamps commentedI agree with Philipp. We will create a revision every time a subscriber is edited. It won't happen when sending a newsletter as far as I can see.
Comment #14
adamps commentedComing back to the discussing started in #5, and in particular point 3:
After #3037307: Improve experience for anonymous subscribers to modify existing subscriptions I believe it will work like this:
The way you describe is correct for the new path that will be added: an anonymous validating via email then modifying their subscription.
However both methods of ConfirmationController are still needed for other cases that need to work exactly the same way as now.
confirmCombined()could be renamed toconfirmSubscribe(). It is used for an anonymous user submitting the subscription block, then clicking the link in the email.confirmSubscription()could be renamed toconfirmUnsubscribe(). It is used when someone clicks an unsubscribe link in an email.So I feel we can safely work on this issue ahead of #3037307. I agree they both depend on each other a little but I don't think we'll throw away much.
Comment #15
adamps commentedComment #16
adamps commentedComment #17
jonathanshawFrom the IS:
As discussed in #3271864-2: Test explicit tracking of unsubscribing this is not in fact true; this flag does have useful data. Furthermore, that data can be extremely valuable in situations where you want to subscribe people automatically but without overriding their previous opt-outs.
I suggest we proceed with the IS solution but do not implement point 3 of the IS:
Timestamp and source can be removed, but we keep status on SubscriptionItem. I'm not aware that keeping SubscriptionItem causes any problems for moving to revisions in the way the IS proposes. In fact, this should shrink the issue scope in a good way.
Comment #18
adamps commented@jonathanshaw Thanks for the testing and correction.
I still have some concerns about the unsubscribe flag.
1) Accuracy / reliability
This module offers the visitor a choice of newsletters on various pages: subscribe block, user register page, user newsletter settings page. In each case there are a series of checkboxes one for each newsletter. In the first two cases, some boxes might be checked by default.
Suppose that the visitor carefully reviews all the options and adjusts the values. We could reasonably interpret that they have made a conscious choice that they don't want the newsletters that were unchecked. However I suspect that the unsubscribe setting would not be recorded, even in the case where the visitor unchecked a box that was checked by default.
2) Complexity.
I feel the subscriber settings are rather complex, making it harder for the admin to understand and harder for developers to maintain.
It would be interesting to know what the major newsletter providers do. Another possible "mass subscribe" strategy would be to exclude existing subscribers by default. However if there was an entirely new newsletter then it might be reasonable to include them. For privacy legislation concerns, a subscriber-wide unsubscribed flag might be suitable. Probably the "right to be deleted" people should never be added again in any circumstances.
I agree that we can achieve the goal of this issue keeping the concept of unsubscribed.
Well in terms of development time, I don't know - I was attracted to the simplicity of removing SubscriptionItem. Otherwise we would have to keep it, but remove two of the columns and remove one of the states (unconfirmed) of the column we're keeping.
Comment #19
adamps commentedComment #4 from #3271864: Test explicit tracking of unsubscribing:
I think this is the separate issue😃. See IS, especially the heading SubscriptionManagerInterface.
Comment #20
adamps commentedI feel I still have a preference still for simplification although I'm not set against keeping the unsubscribe concept. If we do keep it, maybe we ought to improve how it works.
What does anyone else think?
Comment #21
jonathanshawI think you make some great points in #18. I find that the way it works currently - distinguishing between explicit unsubscribes and mere "not sets" - suits my use case perfectly. Part of my reason for using simplenews is that Mailchimp are horribly draconian with their subscriber-wide unsubscribed flag, making it hard to provide a good experience for returning students.
But I can see that supporting this adds complexity, and even worse is that the storage model, API and expectations are somewhat opaque. If we were doing this from scratch probably using a workflow field from core or state_machine would be the way to go; it would give us rich, explicit, extensible API and storage for the subscription state.
Another possibility would be to recogise that what we really care about is the history of explicit actions by the subscriber, and this is potentially recreatable from the subscribers revision history. Maybe the subscription item status does not hold any information we don't have anyway. For example, we could have a method Subscriber::hasOptedOut($newsletter) that iterates over the revisions to find the one where the newsletter subscription was last removed and sees if the subscriber was the author of that revision; hypothetically one could imagine Subscriber::hasEverBeenSubscribed() that sees if there was ever a time when the person was subscribed. Frankly this feels pretty hairy to me, but maybe it's ok.
Comment #22
adamps commentedYes that makes sense. Each site might choose a different policy to decide whether to bulk subscribe existing users.
It's potentially a bit academic anyway as we don't currently have anyone wishing to develop this issue. My inclination is not to commit #3271864: Test explicit tracking of unsubscribing right now. However if we ever fix this issue we should make sure there is a comparable test and we can likely use similar code.
Anyway thank you for the insight and correction of #18 which helps keep this issue pointing in the right direction.
Comment #23
adamps commentedI thought some more and actually I would happily commit #3271864: Test explicit tracking of unsubscribing if we can write the patch in a way that is independent of the internal implementation.
I.e. subscribe, receive a mail, click unsubscribe link, do bulk subscribe, check not subscribed.
Comment #24
adamps commentedComment #25
adamps commentedComment #26
adamps commentedComment #27
adamps commentedComment #28
jonathanshawThinking about your options:
maybe the storage choice doesn't matter too much, because you can wrap any storage solution even a "hacky" one in a pretty API easily enough.
Maybe the hard thing is usually the UI. In this case, I'm thinking about how this manifests in a subscription form and how easy that is to customise. In this case though I can't see a lot to choose between the options. We will need a custom widget or equivalent regardless.
I was tempted by option C, but the WTF is going to be pretty nasty.
Option A seems pretty sane. I'm assuming you mean not just a new database table, but a new simple SimplenewsSubscriptionEvent entity type. And then for convenience you'd reference the active subscriptions from the subscriptions entity reference field on the subscriber.
Comment #29
adamps commentedThe subscription history is complex, allowing for multiple data sets per newsletter including value, date, uid, source, etc. For this part then yes I mostly agree with your remark, although the data will be accessed by views in addition to through the API.
The current subscription status is on/off per newsletter. Although we have an API (
SubscriberInterface::subscribe()andSubscriptionManagerInterface::subscribe()), we want it to act like a field, so it can't be hidden/abstracted. It's accessed through the entity field interface, through entity queries, through direct SQL queries, and through views. Some of these are performance critical for sending newsletters. The fact that the current code overloads the field to store partial history requires various hacks elsewhere in the code to then try and hide this history.My proposal is to make the subscriber field back into a normal EntityReferenceItem. We can then remove the above hacks. I am aware that this duplicates the most recent history item, however I feel it's worth it:
Actually I had been imagining a table, but you are probably right to have an entity type.
Comment #30
adamps commentedComment #31
adamps commentedHere's a work in progress patch that removes a lot of code. It doesn't yet try to add the new subscription history entity.
Comment #33
adamps commentedComment #35
adamps commentedComment #36
adamps commentedComment #38
adamps commentedComment #40
adamps commentedComment #42
adamps commentedComment #44
adamps commentedComment #46
adamps commentedComment #48
adamps commentedComment #49
adamps commentedPart 1 done - removed the old history tracking. Some tests therefore fail, and are now commented out with @@@@
Part 2 next is to add new history tracking, and uncomment the tests
Comment #50
adamps commentedUpdate hook is unfortunately not working due to #937442: Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed)
Comment #51
adamps commentedFirst go at part 2, missing most of the update hooks
Comment #53
adamps commentedComment #55
adamps commentedComment #57
adamps commentedComment #59
adamps commentedComment #60
adamps commentedComment #61
adamps commentedComment #62
adamps commentedComment #64
adamps commentedComment #66
adamps commentedComment #67
adamps commented