DPMMA-3048 Fix campaign execution stuck due to incorrect lead detachment in membership change action

[patrykgruszka]

Q	A
Bug fix? (use the a.b branch)	🟢
New feature/enhancement? (use the a.x branch)	🔴
Deprecations?	🔴
BC breaks? (use the c.x branch)	🔴
Automated tests included?	🟢
Related user documentation PR URL	mautic/user-documentation#...
Related developer documentation PR URL	mautic/developer-documentation-new#...
Issue(s) addressed	Fixes #14034

Description

The campaign execution was getting stuck due to an issue with lead detachment in the change membership action. The previous implementation was detaching all contacts, including those newly added to the campaign, which caused problems while trying to save the campaign log after detach.

This PR also addresses an issue in the testCampaignActionChangeMembership test. Previously, the test was incorrectly set up: it was adding the contact to both campaigns before executing the campaign action.

---

📋 Steps to test this PR:

1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

2. Create two empty segments in Mautic.

3. Create Campaign 1:

   • Start with Segment 1
   • Add an action (e.g., add tag to contact)

4. Create Campaign 2:

   • Start with Segment 2
   • Add "Change Campaigns" action
   • Select "Add to Campaign 1" in the action settings

5. Add a contact to Segment 2 and run the campaigns:

   php bin/console m:ca:u
   php bin/console m:ca:t
   

6. Verify that the campaign execution was successful.

7. Check if the created contact is now a member of Campaign 1.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.47%. Comparing base (eec36a1) to head (0cbc1f7).
Report is 74 commits behind head on 5.2.

Additional details and impacted files

@@             Coverage Diff              @@
##                5.2   #14497      +/-   ##
============================================
+ Coverage     63.44%   63.47%   +0.02%     
- Complexity    34630    34637       +7     
============================================
  Files          2273     2273              
  Lines        103594   103609      +15     
============================================
+ Hits          65728    65767      +39     
+ Misses        37866    37842      -24     

Files with missing lines	Coverage Δ
...es/CampaignBundle/Membership/MembershipManager.php	68.75% <100.00%> (-6.25%)	⬇️

... and 9 files with indirect coverage changes

[escopecz]

This happened by accident when the EM->clear() method was removed and the code needed to be refactored in 777e2dc#diff-88261e62c0ae339645adadf9bb77a8efda5a985a11c5c517c7dcf10da734507a

@biozshock can you please review this too?

[biozshock]

You can change the campaign in place, allowing later to grasp what's going on without a need to review method signature.

Suggested change

	$campaign2 = $this->createCampaign('Campaign 2', true);
	$campaign2 = $this->createCampaign('Campaign 2');
	$campaign2->setAllowRestart(true);

[biozshock]

I'm not a big fan of adding more and more parameters to the methods. This ups the cognitive load. Because the campaign is an object you can freely change it whenever needed later in the test.

Suggested change

	protected function createCampaign(string $campaignName, bool $allowRestart = false): Campaign
	{
	$campaign = new Campaign();
	$campaign->setName($campaignName);
	$campaign->setIsPublished(true);
	$campaign->setAllowRestart($allowRestart);
	protected function createCampaign(string $campaignName): Campaign
	{
	$campaign = new Campaign();
	$campaign->setName($campaignName);
	$campaign->setIsPublished(true);

[mautibot]

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/cron-job-for-campaigns-buggy-5-1-0/33217/6

[mikiba]

I tested it in a real application. Before this change I got huge amount of "app.ERROR: CAMPAIGN: Multiple non-persisted new entities were found through the given association graph:" errors and basically every campaign email sending stopped. With this small, but important change everything works as excepted.