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

FIX SearchableDropdownField pass incorrect value on onChange event #1736

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova force-pushed the pulls/2.2/fix-serchabledropdownfield branch from 5747604 to 50bc410 Compare May 6, 2024 01:11
Comment on lines 136 to 144
if (typeof val !== 'object') {
options.forEach(option => {
// eslint-disable-next-line eqeqeq
if (option.value == value) {
val = option;
}
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-select expect to get value as an array of objects of ValueType in the following format { label: 'label', value: 'value' }. See discussion here.
FormBuilder.buildComponent() set onChange props as function with 2 arguments (event and payload), but handle payload value first and redux-form set this payload value in the state. See
After we render SearchableDropdownField again we get integer value from state instead of object. So, if we pass only value that is returned back from react-select (See) then we process it and store it as an Object.toString() and get [object.Object] in view. If we pass two params then we get integer or any other value, but not object, after processing.

@sabina-talipova sabina-talipova force-pushed the pulls/2.2/fix-serchabledropdownfield branch from 2daaf21 to 038173c Compare May 7, 2024 00:27
@sabina-talipova
Copy link
Contributor Author

I left comment in the issue: #1733 (comment)

@GuySartorelli
Copy link
Member

This PR seems to work without any problems for me.

In #1733 (comment) you mentioned "this requires more careful study than simply fixing a tag display problem" - but I don't understand what problems are left?
Does this PR introduce new regressions? Or are there other separate problems which also need to be fixed separately?

@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented May 8, 2024

That's right, this PR solves the current problem and, as far as I was able to test, it does not cause regression problems.

I will try to slightly reformulate my doubts about the correct operation of SearchableDropdownField expressed in the comments.

SearchableDropdownField works great everywhere except for search or where we need to pass a specific value (number or string).
Since react-select accepts an object or an array of objects as a “props.value” and return specific object onChange event, SearchableDropdownField through FormBuilder stores the object in redux state, from where we later retrieve this data before sending it to the server.

Everything works fine if we process the data before sending it to the server, as for example in the case of an asset-admin and users who are allowed to view. But not in the case of searching, since the data is not processed in any way before sending and we simply transfer the object.

The problem occurs in $.ajax, where I assume obj.toString() is applied to the object instead of JSON.stringify(obj). Therefore, we send "[objectObject]" to the server instead of "{id: 1, value: something}". But in any case, the server expects to receive some specific value (a number or a string), but not an object. Therefore, we should make changes to the search function on the backend.

Also, after the server returns the response, we will store the value “[objectObject]” in state, which in turn will later be displayed in the CMS elements.

So I believe the problem is a little deeper than just SearchableDropdownField. That is, I think that we should analyse where and how this field can be used and what data it will contain. You should also consider what type of data react-select accepts. Since in the future we plan to change the approach to working using Redux, this should also be given attention.

This PR simply stores in state only the value for a single value field and the entire array for a multiple choice field. That is, I think if someone decides to use SearchableDropdownField with multiple choice as a filter for searching, then it will not work.

@GuySartorelli
Copy link
Member

Sorry, that went a bit too deep into the weeds (too much specific technical detail) for me to be able to easily understand what problems remain.

But not in the case of searching, since the data is not processed in any way before sending and we simply transfer the object.

The problem occurs in $.ajax, where I assume obj.toString() is applied to the object instead of JSON.stringify(obj). Therefore, we send "[objectObject]" to the server instead of "{id: 1, value: something}". But in any case, the server expects to receive some specific value (a number or a string), but not an object. Therefore, we should make changes to the search function on the backend.

The above suggests there's still some problem related to SearchableDropdownField in relation to searching - but that's exactly what this PR fixes, isn't it?

You should also consider what type of data ReactSelect accepts. Since in the future we plan to change the approach to working using Redux, this should also be given attention.

Are there any specific problems related to this that you're aware of?

This PR simply stores in state only the value for a single value field and the entire array for a multiple choice field. That is, I think if someone decides to use SearchableDropdownField with multiple choice as a filter for searching, then it will not work.

That's a good point. I've just tested that using the following code:

<?php

namespace App\Extension;

use SilverStripe\Core\Extension;
use SilverStripe\Forms\SearchableMultiDropdownField;
use SilverStripe\Security\Member;

class GroupSearchableFieldsExtension extends Extension
{
    public function updateSearchableFields(array &$fields)
    {
        $fields['Members'] = [
            'title' => 'Users',
            'field' => SearchableMultiDropdownField::create('Members', 'Users', Member::get()),
        ];
    }
}
SilverStripe\Security\Group:
  extensions:
    - App\Extension\GroupSearchableFieldsExtension

Searching with that field does indeed give a bad result - specifically if I select two records I get [object+Object],[object+Object] in the request, which results in an exception. That should be easy to remedy in this PR.

@GuySartorelli
Copy link
Member

Are there any specific problems outstanding beyond that? As in specific problems that cause errors or bad UX, which can be easily reproduced?

@sabina-talipova
Copy link
Contributor Author

@GuySartorelli , how I said before, the current PR fixes initial issue. For another problems that were discussed in comments I've opened new ticket #1746

Are there any specific problems outstanding beyond that? As in specific problems that cause errors or bad UX, which can be easily reproduced?

No, I couldn't find any problems. I would be good to test this field created by FormBuilder.

@GuySartorelli
Copy link
Member

I've updated that issue to reflect what we discussed in slack - the main problem is that many_many relations can't be searched against, and fixing that is necessary for using multiple values in SearchableMultiDropdownField to be useful.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, works well locally for single values and there's a separate issue to deal with multi values since that's a more involved problem.

@GuySartorelli GuySartorelli merged commit ca4d75b into silverstripe:2.2 May 13, 2024
12 checks passed
@GuySartorelli GuySartorelli deleted the pulls/2.2/fix-serchabledropdownfield branch May 13, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants