24070 - Allowed multivalue form elements to submit data to website/gr…

[ericclaeren]

Example

Issue

Proposed solution

Description (*)

Fix for issue: #24070

When submitting a website, group or store entity form (add/edit), submitted multivalue form elements get filtered and only the last selected element will be posted to the controller.

This posted data

fieldname[]: value1
fieldname[]: value2

Turns into

{
   fieldname[]: value2
}

Fixed Issues (if relevant)

1. Website/Group/Store entity form can't be extended with multiple value form element for extension attributes #24070 (comment) : Website/Group/Store entity form can't be extended with multiple value form element for extension attributes

• Transform fieldnames with multiple [] to an indexed one.
  ** This:

fieldname[]: value1
fieldname[]: value2

** Turns into

fieldname[]: value1
fieldname[1]: value2

Therefore the names are different and both fields can be converted to a new input to be actually posted to the controller.

I chose not to convert the first fieldname[] to fieldname[0] because altering this, I could not reference to the fieldname within the same set of data and this needed more javascript code. This is not necessary for PHP which receives fieldname[] as 0 and fieldname[1] as 1.

Used let/const because they are supported in IE11 and don't interfere with scope of the function.

Manual testing scenarios (*)

• Extend the Store.php form block with a multivalue field. See: Website/Group/Store entity form can't be extended with multiple value form element for extension attributes #24070
  ** The submitted data in validate-store.js will converted a key/value object A new html form will be generated with of the previous object entries.
• This form is posted to the controller.
• Current postdata fields won't be altered, multivalue fields will be submitted, before only the last element gets posted.

[m2-assistant]

Hi @ericclaeren. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[magento-cicd2]

All committers have signed the CLA.

[ericclaeren]

Hi @VladimirZaets, have fixed the variable declarations and ran into js linting errors which I have resolved. Would you mind reviewing my code again? Thanks

[VladimirZaets]

Thanks for fixes.

[magento-engcom-team]

Hi @VladimirZaets, thank you for the review.
ENGCOM-5604 has been created to process this Pull Request
✳️ @VladimirZaets, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[magento-engcom-team]

@ericclaeren thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[ericclaeren]

@VladimirZaets The purpose of this change is untested by auto-test.

Although Magento Core has a jasmin JS test for this validate function, there isn't a test for the _saveHandler() method. See: validate-store.test.js. So the whole core JS functionality isn't being tested right now.

[ericclaeren]

@magento-engcom-team The label has been added.

[engcom-Delta]

Hi @ericclaeren seems like issue described by PR wasn't fixed.
Manual testing scenario:

• Create Observer for adminhtml_store_edit_form_prepare_form
• Add field with multiselect.

     * @param Observer $observer
     */
    public function execute(Observer $observer): void
    {
        $block = $observer->getEvent()->getBlock();
        $form = $block->getForm();
        $fieldSettings = [
            'name' => 'store[multiple][]',
            'label' => __('Multiselect field'),
            'value' => '',
            'values' => [
                ['value' => 'value1', 'label' => 'testLabel1'],
                ['value' => 'value2', 'label' => 'testLabel2']

            ],
            'required' => false,
            'disabled' => false,
        ];

        $fieldset = $form->addFieldset('group_multiselect', ['legend' => __('Multiselect fieldset')]);
        $fieldset->addField('multiselectElement', 'multiselect', $fieldSettings);
        $observer->setData('block', $block->setForm($form));

• Select all values from multiselect

• Save form and check headers of postdata.

Result:
❌ Only last value from multiselect was sent in POST

[ericclaeren]

That's really weird, are you sure the correct (patched) validation-store.js code is loaded.

Some screens from my code. The observer is pretty much the same, although my data are store Ids.

Multiselect

console.table(formData) of validate-store.js

Request recorded in Google Chrome.

As you may see in the screenshot both values are posted. I have a working / functioning module with this patch.

Could you explain how you evaluated your code?

[sidolov]

@engcom-Delta please, take a look a comment from @ericclaeren

[engcom-Charlie]

@ericclaeren please sign CLA. Otherwise, we can't proceed with your PR.
Thank you.

[engcom-Charlie]

@ericclaeren please look at #24094 (review)
Thank you.

[engcom-Charlie]

@magento run all tests

[engcom-Dash]

@magento run all tests

[engcom-Dash]

As code review, testing is already completed on this PR. Also the Adobe CLA is signed now. Moving this PR in Extended Testing.

[ericclaeren]

I'm not able to review this issue, I have not worked on a Magento website in the last 4 years and have no means to test any of the requested features.

[engcom-Dash]

Hello @ericclaeren

Thank you for your contribution.

We’ve verified that the issue is still reproducible, and we can confirm that the solution proposed in this PR effectively resolves it. We’ll be taking it forward from here and proceeding with the necessary steps to get it merged.

Appreciate your efforts!

Thanks!

[engcom-Dash]

@magento run all tests

[engcom-Dash]

@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE, Static Tests, Unit Tests

[engcom-Dash]

The consistent test failures for Functional B2B are known Issues and JIRA is raised for them.Other failures are inconsistent and seems to be flaky. They neither part of PR nor failing because of the PR changes.

Build 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/ec0fbee6f4a35121fcbcac8ef3d21f40/Functional/allure-report-b2b/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Build 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/91ee2b583a5b9b599405f0d4cc881828/Functional/allure-report-b2b/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Known Issues:
EnablePaypalExpressCheckoutAndSubmitAnOrderUsingPaypalExpressCheckoutTest : ACQE-8007
StoreFrontSimpleProductWithSpecialAndTierDiscountPriceTest : ACQE-7971
OrderWithMultipleConfigurableProductsAdditionalStockTest : ACQE-8060

The consistent test failures for Functional CE is known Issues and JIRA is raised for the same.

Build 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/7a14a3f03d8eee6c82317625d6f2a878/Functional/allure-report-ce/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Build 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/848d19ce28e8f4b485bb1a15c5f9e8ea/Functional/allure-report-ce/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Known Issues:
EnablePaypalExpressCheckoutAndSubmitAnOrderUsingPaypalExpressCheckoutTest : ACQE-8007

The consistent test failure for Functional EE is known Issues and JIRA is raised for the same. Other failures are inconsistent and seems to be flaky. They neither part of PR nor failing because of the PR changes.

Build 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/7d7815d10fa81049299e03f5edb44afc/Functional/allure-report-ee/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Build 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/d26ce05b519889ad04a681dc340fa750/Functional/allure-report-ee/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Known Issues:
EnablePaypalExpressCheckoutAndSubmitAnOrderUsingPaypalExpressCheckoutTest : ACQE-8007

Static Test Failure
The consistent static test failure is not because of this code changes done in this PR. It is a known issue and has JIRA Opened for the same.

Build 1: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/5de292251065c749128833c2e6ad4b12/Statics/allure-report-b2b/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Build 2: https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/24094/5d12c6d2f8a016fe3500d4481c40b0a5/Statics/allure-report-b2b/index.html#categories/8fb3a91ba5aaf9de24cc8a92edc82b5d

Known Issue:
"use strict" is unnecessary inside of modules: https://jira.corp.adobe.com/browse/AC-14517.

Hence moving this PR in Merge In Progress.

[engcom-Dash]

@magento run all tests