Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Campaign decisions after contact removed #7802

Merged

Conversation

escopecz
Copy link
Member

@escopecz escopecz commented Aug 19, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Y
New feature? N
Automated tests included? Y
Related user documentation PR URL /
Related developer documentation PR URL /
Issues addressed (#s or URLs) #6407
BC breaks? N
Deprecations? N

Description:

Turns out we've had a bug for a long time that would execute events associated with the inactive path of a decision even if the contact had since been removed from the campaign.

Steps to reproduce the bug:

  1. Create a segment based on a filter
  2. Create a campaign using that segment as a source, send an email, connect an opens email decision, then connect a second send email to the right/red/inactive anchor of the decision. Set a wait time of 2 minutes
  3. If you don't already have contacts in the segment, modify/create a contact that matches the segments filter.
  4. Wait till the campaign sends the first email but DO NOT open the email
  5. Immediately update the contact so that they no longer match the filters in which they should be removed from the segment and campaign
  6. Wait until 2 minutes have passed and notice that the contact will get the second email even though they are removed from the campaign

Steps to test this PR:

  1. Repeat and the contact will not receive the second email after being removed from the campaign

@escopecz escopecz added bug ready-to-test labels Aug 19, 2019
@escopecz escopecz added this to the 2.15.3 milestone Aug 19, 2019
@regevbr
Copy link
Contributor

@regevbr regevbr commented Aug 19, 2019

@escopecz that is exactly my fix, but I think you missed the checkLeadInCampaigns method that should have the same fix as well. What do you think?

@escopecz
Copy link
Sponsor Member Author

@escopecz escopecz commented Aug 19, 2019

I'd agree. @alanhartless what about you?

@alanhartless
Copy link
Contributor

@alanhartless alanhartless commented Aug 19, 2019

Just by doing a quick glance at the code, yes it does seem to be the same bug in checkLeadInCampaigns.

Copy link
Member

@RCheesley RCheesley left a comment

Successful test - pre-patch the contact received the email despite being removed from the segment. Post-patch the contact does not receive the email!

@regevbr
Copy link
Contributor

@regevbr regevbr commented Aug 25, 2019

guys please don't forget to add the missing code we discussed if it is indeed relevant

@escopecz
Copy link
Sponsor Member Author

@escopecz escopecz commented Aug 26, 2019

@regevbr as we agree that the code change you propose make theoretically sense, it's not needed for the issue this PR solves. I don't think it's a good idea to change the code solely based on a theory. Do you have a concrete issue that is related to the checkLeadInCampaigns method?

@regevbr
Copy link
Contributor

@regevbr regevbr commented Aug 26, 2019

@escopecz nope, I don't have any... In any case I did change that line on my production instance and nothing breaks (for now) :-)

@escopecz escopecz added pending-test-confirmation and removed ready-to-test labels Aug 31, 2019
Copy link

@Strubbel94 Strubbel94 left a comment

works fine, just received one email.
no bugs found.

@npracht npracht added the ready-to-commit label Sep 6, 2019
@escopecz escopecz removed the pending-test-confirmation label Sep 6, 2019
@escopecz escopecz merged commit ff43f35 into mautic:staging Sep 6, 2019
2 checks passed
@escopecz escopecz deleted the campaign-decisions-after-contact-removed branch Sep 6, 2019
@waltercipcip
Copy link

@waltercipcip waltercipcip commented Sep 19, 2019

I don't find one of the files fixed in 2.15.1 installation:
app/bundles/CampaignBundle/Tests/Command/ValidateEventCommandTest.php
is this a new file to be added ?

@escopecz
Copy link
Sponsor Member Author

@escopecz escopecz commented Sep 19, 2019

Yes, but you don't need any files ending with *Test.php as those are just unit tests. Those files do not run on run time.

@waltercipcip
Copy link

@waltercipcip waltercipcip commented Sep 20, 2019

Yes, but you don't need any files ending with *Test.php as those are just unit tests. Those files do not run on run time.

So in this case I update only app/bundles/CampaignBundle/Entity/LeadRepository.php and don't need to add test file ?

@escopecz
Copy link
Sponsor Member Author

@escopecz escopecz commented Sep 20, 2019

Exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ready-to-commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants