Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

3.5.30 Differences in newsletter subscriptions according to member deactivation method #8812

Closed
dubs10 opened this issue Nov 15, 2017 · 14 comments
Assignees
Labels
Milestone

Comments

@dubs10
Copy link

dubs10 commented Nov 15, 2017

Deactivating by date and deactivating by checkbox behave differently regarding newsletters, despite appearing like they would do the same thing on the back end.

When a member account is deactivated automatically by date, it still is included on newsletter sends. Deactivating via the checkbox instead removes the email address from all newsletters for as long as the box is checked.

How about this behaviour be made consistent, ideally by using the date range to trigger the checkbox being turned on, or else by copying the same temporary newsletter removal behaviour?

@leofeyer
Copy link
Member

Is the newsletter sent to the deactivated members?

@dubs10
Copy link
Author

dubs10 commented Nov 21, 2017

Yes, when the deactivation checkbox is off but the current date is past the tl_member->stop value.

@KaiserCh
Copy link

The reason it works when deactivating a member is found at the end of core/dca/tl_member.php. The toggleVisibility() hooks into the newsletter recipient table. As a result, a member that is not yet active but will be activated in the future (e.g. new employee) should also receive newsletters associated to that member.
Solution: Make a date check on tl_member for any NL recipient who is also a member.

@leofeyer leofeyer added this to the 3.5.32 milestone Nov 23, 2017
@leofeyer
Copy link
Member

a member that is not yet active but will be activated in the future (e.g. new employee) should also receive newsletters associated to that member

I don't think so. You are not allowed to send newsletters to anyone before they have completed the double opt-in process.

Make a date check on tl_member for any NL recipient who is also a member.

What exactly do you mean? Shouldn't the newsletter script check for disabled members upon sending instead?

@dubs10
Copy link
Author

dubs10 commented Jan 15, 2018

What exactly do you mean? Shouldn't the newsletter script check for disabled members upon sending instead?

This would work but would be inconsistent with the checkbox method of disabling an account. When the checkbox is ticked the corresponding email address in the newsletter gets deactivated (the "unpublished" icon). However, when an account runs past its stop date it will still appear as active in the newsletter recipient list. Filtering these out at send-time still wouldn't mean that the newsletter list always reflected the active members.

Perhaps some sort of daily cronjob task would be a better approach? You could check daily for expired accounts and then turn on the tickbox plus deactivate the corresponding newsletter entry.

@leofeyer
Copy link
Member

@contao/developers How do we solve this best?

@KaiserCh
Copy link

KaiserCh commented Jan 16, 2018

I don't think so. You are not allowed to send newsletters to anyone before they have completed the double opt-in process.

Does this apply to employees as well? How would you violate §7 UWG if you keep it company-internal? Anyway, in theory it is possible for a member to be temporarily suspended (via the date-fields), but still be a valid NL recipient later.

What exactly do you mean? Shouldn't the newsletter script check for disabled members upon sending instead?

The NL script should check for multiple states:

  • if the recipient is no member, just use tl_newsletter_recipients.active (like in the current code)
  • if the recipient is a member, you'll have to make additional checks if the entry in tl_member is enabled and not disabled via start/stop-fields

While a cronjob testing any member for start/stop would work, it might be fatal for large databases. The cronjob could use a mechanism similar to Newsletter::synchronize().
In the end, it might be much easier to add the necessary tests to Newsletter::send(), right in the $objRecipients - loop. Instead of putting stress on the DB, just use PHP to check for the member date fields and, in case the user is suspended, just continue();

@dubs10
Copy link
Author

dubs10 commented Jan 17, 2018

While a cronjob testing any member for start/stop would work, it might be fatal for large databases.

If tl_member->stop is indexed on the database, it would run as quickly as any other SQL query. Select the records where the stop date is equal or greater to today's date and update so that tl_member->disable is true and the joined tl_newsletter_recipients->active is false. This could possibly all be done just in SQL.
And remember this is just a once a day thing. I don't think it would be a burden for the system.

@KaiserCh
Copy link

The Start/Stop-fields are using minute-precision, so a daily job is not enough to do it right. Also, a problem that somehow still exists in Contao 4: The fields are varchar(10) NOT NULL, instead of "int NULL" (where NULL is like the current empty field). This will prevent any Integer-based search.
Start/Stop currently are no keys, and a large amount of keys massively slows down DB performance on write/update/delete.
I would not advise a pure SQL solution, as there might be some custom hooks involved. Think of 3rd party addons relying on the correct onsave-callbacks etc.

@leofeyer leofeyer modified the milestones: 3.5.32, 3.5.33 Jan 18, 2018
@asaage
Copy link

asaage commented Jan 19, 2018

Honestly i do not like the idea of not being able to send newsletters to members that are no longer active.
Newsletters are its own thing and recipients should be managed under the newsletter BE module or the subscribe/unsubsribe FE Modules.

@dubs10
Copy link
Author

dubs10 commented Jan 19, 2018

Asaage, point taken. A large part of this issue was just the inconsistency between how manual and date-based deactivations affect the newsletter subscriptions. However it is resolved, it would be best for the behaviour to be consistent.

Here is a suggestion that might keep everyone happy:
You run a date check to see if the stop date is equal (and only equal) to today, and if so you trigger two things to happen:

  1. the checkbox on the member record for "deactivated" is set to true
  2. any corresponding newsletter subscriptions are set to inactive, but not removed.
    This would allow manual reactivation or adding of a newsletter subscription that would not be blocked by the date checking. Worst case scenario: when passing through the stop date, users are required to reactivate their subscriptions.
    Alternatively you'd be looking at adding an option to choose whether a subscription is linked to the active state of the account or not - probably a little too much work, though it would be useful.

Although some people are using Newsletters as its own separate service, many Contao users are running member services and don't want expired accounts still receiving members-only newsletters by default for obvious reasons, so any solution needs to take this possibility into account.

Regarding KaiserCh's point about minute precision, the worst case scenario would just be that someone is removed from the list a day late, in the case of a "greater than now" comparison, but couldn't you run the date check daily as an equals comparison on the date half of the datetime string and remove this problem that way? (Or did you mean the date isnt a string but an int? - sorry I didnt check this before)

Point taken that in some situations it's going to tax the database quite heavily, but is checking at send-time better? Those comparisons are going to be need to be done sooner or later to get the desired result, so why not just do them on the expiry day? Doing it at send time means you may have people on your subscription list who appear to be subscribed but will actually get filtered out. You wouldn't be able to export a current list of subscribers without first cross-referencing with the member database.

@asaage
Copy link

asaage commented Jan 19, 2018

many Contao users are running member services and don't want expired accounts still receiving members-only newsletters by default

I totally understand that but there are similar inconsitencies of this kind with stop-dates: in articles, pages or ce_elements for example - where the un/published-state is not linked with start/stop-dates.
I guess it's a question of concept rather than an inconsistency.

At least there should be better documentation in terms of the difference between de/activate, allow-login, start/stop-dates and the subscriptions.

@leofeyer leofeyer removed this from the 3.5.33 milestone Jan 19, 2018
@leofeyer
Copy link
Member

Make a date check on tl_member for any NL recipient who is also a member.

What exactly do you mean? Shouldn't the newsletter script check for disabled members upon sending instead?

As discussed in Mumble on September 27th, we have to check if there is a member with the same e-mail-address and check if they are disabled.

@leofeyer leofeyer added this to the 4.4.27 milestone Sep 27, 2018
@leofeyer leofeyer self-assigned this Oct 30, 2018
@leofeyer
Copy link
Member

Fixed in contao/contao@834e969.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants