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: .map(Number) creates problems if preselect is not an id but a string #206

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

MercuryKojo
Copy link
Contributor

this checks if filterGroups in fieldConfig contains 'relation' and only maps to Number. This could still cause an issue if a field contains IDs and strings.
I couldn't find a use case for this, but something to think about. Another solution would be to cast the key in FilterGroupHelper->getGroupValuesForFilterGroup to string and we can remove the map function altogether. The function is only used in the pimcore_ecommerceframework_index_getvaluesforfilterfield route and therefore should not cause other problems

…ring

this is checking if filterGroups in fieldConfig contains 'relation' and only then maps to Number. This could still cause an issue, if a field contains ids and strings. 
I couldn't find a usecase for this, but something to think about.
Another solution would be to cast the key in FilterGroupHelper->getGroupValuesForFilterGroup to string and we can remove the map function altogether. Function is only used in the pimcore_ecommerceframework_index_getvaluesforfilterfield route and therefor should not cause other problems
@mattamon
Copy link
Contributor

Hey @MercuryKojo thanks for the PR .

Could you maybe also describe a way to reproduce it?

@MercuryKojo
Copy link
Contributor Author

Hey @MercuryKojo thanks for the PR .

Could you maybe also describe a way to reproduce it?

Sure thing:
https://demo.pimcore.com/admin/login/deeplink?object_563_object

add a FilterMultiSelect Precondition
select field 'Name' for example (any field with strings as values)
preselect a Value -> at the moment there seems to be a loading animation which won´t go away, but if you scroll a little you can still select one
save and reload
preselect is gone because it tries to map a string to a number in Javascript

@mattamon mattamon added the Bug label Oct 23, 2024
@lukmzig lukmzig added this to the 1.2.2 milestone Oct 23, 2024
@mattamon
Copy link
Contributor

@MercuryKojo Thank you!

I do get it now and it was introduced here #194.
Sorry for that.

Your solution works fine, but maybe since it is 4 times the same code of getting the data out of the preselect, we could introduce a method for this and then just call the method inside of setValue?

@mattamon
Copy link
Contributor

@MercuryKojo Thank you!

I do get it now and it was introduced here #194. Sorry for that.

Your solution works fine, but maybe since it is 4 times the same code of getting the data out of the preselect, we could introduce a method for this and then just call the method inside of setValue?

Alternatively I can do it too for you since I already would have it preprared. WDYT?

@MercuryKojo
Copy link
Contributor Author

@MercuryKojo Thank you!
I do get it now and it was introduced here #194. Sorry for that.
Your solution works fine, but maybe since it is 4 times the same code of getting the data out of the preselect, we could introduce a method for this and then just call the method inside of setValue?

Alternatively I can do it too for you since I already would have it preprared. WDYT?

would be apreciated

@mattamon
Copy link
Contributor

@MercuryKojo Thank you!
I do get it now and it was introduced here #194. Sorry for that.
Your solution works fine, but maybe since it is 4 times the same code of getting the data out of the preselect, we could introduce a method for this and then just call the method inside of setValue?

Alternatively I can do it too for you since I already would have it preprared. WDYT?

would be apreciated

Done! :) Could you confirm that this still works for you?

@mattamon mattamon requested a review from lukmzig October 24, 2024 15:03
@MercuryKojo
Copy link
Contributor Author

@MercuryKojo Thank you!
I do get it now and it was introduced here #194. Sorry for that.
Your solution works fine, but maybe since it is 4 times the same code of getting the data out of the preselect, we could introduce a method for this and then just call the method inside of setValue?

Alternatively I can do it too for you since I already would have it preprared. WDYT?

would be apreciated

Done! :) Could you confirm that this still works for you?

sry for the late reply
can confirm, still works for me

@mattamon mattamon merged commit c3223f1 into pimcore:1.2 Oct 31, 2024
2 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants