Restore X value in contact boolean fields

[patrykgruszka]

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

Description:

This PR reverts changes from #9931 that caused issues with contact boolean fields. YesNoButtonGroupType actually uses the Symfony ChoiceType and not the CheckboxType as assumed in #9931, so the 1 0 options should be correct.

After applying this PR the boolean fields in the contact edit page should have 3 options again:

Before applying this PR:

I have checked the failing test Mautic\Tests\Api\MessagesTest::testBatchEndpoints from Mautic API repo, and it works fine after applying this PR. I couldn't replicate this issue from #9931.

Steps to test this PR:

1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
2. Add boolean custom field if you don't have any
3. The boolean field in contact edit page should have 3 options X Yes No. Test changing the state.
4. The boolean field in the campaign update contact action should have 3 options X Yes No. Test changing the state.

#9931 Regression tests
4. Browse the UI and toggle the Yes/No buttons in various forms and on the contact edit page. Changing the state, saving, and editing should repopulate the correct state.
5. API functional tests should pass (the 5 tests that failed always fail on my local)
6. Test the Configuration can be saved and manipulated
7. Create a batch of messages via the API
POST /api/messages/batch/new
(change channelId to be the ID of an Email and SMS that exists in your Mautic)

[{
		"name": "API message",
		"description": "Marketing message created via API unit test",
		"channels": {
			"email": {
				"channel": "email",
				"channelId": 27,
				"isEnabled": false
			},
			"sms": {
				"channel": "sms",
				"channelId": 17,
				"isEnabled": true
			}
		}
	}, {
		"name": "API message",
		"description": "Marketing message created via API unit test",
		"channels": {
			"email": {
				"channel": "email",
				"channelId": 27,
				"isEnabled": false
			},
			"sms": {
				"channel": "sms",
				"channelId": 17,
				"isEnabled": true
			}
		}
	}, {
		"name": "API message",
		"description": "Marketing message created via API unit test",
		"channels": {
			"email": {
				"channel": "email",
				"channelId": 27,
				"isEnabled": false
			},
			"sms": {
				"channel": "sms",
				"channelId": 17,
				"isEnabled": true
			}
		}
	}
]

Then try to PATCH by switching the isEnabled of each channel and adding the message ID:
PATCH /api/messages/batch/edit

[
    {
        "id": 97,
        "name": "API message",
        "description": "Marketing message created via API unit test",
        "channels": {
            "email": {
                "channel": "email",
                "channelId": 27,
                "isEnabled": true
            },
            "sms": {
                "channel": "sms",
                "channelId": 17,
                "isEnabled": false
            }
        }
    },
    {
        "id": 98,
        "name": "API message",
        "description": "Marketing message created via API unit test",
        "channels": {
            "email": {
                "channel": "email",
                "channelId": 27,
                "isEnabled": true
            },
            "sms": {
                "channel": "sms",
                 "channelId": 17,
                "isEnabled": false
            }
        }
    },
    {
        "id": 99,
        "name": "API message",
        "description": "Marketing message created via API unit test",
        "channels": {
            "email": {
                "channel": "email",
                "channelId": 27,
                "isEnabled": true
            },
            "sms": {
                "channel": "sms",
                "channelId": 17,
                "isEnabled": false
            }
        }
    }
]

Then try PUT as well
PUT /api/messages/batch/edit

[
    {
        "id": 97,
        "isPublished": true,
        "name": "API message",
        "description": "Marketing message created via API unit test",
        "channels": {
            "email": {
                "channel": "email",
                "channelId": 27,
                "isEnabled": true
            },
            "sms": {
                "channel": "sms",
                "channelId": 17,
                "isEnabled": false
            }
        }
    },
    {
        "id": 98,
        "name": "API message",
        "description": "Marketing message created via API unit test",
        "channels": {
            "email": {
                "channel": "email",
                "channelId": 27,
                "isEnabled": true
            },
            "sms": {
                "channel": "sms",
                 "channelId": 17,
                "isEnabled": false
            }
        }
    },
    {
        "id": 99,
        "name": "API message",
        "description": "Marketing message created via API unit test",
        "channels": {
            "email": {
                "channel": "email",
                "channelId": 27,
                "isEnabled": true
            },
            "sms": {
                "channel": "sms",
                "channelId": 17,
                "isEnabled": false
            }
        }
    }
]

