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

Can not select multiple statuses with Prototype 1.7.3 in Reports #1549

Closed
luigifab opened this issue Apr 15, 2021 · 6 comments
Closed

Can not select multiple statuses with Prototype 1.7.3 in Reports #1549

luigifab opened this issue Apr 15, 2021 · 6 comments
Labels

Comments

@luigifab
Copy link
Contributor

With the update of Prototype 1.7.3, we can not select multiple order statuses in Reports.

Steps to reproduce

  1. Go to Reports / Sales / Coupon
  2. Select more than one order status
  3. Run in console: Form.serializeElements($$('#sales_report_order_statuses'))
  4. Try to show report
  5. Only one status is selected

Results

  1. With prototype 1.7: "order_statuses%5B%5D=closed%2Ccomplete"
  2. With prototype 1.7.3: "order_statuses%5B%5D=closed&order_statuses%5B%5D=complete"
    It break Mage_Adminhtml_Block_Report_Filter_Form::_initFormValues.

Perhaps we can fix with:

    protected function _initFormValues()
    {
        $data = $this->getFilterData()->getData();
        foreach ($data as $key => $value) {
            if (is_array($value) && isset($value[0]) && (count($value) == 1)) {
                $data[$key] = explode(',', $value[0]);
            }
        }
        $this->getForm()->addValues($data);
        return parent::_initFormValues();
    }
@luigifab luigifab added the bug label Apr 15, 2021
@addison74
Copy link
Contributor

I did all your steps except the third. This is an OpenMage 20.0.8 installed from scratch based on the sample package from Magento 1.9. Here are the results:

Before pressing [Show Report] button

before

After pressing [Show Report] button

after

The bug is confirmed! It does not appear in version 20.0.6 that I am currently working on.

I suggest immediately replacing the Prototype library with the previous version and carefully checking version 1.7.3 compared to 1.7 before adding to the new versions to avoid any issue.

@luigifab - I appreciate for your help.

@Flyingmana
Copy link
Contributor

Iam also in favor of reverting it for now.
Does it happen on a specific browser, or in general?
Does someone volunteer to submit the issue upstream to prototype?

@kiatng
Copy link
Contributor

kiatng commented Apr 16, 2021

Ref this commit Add test and changes for serializing forms with multiple select to a string to work again in prototype repo, the serialization of multiple select is supposed to be like this

this.assertEqual("peewee=herman&colors=blue&colors=yellow&colors=not+grey&number=2", form.serialize(false));

A use case for array insead of returning comma-delimted string is when the value contains comma, which will be wrongly parsed.

The title of the commit says to make it work again, it so happens we can test that in version 1.6:

image

Note the popup is hobbies=coding&hobbies=swimming. Also take a look at the end of the screenshot:

Keep in mind that "hobbies" multiple select should really be named "hobbies[]" if we're posting to a PHP or Ruby on Rails backend because we want to send an array of values instead of a single one.

For this issue, the multiple select element is correcly named order_statuses[].

So, if that is the case, what do we do? Revert prototype and re-introduce the "bug", or fix OM?

@addison74
Copy link
Contributor

If someone makes it quick I suggest fixing OM and checking the new library too. A comparison with the previous version we used in OM for a long time is a must.

@colinmollenhour
Copy link
Member

I reverted the change to the serialize behavior so it is compatible with the old Prototype without reverting the entire file in #1552

@addison74
Copy link
Contributor

Now that we have a quick fix that works maybe we should consider whether the initial form of the Prototype library brings real improvements. I am of the opinion that if it brings improvements then it should be used as such, following that the change will happen in the OpenMage code as suggested @luigifab suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants