BUG: Major issue: Segment with filter incorrectly deletes leads if a new lead with a lower "leadid" qualifies for segment.

[fcatanzaro]

What type of report is this:

Q	A
Bug report?	Yes
Feature request?
Enhancement?

Description:

On line 320 of LeadListRepository.php the code is adding $batchLimiters['maxId'] to the segement selection filter in an attempt to "Only leads that existed at the time of count" like the code comment states.

This a fundamentally FLAWED way of doing things and creates 2 HUGE problems:

1. The "maxId" is the highest leadid # in the qualifying list. It uses this to not choose any leads that have a higher id than this, presumably "newer" leads, but the problem is that a lead can have a lower id# and qualify later. It makes no sense to correlate the leadid ordinal to the order in which leads will qualify for a segment! A "newer" leadid may do the thing that makes them qualify for a segment (e.g. hit a specific page) before or after an "older" leadid, this makes no sense to have this at all. I get the intention of the code (to be consistent between the count(*) query and the actual select, but this won't work and it is better to give up that consistency than to break the segmenting.
2. The SECOND AND LARGER PROBLEM: because of the way the segment filter is built, if an "older" lead qualifies as per above, then any leads that were "newer" (higher) WILL GET INCORRECTLY DELETED FROM THE SEGMENT. Then upon re-running the segment:update those deleted leads will get re-qualifies and re-added!! This wreaks complete havoc with campaigns!

If a bug:

Q	A
Mautic version	2.1.1
PHP version	7.0.8

Steps to reproduce:

1. I described the exact problem with the code above. The simplest solution would be to get rid of lines 319-324 altogether and not worry about that attempt at consistency. Ignore the "maxId" and not use it in the selections.

Log errors:

See above.

[fcatanzaro]

Hi, I'm a little worried nobody has picked this up or tested this. Without addressing this, many(maybe all) automatic segments are just broken.

[escopecz]

Would you be able to send a pull request with your solution?

[alanhartless]

I think we need to fix the intention rather than just removing it. Otherwise the script may very well find itself stuck in a loop. I thought that "newer" leads should be filtered out by

mautic/app/bundles/LeadBundle/Entity/LeadListRepository.php

Line 351 in c44c1ab

$q->expr()->lte('ll.date_added', $q->expr()->literal($batchLimiters['dateTime']))

and thus not deleted.

Have you been able to reproduce this and confirming the fix by manipulating the code? I'll try to reproduce.

[fcatanzaro]

To duplicate the error:

1. create a segment which adds people based on some criteria, like visiting a page (my specific issue) or having a particular field value set should also work to simulate the effect.
2. get a handful of leads to do whatever they need to qualify for this segment & run cron to get them in.
3. pick a lead who has a leadid somewhere in between the group that was previously added and then do the thing to this lead that will make it qualify for the segment.
4. When you run cron next time you will see that this latest lead (it ain't new) gets added into the segment, and then ANYONE whose leadid > than the one you just entered (this is where maxId comes in) gets removed from the segment.
5. When you run cron again, then the removed people get re-qualified and re-added to the segment, but now their date-added is newer, which will cause problems with campaigns that starts with qualifying into a segment and relies on waiting time before another step, the people keep re-qualifying and restarting the campaign, never getting to trigger the next step...

[fcatanzaro]

To be clear @alanhartless the 'newer' leads being filtered as you point out in that line would only work if the leads qualify for a segment in the same order that the leads were identified/created. That line would not leave out leads that qualify after the count but are 'older' than the date listed when the count ran, that is one of the problems, albeit a minor one.

[fcatanzaro]

Any update on this bug? Can I provide any further info on this?

[alanhartless]

@fcatanzaro Please test against #2716. Thanks!