Finally, try using 0/1 instead of true/false.

[jensolsson]

Tried it a bit, seem to work perfect in the web GUI, but over the REST API I cannot set it to X. I have tried setting a null-var but it does not work.
Maybe it has always been like this and was not an issue, but now when all these have been defaulted to No, I need a way to set them to null / X.

So for exampe this works to set Yes:
curl -X "POST" "http:///api/contacts/new"
-H 'Content-Type: application/json; charset=utf-8'
-u 'jens.olsson:Cryst4l!@#'
-d $'{
"testbool": true,
"overwriteWithBlank": false,
"email": "",
}'

This works to set No
curl -X "POST" "http:///api/contacts/new"
-H 'Content-Type: application/json; charset=utf-8'
-u 'jens.olsson:Cryst4l!@#'
-d $'{
"testbool": false,
"overwriteWithBlank": false,
"email": "",
}'

But this does not work to set X:
curl -X "POST" "http:///api/contacts/new"
-H 'Content-Type: application/json; charset=utf-8'
-u 'jens.olsson:Cryst4l!@#'
-d $'{
"testbool": null,
"overwriteWithBlank": false,
"email": "",
}'

Would be great if we can have this fixed as well

[patrykgruszka]

@jensolsson Thank you for feedback. Setting overwriteWithBlank to true and passing field value as null should do the trick.

[jensolsson]

@jensolsson Thank you for feedback. Setting overwriteWithBlank to true and passing field value as null should do the trick.

Problem is that in that case all values I do not explicity set are reset to null. For example if I completely leave out for example "title" then it would set it to null on the contact even though the contact already have a title as CEO for example.

So this imposes a great problem when having multiple data sources.

[patrykgruszka]

This case actually works fine for me.
POST /api/contacts/new

{
    "email": "test3@example.com",
    "title": "ceo",
    "newsletter_consent": true,
    "overwriteWithBlank": false
}

Contact created, id: 489

Edit contact with PATCH method:
PATCH /api/contacts/489/edit

{
    "email": "test3@example.com",
    "newsletter_consent": null,
    "overwriteWithBlank": true
}

After update the contact title is persisted.

[jensolsson]

We always create new contacts with Post to /api/contacts/new and it has always worked. It just changes the fields we send and it updates the existing contact if it existe.

The overhead of first searching for the contact and using its id would be alot i think even though it would be possible. But maybe it worked the same in <4.0.0. I have never had a need to reset a yesno switch via the api before.

[jensolsson]

Here is another one referencing this complicated sequence https://forum.mautic.org/t/mautic-api-editing-contact-by-email/16997

[patrykgruszka]

Updating contacts using the create method doesn't really seem correct to me, it works because of contact merging functionality. In my opinion, the changes you suggest should be the subject of another PR. Let's focus here on fixing the boolean fields, which I think is more critical.

[jensolsson]

Totally agree with you this is off topic.

[codecov]

Codecov Report

Merging #10716 (7102e4e) into 4.1 (7de3cbc) will increase coverage by 0.26%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##                4.1   #10716      +/-   ##
============================================
+ Coverage     44.76%   45.03%   +0.26%     
- Complexity    34814    34815       +1     
============================================
  Files          2080     2080              
  Lines        116716   116737      +21     
============================================
+ Hits          52246    52568     +322     
+ Misses        64470    64169     -301     

