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

Symfony expects true/false for CheckboxType #9931

Merged
merged 6 commits into from
May 4, 2021

Conversation

alanhartless
Copy link
Contributor

@alanhartless alanhartless commented Apr 24, 2021

Q A
Branch? "features"
Bug fix? yes
New feature? no
Deprecations? no
BC breaks? no
Automated tests included? no (but functional and API tests should cover this change)
Related user documentation PR URL na
Related developer documentation PR URL na
Issue(s) addressed https://mautic.atlassian.net/secure/RapidBoard.jspa?rapidView=26&projectKey=TPROD&modal=detail&selectedIssue=TPROD-223

Description:

One of the API tests started to fail with the Symfony Forms version bump to 4.4.21, specifically with the addition of this code symfony/form@v4.4.20...v4.4.21#diff-bb5d9d94844af7f842e5b6aa429a8a0141316e527f0c4bd31d3a140adfc7c581R145.

1) 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

It seems that prior to 4.4.21, our YesNoButtonGroupType returning integers for values worked but now fails because of the above change leading to isEnabled being set as null. YesNoButtonGroupType is getting proxied as RadioType when resolved by Symfony forms which inherits CheckboxType and according to Symfony docs, should have boolean values. Switching the values of YesNoButtonGroupType from integers to booleans caused the API test to pass. Yes/No toggles in the UI also continue to work.

Steps to test this PR:

  1. Load up this PR
  2. Browse the UI and toggle the Yes/No buttons in various forms (typically used for published/enabled states). Changing the state, saving, and editing should repopulate the correct state.
  3. API functional tests should pass (the 5 tests that failed always fail on my local)

Screen Shot 2021-04-23 at 11 54 04 PM

4. Test the Configuration can be saved and manipulated 4. 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.

BC Breaks:

This changes causes Symfony to render the value of the No button as empty. This shouldn't cause any issues for most. But if they happen to be using a NotBlank constraints on the YesNoButtonGroupType, as found in this PR, it will fail because the submission will have an empty value (it shouldn't be required to start with because the buttons default to No). It also means that some functional tests that may have previously submitted 0 as the value will value as an invalid value.

@alanhartless alanhartless added ready-to-test PR's that are ready to test API Anything related to the API labels Apr 24, 2021
@alanhartless alanhartless added this to the 4.0-beta milestone Apr 24, 2021
@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Apr 24, 2021
@codecov
Copy link

codecov bot commented Apr 24, 2021

Codecov Report

Merging #9931 (08c5dfd) into features (c806115) will increase coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             features    #9931   +/-   ##
===========================================
  Coverage       39.91%   39.91%           
  Complexity      34336    34336           
===========================================
  Files            2038     2038           
  Lines          108606   108604    -2     
===========================================
  Hits            43350    43350           
+ Misses          65256    65254    -2     
Impacted Files Coverage Δ Complexity Δ
...nnelBundle/Controller/Api/MessageApiController.php 0.00% <0.00%> (ø) 9.00 <0.00> (ø)
...dles/CoreBundle/Form/Type/YesNoButtonGroupType.php 100.00% <100.00%> (ø) 7.00 <0.00> (ø)
app/bundles/ReportBundle/Form/Type/ConfigType.php 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
.../bundles/CoreBundle/Helper/Chart/AbstractChart.php 72.72% <0.00%> (-1.52%) 23.00% <0.00%> (ø%)
app/bundles/FormBundle/Entity/Form.php 80.47% <0.00%> (+1.19%) 76.00% <0.00%> (ø%)

@RCheesley RCheesley added the essential This must be done to close the milestone label Apr 24, 2021
Copy link
Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Load up this PR ✅
  2. Browse the UI and toggle the Yes/No buttons in various forms (typically used for published/enabled states). Changing the state, saving, and editing should repopulate the correct state. ✅
  3. API functional tests should pass (the 5 tests that failed always fail on my local) ❓ Seemingly unable to get this up and running locally as all my tests are failing having cloned the API Library and set up with ddev / run ddev exec composer test
  4. Test the Configuration can be saved and manipulated ✅
  5. Create a batch of messages via the API ✅
  6. Try to PATCH ✅
  7. Try to PUT ✅
  8. Try using 0/1 rather than yes/no ✅

So this is a +1 from me but we need someone to check the tests in step 3 as I was unable to get that part to work!

@RCheesley RCheesley added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels May 1, 2021
Copy link
Member

@dennisameling dennisameling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm this fixes the message_channels bug in the API library tests: https://github.com/mautic/api-library/pull/249/checks?check_run_id=2495446071

Code also LGTM, thanks @alanhartless 🚀

@dennisameling dennisameling added bc-break A BC break PR for major release milestones only bug Issues or PR's relating to bugs ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels May 3, 2021
@RCheesley RCheesley merged commit f409dbe into mautic:features May 4, 2021
@escopecz
Copy link
Member

escopecz commented Jan 6, 2022

This will have to be reverted: #10716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Anything related to the API bc-break A BC break PR for major release milestones only bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement essential This must be done to close the milestone ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants