An optimistic locking mechanism for campaigns

[fedys]

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
Related developer documentation PR URL
Issue(s) addressed

Description:

This PR adds support for optimistic locking. I tried to implement it via Doctrine functionality. It turned out to be too dangerous as it could cause many bugs (connected with OptimisticLockException). I implemented the optimistic locking the way we can control when an entity version is incremented/checked.

Let's consider the following steps:

1. Navigate to the edit page of an existing campaign.
2. Open a new browser tab and navigate to the edit page of the same campaign.
3. In the latter tab modify the name (append -2 for example) and click Apply.
4. Switch back to the first tab and without refreshing the browser modify the name (append -3 for example) and click Apply.
5. The modification from the third step is lost.

Steps to test this PR:

1. Run the migration with the command ddev exec bin/console doctrine:migrations:migrate if needed
2. Navigate to the edit page of an existing campaign.
3. Open a new tab and navigate to the edit page of the same campaign.
4. In the latter tab modify the name (append -2 for example) and click Apply.
5. Switch back to the first tab and without refreshing the browser modify the name (append -3 for example) and click Apply.
6. You should get an error message that reads The record you are updating has been changed by someone else in the meantime. Please refresh the browser window and re-submit your changes..
7. The modification from the third step is not lost.

[codecov]

Codecov Report

Attention: Patch coverage is 92.53731% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 61.61%. Comparing base (f13b8a4) to head (106c9fe).

Additional details and impacted files

@@             Coverage Diff              @@
##                5.x   #13659      +/-   ##
============================================
+ Coverage     61.53%   61.61%   +0.07%     
- Complexity    34098    34118      +20     
============================================
  Files          2241     2243       +2     
  Lines        101930   101997      +67     
============================================
+ Hits          62719    62841     +122     
+ Misses        39211    39156      -55     

Files	Coverage Δ
app/bundles/CampaignBundle/Entity/Campaign.php	80.58% <100.00%> (+0.09%)	⬆️
.../bundles/CampaignBundle/Form/Type/CampaignType.php	93.93% <100.00%> (+0.18%)	⬆️
.../bundles/CoreBundle/Entity/OptimisticLockTrait.php	100.00% <100.00%> (ø)
...eBundle/EventListener/OptimisticLockSubscriber.php	92.30% <92.30%> (ø)
...ndle/Controller/AbstractStandardFormController.php	61.89% <86.36%> (+5.94%)	⬆️

... and 6 files with indirect coverage changes

[shinde-rahul]

LGTM!

Thanks @fedys!!

[Mike-Dropsolid]

Followed the steps and works as described!

[RCheesley]

This worked fine if I just changed something like the name.

If I opened the builder, and made a change to the campaign, then I got the following 500 error:

500 Internal Server Error - Key "deleted" for array with keys "canvasSettings, name, triggerMode, triggerDate, triggerInterval, triggerIntervalUnit, triggerHour, triggerRestrictedStartHour, triggerRestrictedStopHour, anchor, properties, type, eventType, anchorEventType, campaignId, _token, buttons, settings, triggerRestrictedDaysOfWeek, tempId, id" does not exist.

Stack trace:

[2024-04-25T13:22:03.155371+00:00] mautic.CRITICAL: Uncaught PHP Exception Twig\Error\RuntimeError: "Key "deleted" for array with keys "canvasSettings, name, triggerMode, triggerDate, triggerInterval, triggerIntervalUnit, triggerHour, triggerRestrictedStartHour, triggerRestrictedStopHour, anchor, properties, type, eventType, anchorEventType, campaignId, _token, buttons, settings, triggerRestrictedDaysOfWeek, tempId, id" does not exist." at /var/www/html/app/bundles/CampaignBundle/Resources/views/Campaign/_builder.html.twig line 36 {"exception":"[object] (Twig\\Error\\RuntimeError(code: 0): Key \"deleted\" for array with keys \"canvasSettings, name, triggerMode, triggerDate, triggerInterval, triggerIntervalUnit, triggerHour, triggerRestrictedStartHour, triggerRestrictedStopHour, anchor, properties, type, eventType, anchorEventType, campaignId, _token, buttons, settings, triggerRestrictedDaysOfWeek, tempId, id\" does not exist. at /var/www/html/app/bundles/CampaignBundle/Resources/views/Campaign/_builder.html.twig:36)
[stacktrace]
#0 /var/www/html/vendor/twig/twig/src/Environment.php(360) : eval()'d code(132): twig_get_attribute(Object(Twig\\Environment), Object(Twig\\Source), Array, 'deleted', Array, 'any', false, false, false, 36)
#1 /var/www/html/vendor/twig/twig/src/Template.php(394): __TwigTemplate_b3b345528e9a52da99d56ed8e00e11da->doDisplay(Array, Array)
#2 /var/www/html/vendor/twig/twig/src/Template.php(367): Twig\\Template->displayWithErrorHandling(Array, Array)
#3 /var/www/html/vendor/twig/twig/src/Template.php(379): Twig\\Template->display(Array)
#4 /var/www/html/vendor/twig/twig/src/TemplateWrapper.php(38): Twig\\Template->render(Array)
#5 /var/www/html/vendor/twig/twig/src/Extension/CoreExtension.php(1347): Twig\\TemplateWrapper->render(Array)
#6 /var/www/html/vendor/twig/twig/src/Environment.php(360) : eval()'d code(170): twig_include(Object(Twig\\Environment), Array, '@MauticCampaign...', Array)
#7 /var/www/html/vendor/twig/twig/src/Template.php(171): __TwigTemplate_38a11f0e36529cc48894b46211a2c936->block_content(Array, Array)
#8 /var/www/html/vendor/twig/twig/src/Template.php(243): Twig\\Template->displayBlock('content', Array, Array, true)
#9 /var/www/html/vendor/twig/twig/src/Environment.php(360) : eval()'d code(66): Twig\\Template->renderBlock('content', Array, Array)
#10 /var/www/html/vendor/twig/twig/src/Template.php(394): __TwigTemplate_274f4d32b0e34e384411d368c6082172->doDisplay(Array, Array)
#11 /var/www/html/vendor/twig/twig/src/Template.php(367): Twig\\Template->displayWithErrorHandling(Array, Array)
#12 /var/www/html/vendor/twig/twig/src/Environment.php(360) : eval()'d code(49): Twig\\Template->display(Array, Array)
#13 /var/www/html/vendor/twig/twig/src/Template.php(394): __TwigTemplate_38a11f0e36529cc48894b46211a2c936->doDisplay(Array, Array)
#14 /var/www/html/vendor/twig/twig/src/Template.php(367): Twig\\Template->displayWithErrorHandling(Array, Array)
#15 /var/www/html/vendor/twig/twig/src/Template.php(379): Twig\\Template->display(Array)
#16 /var/www/html/vendor/twig/twig/src/TemplateWrapper.php(38): Twig\\Template->render(Array)
#17 /var/www/html/vendor/twig/twig/src/Environment.php(280): Twig\\TemplateWrapper->render(Array)
#18 /var/www/html/vendor/symfony/framework-bundle/Controller/AbstractController.php(258): Twig\\Environment->render('@MauticCampaign...', Array)
#19 /var/www/html/app/bundles/CoreBundle/Controller/CommonController.php(334): Symfony\\Bundle\\FrameworkBundle\\Controller\\AbstractController->renderView('@MauticCampaign...', Array)
#20 /var/www/html/app/bundles/CoreBundle/Controller/CommonController.php(157): Mautic\\CoreBundle\\Controller\\CommonController->ajaxAction(Object(Symfony\\Component\\HttpFoundation\\Request), Array)
#21 /var/www/html/app/bundles/CoreBundle/Controller/AbstractStandardFormController.php(490): Mautic\\CoreBundle\\Controller\\CommonController->delegateView(Array)
#22 /var/www/html/app/bundles/CampaignBundle/Controller/CampaignController.php(207): Mautic\\CoreBundle\\Controller\\AbstractStandardFormController->editStandard(Object(Symfony\\Component\\HttpFoundation\\Request), '1', false)
#23 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(163): Mautic\\CampaignBundle\\Controller\\CampaignController->editAction(Object(Symfony\\Component\\HttpFoundation\\Request), '1', false)
#24 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(75): Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw(Object(Symfony\\Component\\HttpFoundation\\Request), 2)
#25 /var/www/html/vendor/symfony/framework-bundle/Controller/AbstractController.php(156): Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 2)
#26 /var/www/html/app/bundles/CoreBundle/Controller/CommonController.php(415): Symfony\\Bundle\\FrameworkBundle\\Controller\\AbstractController->forward('Mautic\\\\Campaign...', Array, Array)
#27 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(163): Mautic\\CoreBundle\\Controller\\CommonController->executeAction(Object(Symfony\\Component\\HttpFoundation\\Request), 'edit', '1', 0, '')
#28 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(75): Symfony\\Component\\HttpKernel\\HttpKernel->handleRaw(Object(Symfony\\Component\\HttpFoundation\\Request), 1)
#29 /var/www/html/vendor/symfony/http-kernel/Kernel.php(202): Symfony\\Component\\HttpKernel\\HttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#30 /var/www/html/app/AppKernel.php(109): Symfony\\Component\\HttpKernel\\Kernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#31 /var/www/html/app/middlewares/CORSMiddleware.php(76): AppKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#32 /var/www/html/app/middlewares/HSTSMiddleware.php(39): Mautic\\Middleware\\CORSMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#33 /var/www/html/app/middlewares/CatchExceptionMiddleware.php(28): Mautic\\Middleware\\HSTSMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#34 /var/www/html/app/middlewares/Dev/IpRestrictMiddleware.php(52): Mautic\\Middleware\\CatchExceptionMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#35 /var/www/html/app/middlewares/VersionCheckMiddleware.php(58): Mautic\\Middleware\\Dev\\IpRestrictMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#36 /var/www/html/app/middlewares/TrustMiddleware.php(42): Mautic\\Middleware\\VersionCheckMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#37 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Mautic\\Middleware\\TrustMiddleware->handle(Object(Symfony\\Component\\HttpFoundation\\Request), 1, true)
#38 /var/www/html/index.php(19): Stack\\StackedHttpKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request))
#39 {main}
"} {"hostname":"mautic-web","pid":11885}

[fedys]

Thanks @RCheesley! I tried to reproduce it in my environment but with no luck. Please can you provide me with the exact steps you took. Screen recording would help a lot.

[RCheesley]

Sure thing, here's a screen recording: https://app.screencastify.com/v3/watch/oBWSIOnNIxfozpvu2qJP

1. Edit the campaign name with -2 appended in window 2 and save
2. In the first window, edit the campaign, add -3 to the name (for example) and then edit it in the campaign builder and add a new action, for example, then save the campaign
3. Error thrown

[fedys]

@RCheesley thanks a ton! I'm able to reproduce it. I will take a look at what is going wrong.

[fedys]

@RCheesley I fixed the issue via cc159d1.

[RCheesley]

Thanks @fedys - it doesn't error out now but the message is a little bit confusing. It seems to be a combination of two messages, maybe:

You can see the form validation message at the start "There are some form validation errors"

Then it's telling me about the campaign issue "Campaign: The record you are updating has been changed by someone else in the meantime. Please refresh the browser window and re-submit your changes"

Then it finally closes about checking the form fields "Please close the builder and fix them".

I think here, we only need the Campaign warning message, not the two bits about the form validation? For reference all the fields were complete and valid, so I am guessing somewhere there's a bug with which error/s are triggering.

[fedys]

@RCheesley It comes from this generic campaign builder error message.

mautic/app/bundles/CoreBundle/Translations/en_US/messages.ini

Line 570 in fe72cb7

mautic.core.form.builder.error="There are some form validation errors (%error%) Please close the builder and fix them."

Please can you suggest a better wording?

[RCheesley]

Perhaps:

There are some errors preventing the saving of this campaign: (%error). You will need to fix those errors before you can save.

Looping in @mautic/ux-ui-tiger-team as they've been doing a bit of work on improving the consistency of our user-facing messages and might have a better suggestion!

[kuzmany]

Works for me 👍

[andersonjeccel]

The PR effectively does what it should.

Ruth’s comment about better wording for campaign builder errors probably should be tackled in a separate PR to fix the entire errors presentation, so I'd say we’re good for now