Impacted Files	Coverage Δ
...nnelBundle/Controller/Api/MessageApiController.php	93.33% <80.00%> (+93.33%)	⬆️
...dles/CoreBundle/Form/Type/YesNoButtonGroupType.php	100.00% <100.00%> (ø)
app/bundles/CoreBundle/Entity/CommonEntity.php	94.82% <0.00%> (+8.62%)	⬆️
...bundles/ChannelBundle/Entity/MessageRepository.php	13.95% <0.00%> (+13.95%)	⬆️
app/bundles/ChannelBundle/Event/ChannelEvent.php	88.88% <0.00%> (+38.88%)	⬆️
app/bundles/ChannelBundle/Model/MessageModel.php	58.49% <0.00%> (+41.50%)	⬆️
.../ChannelBundle/EventListener/MessageSubscriber.php	62.50% <0.00%> (+41.66%)	⬆️
app/bundles/ChannelBundle/Entity/Channel.php	83.33% <0.00%> (+53.33%)	⬆️
app/bundles/ChannelBundle/Entity/Message.php	85.33% <0.00%> (+62.66%)	⬆️
... and 4 more

[escopecz]

This is interesting. We must ensure that the API Library test that was failing before and why the changes were originally made are passing with this change. I wanted to run GitHub Actions on this PR but it failed on outdated Composer package. I prepared a fix for that but it needs reviewers.

mautic/api-library#264

is blocking this PR until merged.

[escopecz]

So the API Library tests are passing on main but when I run them on your PR it fails:
https://github.com/mautic/api-library/runs/4712825499?check_suite_focus=true

There were 2 failures:

1) Mautic\Tests\Api\CampaignsTest::testCampaignContactEditEvent
The response has unexpected status code (500).

