Skip to content

Commit

Permalink
Restore X value in contact boolean fields (#10716)
Browse files Browse the repository at this point in the history 
* Fix empty value in contact boolean fields

* Fix configuration page save

* Fix controller functional tests

* Always retype the isEnabled param to int. Even for PATCH

* Fixing phpstan issue

  Line   app/bundles/ChannelBundle/Controller/Api/MessageApiController.php
 ------ -------------------------------------------------------------------
  32     Property
         Mautic\ChannelBundle\Controller\Api\MessageApiController::$model
         (Mautic\ChannelBundle\Model\MessageModel) does not accept
         Mautic\CoreBundle\Model\AbstractCommonModel.

* Fixing PHP Notice - Undefined index: email

"file":"\/var\/www\/html\/mautic\/app\/bundles\/ChannelBundle\/Controller\/Api\/MessageApiController.php","line":54

I cannot figure out why the condition for PATCH was there in the first place. It's causing only troubles.

* Adding functional API tests covering the problematic endpoint and method, fixing the controller

* Fix csfixer issue

* Adding missing description to payloads for assertion

Co-authored-by: John Linhart <admin@escope.cz>
Co-authored-by: Ruth Cheesley <ruth.cheesley@acquia.com>
  • Loading branch information
@patrykgruszka @escopecz @RCheesley
3 people committed Jan 24, 2022
1 parent 7de3cbc commit 9e271e5
Show file tree
Hide file tree
Showing 8 changed files with 235 additions and 29 deletions.
1 change: 0 additions & 1 deletion UPGRADE-4.0.md
View file
Expand Up @@ -6,7 +6,6 @@
* Symfony 4
* Symfony deprecations were removed or refactored [https://github.com/symfony/symfony/blob/4.4/UPGRADE-4.0.md](https://github.com/symfony/symfony/blob/4.4/UPGRADE-4.0.md)
* Services are now private by default in Symfony 4. Mautic has a "hack" to register its own services as public but dependency injection should be preferred for Commands, Controllers, and services. Some Symfony services may no longer be available to the Controller via the Container.
* \Mautic\CoreBundle\Form\Type\YesNoButtonGroupType now uses false/true values which Symfony 4 will convert to empty values for No in the UI. This shouldn't cause issues for most unless the field is using a NotBlank constraint, which is no longer valid, or submitting a form via a functional test with 0 as the value of a YesNoButtonGroupType field.
* Packages removed
* debril/rss-atom-bundle removed
* egeloen/ordered-form-bundle removed
Expand Down
38 changes: 20 additions & 18 deletions app/bundles/ChannelBundle/Controller/Api/MessageApiController.php
View file
Expand Up @@ -15,14 +15,18 @@
use Mautic\ChannelBundle\ChannelEvents;
use Mautic\ChannelBundle\Entity\Message;
use Mautic\ChannelBundle\Event\ChannelEvent;
use Mautic\ChannelBundle\Model\MessageModel;
use Mautic\CoreBundle\Model\AbstractCommonModel;
use Symfony\Component\Form\Form;
use Symfony\Component\HttpKernel\Event\FilterControllerEvent;

/**
* Class MessageController.
*/
class MessageApiController extends CommonApiController
{
/**
* @var MessageModel|AbstractCommonModel
*/
protected $model;

public function initialize(FilterControllerEvent $event)
{
$this->model = $this->getModel('channel.message');
Expand All @@ -38,23 +42,21 @@ protected function prepareParametersFromRequest(Form $form, array &$params, $ent
{
parent::prepareParametersFromRequest($form, $params, $entity, $masks);

if ('PATCH' != $this->request->getMethod()) {
$channels = $this->getModel('channel.message')->getChannels();
if (!isset($params['channels'])) {
$params['channels'] = [];
}
if ('PATCH' === $this->request->getMethod() && !isset($params['channels'])) {
return;
} elseif (!isset($params['channels'])) {
$params['channels'] = [];
}

$channels = $this->model->getChannels();

foreach ($channels as $channelType => $channel) {
if (!isset($params['channels'][$channelType])) {
$params['channels'][$channelType] = [
'isEnabled' => false,
'channel' => $channelType,
];
} else {
$params['channels'][$channelType]['channel'] = $channelType;
$params['channels'][$channelType]['isEnabled'] = (bool) $params['channels'][$channelType]['isEnabled'];
}
foreach ($channels as $channelType => $channel) {
if (!isset($params['channels'][$channelType])) {
$params['channels'][$channelType] = ['isEnabled' => 0];
} else {
$params['channels'][$channelType]['isEnabled'] = (int) $params['channels'][$channelType]['isEnabled'];
}
$params['channels'][$channelType]['channel'] = $channelType;
}
}

Expand Down
205 changes: 205 additions & 0 deletions app/bundles/ChannelBundle/Tests/Controller/Api/MessageApiControllerTest.php
View file
@@ -0,0 +1,205 @@
<?php

declare(strict_types=1);

namespace Mautic\ChannelBundle\Tests\Controller\Api;

use Mautic\ChannelBundle\Entity\Channel;
use Mautic\ChannelBundle\Entity\Message;
use Mautic\CoreBundle\Test\MauticMysqlTestCase;
use PHPUnit\Framework\Assert;

final class MessageApiControllerTest extends MauticMysqlTestCase
{
public function testCreateMessage(): void
{
$payloadJson = <<<'JSON'
{
"name": "API message",
"description": "Marketing message created via API functional test",
"channels": {
"email": {
"channel": "email",
"channelId": 12,
"isEnabled": true
}
}
}
JSON;

$payloadArray = json_decode($payloadJson, true);

$this->client->request('POST', '/api/messages/new', $payloadArray);
$responseJson = $this->client->getResponse()->getContent();
Assert::assertSame(201, $this->client->getResponse()->getStatusCode(), $responseJson);
$this->assertMessagePayload($payloadArray, json_decode($responseJson, true)['message'], $responseJson);
}

/**
* @dataProvider patchProvider
*
* @param mixed[] $payload
* @param mixed[] $expectedResponsePayload
*/
public function testEditMessageWithPatch(array $payload, array $expectedResponsePayload): void
{
$channel = new Channel();
$channel->setChannel('email');
$channel->setChannelId(12);
$channel->setIsEnabled(true);

$message = new Message();
$message->setName('API message');
$message->addChannel($channel);

$this->em->persist($channel);
$this->em->persist($message);
$this->em->flush();
$this->em->clear();

$patchPayload = ['id' => $message->getId()] + $payload;
$this->client->request('PATCH', "/api/messages/{$message->getId()}/edit", $patchPayload);
$responseJson = $this->client->getResponse()->getContent();
Assert::assertSame(200, $this->client->getResponse()->getStatusCode(), $responseJson);
$this->assertMessagePayload(
['id' => $message->getId()] + $expectedResponsePayload,
json_decode($responseJson, true)['message'],
$responseJson
);
}

/**
* Note: the ID is added to the payload automatically in the test.
*
* @return iterable<mixed[]>
*/
public function patchProvider(): iterable
{
yield [
[
'name' => 'API message (updated)',
],
[
'name' => 'API message (updated)',
'description' => null,
'channels' => [
'email' => [
'channel' => 'email',
'channelId' => 12,
'isEnabled' => true,
],
],
],
];

yield [
[
'description' => 'Description (updated)',
'channels' => [
'email' => [
'channel' => 'email',
'channelId' => 13,
'isEnabled' => false,
],
],
],
[
'name' => 'API message',
'description' => 'Description (updated)',
'channels' => [
'email' => [
'channel' => 'email',
'channelId' => 13,
'isEnabled' => false,
],
],
],
];
}

public function testEditMessagesWithPatch(): void
{
$channel1 = new Channel();
$channel1->setChannel('email');
$channel1->setChannelId(12);
$channel1->setIsEnabled(true);

$message1 = new Message();
$message1->setName('API message 1');
$message1->addChannel($channel1);

$channel2 = new Channel();
$channel2->setChannel('email');
$channel2->setChannelId(13);
$channel2->setIsEnabled(true);

$message2 = new Message();
$message2->setName('API message 2');
$message2->addChannel($channel2);

$this->em->persist($channel1);
$this->em->persist($channel2);
$this->em->persist($message1);
$this->em->persist($message2);
$this->em->flush();
$this->em->clear();

$patchPayload = [
['id' => $message1->getId(), 'name' => 'API message 1 (updated)'],
['id' => $message2->getId(), 'channels' => ['email' => ['channelId' => 14, 'isEnabled' => false]]],
];
$this->client->request('PATCH', '/api/messages/batch/edit', $patchPayload);
$responseJson = $this->client->getResponse()->getContent();
Assert::assertSame(200, $this->client->getResponse()->getStatusCode(), $responseJson);
$responseArray = json_decode($responseJson, true);
$this->assertMessagePayload(
[
'id' => $message1->getId(),
'name' => 'API message 1 (updated)',
'description' => null,
'channels' => [
'email' => [
'channel' => 'email',
'channelId' => 12,
'isEnabled' => true,
],
],
],
$responseArray['messages'][0],
$responseJson
);
$this->assertMessagePayload(
[
'id' => $message2->getId(),
'name' => 'API message 2',
'description' => null,
'channels' => [
'email' => [
'channel' => 'email',
'channelId' => 14,
'isEnabled' => false,
],
],
],
$responseArray['messages'][1],
$responseJson
);
}

/**
* @param mixed[] $expectedPayload
* @param mixed[] $actualPayload
*/
private function assertMessagePayload(array $expectedPayload, array $actualPayload, string $deliveredPayloadJson): void
{
Assert::assertSame($expectedPayload['name'], $actualPayload['name'], $deliveredPayloadJson);
Assert::assertSame($expectedPayload['description'], $actualPayload['description'], $deliveredPayloadJson);
Assert::assertCount(count($expectedPayload['channels']), $actualPayload['channels'], $deliveredPayloadJson);
Assert::assertGreaterThan(0, $actualPayload['id'], $deliveredPayloadJson);

Assert::assertSame($expectedPayload['channels']['email']['channel'], $actualPayload['channels'][0]['channel'], $deliveredPayloadJson);
Assert::assertSame($expectedPayload['channels']['email']['channelId'], $actualPayload['channels'][0]['channelId'], $deliveredPayloadJson);
Assert::assertSame($expectedPayload['channels']['email']['isEnabled'], $actualPayload['channels'][0]['isEnabled'], $deliveredPayloadJson);
Assert::assertGreaterThan(0, $actualPayload['channels'][0]['id'], $deliveredPayloadJson);
}
}
2 changes: 1 addition & 1 deletion app/bundles/ConfigBundle/Tests/Controller/ConfigControllerFunctionalTest.php
View file
Expand Up @@ -237,7 +237,7 @@ public function testConfigNotificationConfiguration(): void
$buttonCrawler = $crawler->selectButton('config[buttons][save]');
$form = $buttonCrawler->form();

$send_notification_to_author = '';
$send_notification_to_author = '0';
$campaign_notification_email_addresses = 'a@test.com, b@test.com';
$webhook_notification_email_addresses = 'a@webhook.com, b@webhook.com';

Expand Down
6 changes: 3 additions & 3 deletions app/bundles/CoreBundle/Form/Type/YesNoButtonGroupType.php
View file
Expand Up @@ -54,7 +54,7 @@ public function configureOptions(OptionsResolver $resolver)
return null;
}

return (is_string($choiceKey) && !is_numeric($choiceKey)) ? $choiceKey : (bool) $choiceKey;
return (is_string($choiceKey) && !is_numeric($choiceKey)) ? $choiceKey : (int) $choiceKey;
},
'expanded' => true,
'multiple' => false,
Expand All @@ -63,9 +63,9 @@ public function configureOptions(OptionsResolver $resolver)
'placeholder' => false,
'required' => false,
'no_label' => 'mautic.core.form.no',
'no_value' => false,
'no_value' => 0,
'yes_label' => 'mautic.core.form.yes',
'yes_value' => true,
'yes_value' => 1,
]
);
}
Expand Down
4 changes: 2 additions & 2 deletions app/bundles/FormBundle/Tests/Controller/FormControllerFunctionalTest.php
View file
Expand Up @@ -45,7 +45,7 @@ public function testSaveActionForm(): void
$form->setValues(
[
'mauticform[name]' => 'Test',
'mauticform[renderStyle]' => '',
'mauticform[renderStyle]' => '0',
]
);
$crawler = $this->client->submit($form);
Expand All @@ -54,7 +54,7 @@ public function testSaveActionForm(): void
$form = $crawler->filterXPath('//form[@name="mauticform"]')->form();
$form->setValues(
[
'mauticform[renderStyle]' => '',
'mauticform[renderStyle]' => '0',
]
);

Expand Down
4 changes: 2 additions & 2 deletions app/bundles/LeadBundle/Tests/Controller/LeadControllerTest.php
View file
Expand Up @@ -65,8 +65,8 @@ public function testAddCategorizedLeadList(): void
[
'leadlist[name]' => 'Segment 1',
'leadlist[alias]' => 'segment-1',
'leadlist[isGlobal]' => '',
'leadlist[isPreferenceCenter]' => '',
'leadlist[isGlobal]' => '0',
'leadlist[isPreferenceCenter]' => '0',
'leadlist[isPublished]' => '1',
'leadlist[publicName]' => 'Segment 1',
'leadlist[category]' => '1',
Expand Down
4 changes: 2 additions & 2 deletions app/bundles/LeadBundle/Tests/Controller/ListControllerFunctionalTest.php
View file
Expand Up @@ -64,7 +64,7 @@ public function testUnpublishUsedSegment(): void
$this->client->restart();
$crawler = $this->client->request(Request::METHOD_GET, '/s/segments/edit/'.$list1->getId());
$form = $crawler->selectButton('leadlist_buttons_apply')->form();
$form['leadlist[isPublished]']->setValue('');
$form['leadlist[isPublished]']->setValue('0');
$crawler = $this->client->submit($form);
$this->assertTrue($this->client->getResponse()->isOk());
$this->assertStringContainsString($expectedErrorMessage, $this->client->getResponse()->getContent());
Expand All @@ -89,7 +89,7 @@ public function testUnpublishUnUsedSegment(): void

$crawler = $this->client->request(Request::METHOD_GET, '/s/segments/edit/'.$list2->getId());
$form = $crawler->selectButton('leadlist_buttons_apply')->form();
$form['leadlist[isPublished]']->setValue('');
$form['leadlist[isPublished]']->setValue('0');
$crawler = $this->client->submit($form);
$this->assertTrue($this->client->getResponse()->isOk());

Expand Down

0 comments on commit 9e271e5

Please sign in to comment.