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

Fixed bug when recommendations were returned as strings. #123

Merged
merged 2 commits into from Sep 24, 2020

Conversation

JakubTesarek
Copy link
Contributor

This fixes a bug caused by special behaviour of Matej.

When sending RecommendationCommand, Matej returns one of 2 types of responses. If the request or used scenario specifies item properties to get, it returns list of objects where there's always a field item_id present. This is true even if the list of properties is empty.

For backwards compatibility, Matej returns list of strings instead of objects if the request doesn't contain item_properties field and no scenario is used.

When Matej collects fields to return, it first looks into the request and if there's no item_properties field, it looks into the configured scenario. In version 2.* matej-client-php used to always send this field so users never experienced this bug. in #113 we removed that argument and we send it only when user specifies item properties to get.

This is a temporary bugfix until Matej unify the behaviour of the recommendation request. It will not be necessary afterwards.

VincTheSecond
VincTheSecond previously approved these changes Sep 23, 2020
Copy link
Member

@VincTheSecond VincTheSecond left a comment

Choose a reason for hiding this comment

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

Otestoval jsem vsechny mozne kombinace properties vs. konfiguraci scenaru a klient vzdy spravne vratil pole dictu. ✅

Na samotny commit review udelat neumim. :-)

Copy link
Member

@OndraM OndraM left a comment

Choose a reason for hiding this comment

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

Also please don't forget to look at those codestyle violations reported by the CI.

src/Model/RecommendationCommandResponse.php Show resolved Hide resolved
src/Model/Response/RecommendationsResponse.php Outdated Show resolved Hide resolved
tests/unit/Model/Response/RecommendationsResponseTest.php Outdated Show resolved Hide resolved
@JakubTesarek
Copy link
Contributor Author

Also please don't forget to look at those codestyle violations reported by the CI.

What violations? I didn't get any violations locally and CI report is all green except one that ran out of memory.

@OndraM
Copy link
Member

OndraM commented Sep 24, 2020

What violations? I didn't get any violations locally and CI report is all green except one that ran out of memory.

Yup, sorry, you are right - there are no errors in this run, however the codestyle check was not working properly (I already fixed in master), so it will report some issues once you rebase this branch with master and run composer update. For example there are few missing newlines at the end of files etc.

Copy link
Member

@OndraM OndraM left a comment

Choose a reason for hiding this comment

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

👍

@JakubTesarek JakubTesarek merged commit 360d62f into master Sep 24, 2020
@OndraM OndraM deleted the fix-recommendation-response branch September 25, 2020 10:03
OndraM pushed a commit to OndraM/matej-client-php that referenced this pull request Dec 11, 2020
Fixed bug when recommendations were returned as strings.
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.

None yet

3 participants