Response: {"errors":[{"message":"Call to a member function getTimestamp() on null","code":500,"type":null}],"trace":[{"namespace":"","short_class":"","class":"","type":"","function":"","file":"\/var\/www\/html\/mautic\/app\/bundles\/CampaignBundle\/Model\/SummaryModel.php","line":45,"args":[]},{"namespace":"Mautic\\CampaignBundle\\Model","short_class":"SummaryModel","class":"Mautic\\CampaignBundle\\Model\\SummaryModel","type":"-\u003E","function":"updateSummary","file":"\/var\/www\/html\/mautic\/app\/bundles\/CampaignBundle\/Executioner\/Logger\/EventLogger.php","line":105,"args":[]},{"namespace":"Mautic\\CampaignBundle\\Executioner\\Logger","short_class":"EventLogger","class":"Mautic\\CampaignBundle\\Executioner\\Logger\\EventLogger","type":"-\u003E","function":"persistLog","file":"\/var\/www\/html\/mautic\/app\/bundles\/CampaignBundle\/Executioner\/Scheduler\/EventScheduler.php","line":131,"args":[]},{"namespace":"Mautic\\CampaignBundle\\Executioner\\Scheduler","short_class":"EventScheduler","...

Failed asserting that false is true.

/home/runner/work/api-library/api-library/tests/Api/MauticApiTestCase.php:89
/home/runner/work/api-library/api-library/tests/Api/CampaignsTest.php:436

2) Mautic\Tests\Api\MessagesTest::testBatchEndpoints
The response has unexpected status code (500).

Response: {"errors":[{"message":"An exception occurred while executing \u0027UPDATE message_channels SET channel_id = ?, is_enabled = ? WHERE id = ?\u0027 with params [\u00225\u0022, null, 14]:\n\nSQLSTATE[23000]: Integrity constraint violation: 1048 Column \u0027is_enabled\u0027 cannot be null","code":500,"type":null}],"trace":[{"namespace":"","short_class":"","class":"","type":"","function":"","file":"\/var\/www\/html\/mautic\/vendor\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/Driver\/AbstractMySQLDriver.php","line":125,"args":[]},{"namespace":"Doctrine\\DBAL\\Driver","short_class":"AbstractMySQLDriver","class":"Doctrine\\DBAL\\Driver\\AbstractMySQLDriver","type":"-\u003E","function":"convertException","file":"\/var\/www\/html\/mautic\/vendor\/doctrine\/dbal\/lib\/Doctrine\/DBAL\/DBALException.php","line":182,"args":[]},{"namespace":"Doctrine\\DBAL","short_class":"DBALException","class":"Doctrine\\DBAL\\DBALException","type":"::","function":"wrapException","file":"\/var\/www\/html\/mautic\/vendor\/d...

Failed asserting that false is true.

/home/runner/work/api-library/api-library/tests/Api/MauticApiTestCase.php:89
/home/runner/work/api-library/api-library/tests/Api/MauticApiTestCase.php:104
/home/runner/work/api-library/api-library/tests/Api/MessagesTest.php:74
/home/runner/work/api-library/api-library/tests/Api/MauticApiTestCase.php:284
/home/runner/work/api-library/api-library/tests/Api/MessagesTest.php:126

So that needs to be addressed before we can merge this PR.

[escopecz]

I fixed the API error in the original PR that this one reverts. The API Library error on SummaryModel is not related to this PR and is fixed in #10708

I can confirm the regression bug and this PR fixes that. The problem is in Symfony forms themselves. They take false as null for reasons I don't comprehend. So we have to go back to 0 and 1 in order to support the empty|null option for boolean custom fields.

[escopecz]

I approved it prematurely. The tests fail on normal patch endpoint:

Mautic\Tests\Api\MessagesTest::testEditPatch
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     'channel' => 'email'
     'channelId' => 23
-    'isEnabled' => true
+    'isEnabled' => false
 )

/home/runner/work/api-library/api-library/tests/Api/MessagesTest.php:83
/home/runner/work/api-library/api-library/tests/Api/MauticApiTestCase.php:312
/home/runner/work/api-library/api-library/tests/Api/MessagesTest.php:112

I'm editing marketing message with PATCH /api/messages/2/edit with payload

{
  "id": 2,
  "name": "API message 2",
  "description": "Marketing message created via API unit test",
  "channels": {
    "email": {
      "channel": "email",
      "channelId": 12,
      "isEnabled": true
    },
    "sms": {
      "channel": "sms",
      "channelId": 13,
      "isEnabled": 0
    }
  }
}

similar to the one in the test:

https://github.com/mautic/api-library/blob/main/tests/Api/MessagesTest.php#L26-L42

But it just works...

I cannot understand why it's failing. I cannot reproduce it. It should be asserting true but it's asserting false for some reason. I'll have to sleep on it. (or anyone else see the problem?)

[patrykgruszka]

@escopecz the failing test payload is {"name": "Modified by PATCH"}. I think this condition for PATCH in prepareParametersFromRequest was necessary. When you use this method only to change name the channels param is set to []:

if (!isset($params['channels'])) {
    $params['channels'] = [];
}

And then this conditional is is setting isEnabled to 0:

if (!isset($params['channels'][$channelType])) {
    $params['channels'][$channelType] = ['isEnabled' => 0];
}

[escopecz]

@patrykgruszka that's what I was missing. Thanks for guidance!

I prepared the fix with several tests in c2b3c85

I somehow lost ability to push to your fork directly. Could you please do this:

1. git fetch upstream (assuming the mautic/mautic repo is called upstream, adjust if needed)
2. git checkout mtc-fix-contact-bool-fields
3. git pull
4. git cherry-pick c2b3c8573d57b0a9bc7dee8bfa6da0a26f589ae7
5. git push

[patrykgruszka]

Sure, done. There is a problem with one test.

[escopecz]

I must have different error level setting on my local that it did not show up the problem. Thanks! I managed to push to your branch again. Everything is green including the API Library tests (as mentioned above, the last failing test is not related and fixed in another PR)

@patrykgruszka thanks for this!

[RCheesley]

Thanks for the PR @patrykgruszka and for fixing this issue!

Tested in the UI and the API as instructed (thanks for the very clear instructions with the JSON!) and all cases are acting as expected:

Before the PR:

After the PR:

Via the API:

LGTM! 🚀

[RCheesley]

@all-contributors please add @jensolsson for userTesting

[allcontributors]

@RCheesley

I've put up a pull request to add @jensolsson! 🎉