User recommendation command #31

OndraM · 2017-11-24T16:00:49Z

~~Not yet finished (+ with broken tests right now). However I'd like some feedback for the public API of the command.~~

=> see comments for resolution

Outdated issue description:

The most basic case would look like this:

$response = $matej->request()
    ->recommendation(UserRecommendation::create('user-id', 10, 'scenario', 1.0, 3600, false))
    ->send();

But still the constructor has so many required params... Maybe at least the hard rotation could be optional (and default to false?). The other values must be explicitly defined and there is no "smart default", if I'm not mistaken, right?

And when you will need more customized recommendation, you will have to call setters on the UserRecommendation object:

$recommendationCommand = UserRecommendation::create('user-id',10, 'scenario', 1.0, 3600, false);
$recommendationCommand->setFilter('item_id != 333');
$recommendationCommand->setMinimalRelevance(UserRecommendation::MINIMAL_RELEVANCE_HIGH);

$response = $matej->request()
   ->recommendation($recommendationCommand)
   ->send();

What do you think?

OndraM · 2017-11-24T16:04:22Z

Also the filters could be defined as an array of strings, and concatenated to output string only on build(). This will allow simpler manipulation with the default filter, adding custom filters atd.

Or could the default filter be defined as standalone property - what would allow to simply overwrite it eg. through RequestManager.

jirinovak · 2017-11-24T22:28:43Z

src/Model/Command/UserRecommendation.php

+     */
+    public function setMinimalRelevance(string $minimalRelevance): self
+    {
+        // TODO: assert one of MIN_RELEVANCE_*


Maybe 3 setter should be provided e.g. setLowMinimalRelevance etc. and no validation will be needed.

Maybe... If you find this interesting, we may add it later (or send a PR :) ), it would be quite easy to implement. However we are a bit on time pressure for MVP :).

jirinovak · 2017-11-24T22:30:44Z

src/Model/Command/UserRecommendation.php

+    /** @var string */
+    private $minimalRelevance = self::MINIMAL_RELEVANCE_LOW;
+    /** @var string */
+    private $filter = 'valid_to >= NOW';


Isn`t this client specific?

valid_to must be present in item properties.

Or I am wrong?

Yes, its client specific.

Another argument is that 'filters' are client specific too (afaik all of them are still hardcoded).

So I suggests no default filter.

Yes, it is client specific, but:

all but one client ;-) will use it as for now, so we can make life easier for the vast majority by defining the default filter like this;

the one who does not use this default filter (but custome one), will anyway have to define the filter - so there will be no difference for him (define custom filter vs. overwrite default filter will be the same amount of work).

From the beginning we planned to have a simple way how to overwrite the default filter for all recommendations and sorting - so there will be no extra work for those not using the default filter. But for those who do use the default we will make the library adoption simpler, IMO.

Also, quoting Matej official docs:

Matej supports following filter: valid_to >= NOW which should be used in each recommendation request.

I will talk about this issue with RAD team and we may change this behavior in the future, however, having this feature in the API client was partially their idea ¯\_(ツ)_/¯.

There will be extra work for those who do not use default filter at all. They have to reset it. Or they end up with invalid request i guess.

It still seem weird to me to have default filter in such general library.

No, they will not have to explicitly reset it. They will have to set custom filter anyway (and this will override the default one - if any). So no extra work for them. Because AFAIK there is no meaningful usecase for requesting recommendations without some kind of similar default filter.

I agree. It might be useful though to have a way how to set the default filter for the customers use-case.

How about having a static variable that'll be the default value?

UserRecommendation::$DEFAULTS['filter'] = null;

How do other libs solve this?

jirinovak · 2017-11-24T22:48:39Z

The other values must be explicitly defined and there is no "smart default", if I'm not mistaken, right?

Maybe smart default should also have rotationRate and rotationTime. Those default values may be provided by Matěj crew.

Also the filters could be defined as an array of strings...

I think filters as an array would be better.

Or could the default filter be defined as standalone property - what would allow to simply overwrite it eg. through RequestManager.

I don`t think we could find default filter that fits all clients. See my comment in review.

OndraM · 2017-11-26T11:56:15Z

@jirinovak And what would you suggest as default value for rotation rate and rotation time?

The main criteria whether this should be required in constructor or customisable via setter is the the ratio of "creating request with the default values" vs "creating request with custom values". If you - for example - use in 90 % of cases the defaults, lets do it via setters. But if the ratio is 50:50 or so, maybe the cunstructor params is a better and more transparent way.

jirinovak · 2017-11-26T19:36:21Z

As I said maybe the RAD team could provide smart defaults.

OndraM · 2017-11-26T20:21:49Z

So cc @foglcz - any suggestions :)?

OndraM · 2017-11-27T11:41:58Z

Ok, so:

the default constructor requires just userId, recommendation count and scenario
default value is used for all other properties with setters available for them
I can imagine we may add in future eg. second contructor which will allow setting all properties as part of the instantiation (::createConfigured($userId, $count, $scenario, $rotationRate, $rotationTime, $hardRotation, $minimalRelevance, $filters - if someone will find this useful.
If you won't use the default filter, you will simple call setFilters([...]) to overwrite it. Some simple way to set default filters for all Recommendations and Sortings may be added in the future.

Basic usage with default values

$response = $matej->request()
    ->recommendation(UserRecommendation::create('user-id', 10, 'scenario'))
    ->send();

=> this will use the default values:

rotation_rate' => 1.0,
'rotation_time' => 3600,
'hard_rotation' => false,
'min_relevance' => 'low',
'filter' => 'valid_to >= NOW',

Advanced usage with custom values

$recommendationCommand = UserRecommendation::create('user-id', 10, 'scenario');
$recommendationCommand->setRotationRate(0.1337)
    ->setRotationTime(666)
    ->setMinimalRelevance(UserRecommendation::MINIMAL_RELEVANCE_HIGH)
    ->enableHardRotation()
    ->setFilters(['foo = bar', 'AND baz = ban']);

$response = $matej->request()
    ->recommendation($recommendationCommand)
    ->send();

kubasimon · 2017-11-27T12:39:13Z

I'm missing 'allowNonexistent' parameter - not part of this pull request?

kubasimon · 2017-11-27T12:42:27Z

And with current "smart defaults" is client able to omit some parameters (e.g. rotationRate/rotationTime etc) with $command->setRotationTime(null)?

It's sometimes useful for some A/B tests on Matej side.

foglcz · 2017-11-27T12:42:38Z

Got up to date feedback from RAD

Rotation rate & time - request specific - should be part of constructor (ie. 2 weeks on mailing, 1 hour on serp)
Minimal relevance - let's keep it as MINIMAL_RELEVANCE_LOW as default, as that will always return something. Some clients might want to use MEDIUM as min relevance, would add it as optional parameter of UserRecommendation::create
Hard rotation & filter - can be as spec'ed

foglcz · 2017-11-27T12:44:57Z

For filters, I'd personally refrain from using ->addFilter('AND some_filter') notation, as that's a bit counter intuitive.

Can we keep the array, but do a $filter = join($this->filterSeparator, $this->filters) where $this->filterSeparator = 'AND' && $this->filters = ['foo = bar', 'baz = ban'] ?

So that dev can use:

$recommendationCommand = UserRecommendation::create('user-id', 10, 'scenario')
    ->setFilters(['foo = bar', 'baz = ban'])
    ->addFilter('bar = none')
;

OndraM · 2017-11-27T12:47:47Z

@tenerd allowNonexistent is not part of API v2. I'm not sure what is recommended way of handling this case in the API v2 - but I guess RAD team will advice if you write them.

Also - all parameters are required in the API request according to JSON schema. And eg. the value of rotation rate must be like this: "type_rotation_rate": { "type": "number", "minimum": 0, "maximum": 1.0 },. So you cannot omit the param, nor set its value to null.

OndraM · 2017-11-27T13:21:19Z

Ok, I updated the PR - rotationRate and rotationTime are now required in the constructor, without any default value.

Also the filters are now concatenated using filterOperator. However I kept this as protected, so one can eg. do custom implementation, what about that?

davidkmenta · 2017-11-27T14:18:19Z

src/Model/Command/UserRecommendation.php

+    }
+
+    /**
+     * Even with rotation rate 1.0 user could still obtain the same recommendations in some * edge cases.


The asterisk between the some and edge words has any reason?

Yes, it has! It serves as an example of copy paste error 🙃 .

I will fix this, thanks for noticing.

ghost · 2017-11-27T14:23:52Z

src/Model/Command/UserRecommendation.php

+     * To prevent this, enable hard rotation - recommended items are then excluded until rotation time is expired.
+     * By default hard rotation is not enabled.
+     */
+    public function enableHardRotation(): self


Why not setHardRotation(bool $hardRotation)?

It thought this is easier to read, don't you think?

Yes, but you can't change the value back to false.

Why would you need this?

And even if someone would need this, we can rather do disableHardRotation()...

OndraM requested review from kubasimon, davidkmenta, foglcz, jirinovak, kdosiodjinud, MortalFlesh, hokypierce, MarketaSebkova, florianjiri and a user November 24, 2017 16:00

jirinovak reviewed Nov 24, 2017

View reviewed changes

OndraM added this to the 0.9 milestone Nov 26, 2017

OndraM force-pushed the feature/user-recommendation-command branch from 7c22d8c to a2f2edc Compare November 27, 2017 11:31

OndraM changed the title ~~User recommendation command [WIP] [RFC]~~ User recommendation command Nov 27, 2017

jirinovak previously approved these changes Nov 27, 2017

View reviewed changes

OndraM dismissed jirinovak’s stale review via a5d38a9 November 27, 2017 13:20

foglcz previously approved these changes Nov 27, 2017

View reviewed changes

davidkmenta reviewed Nov 27, 2017

View reviewed changes

ghost previously approved these changes Nov 27, 2017

View reviewed changes

jirinovak previously approved these changes Nov 27, 2017

View reviewed changes

OndraM dismissed stale reviews from jirinovak, ghost , and foglcz via 52f486f November 27, 2017 14:47

foglcz previously approved these changes Nov 27, 2017

View reviewed changes

OndraM dismissed foglcz’s stale review via c49b243 November 27, 2017 15:57

OndraM force-pushed the feature/user-recommendation-command branch from a39c797 to c49b243 Compare November 27, 2017 15:57

Add user-based recommendation command (fixes #25)

bd7a0fb

OndraM force-pushed the feature/user-recommendation-command branch from c49b243 to bd7a0fb Compare November 27, 2017 15:59

OndraM merged commit c36796b into master Nov 27, 2017

OndraM deleted the feature/user-recommendation-command branch November 27, 2017 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User recommendation command #31

User recommendation command #31

OndraM commented Nov 24, 2017 •

edited

Loading

OndraM commented Nov 24, 2017 •

edited

Loading

jirinovak Nov 24, 2017

OndraM Nov 27, 2017

jirinovak Nov 24, 2017 •

edited

Loading

kubasimon Nov 25, 2017

OndraM Nov 25, 2017 •

edited

Loading

jirinovak Nov 26, 2017 •

edited

Loading

OndraM Nov 26, 2017 •

edited

Loading

foglcz Nov 27, 2017

jirinovak commented Nov 24, 2017

OndraM commented Nov 26, 2017

jirinovak commented Nov 26, 2017

OndraM commented Nov 26, 2017

OndraM commented Nov 27, 2017

kubasimon commented Nov 27, 2017

kubasimon commented Nov 27, 2017 •

edited

Loading

foglcz commented Nov 27, 2017

foglcz commented Nov 27, 2017

OndraM commented Nov 27, 2017 •

edited

Loading

OndraM commented Nov 27, 2017

davidkmenta Nov 27, 2017

OndraM Nov 27, 2017

ghost Nov 27, 2017

OndraM Nov 27, 2017

ghost Nov 27, 2017

OndraM Nov 27, 2017

User recommendation command #31

User recommendation command #31

Conversation

OndraM commented Nov 24, 2017 • edited Loading

=> see comments for resolution

Outdated issue description:

OndraM commented Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirinovak Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraM Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

jirinovak Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

OndraM Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirinovak commented Nov 24, 2017

OndraM commented Nov 26, 2017

jirinovak commented Nov 26, 2017

OndraM commented Nov 26, 2017

OndraM commented Nov 27, 2017

Basic usage with default values

Advanced usage with custom values

kubasimon commented Nov 27, 2017

kubasimon commented Nov 27, 2017 • edited Loading

foglcz commented Nov 27, 2017

foglcz commented Nov 27, 2017

OndraM commented Nov 27, 2017 • edited Loading

OndraM commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraM commented Nov 24, 2017 •

edited

Loading

OndraM commented Nov 24, 2017 •

edited

Loading

jirinovak Nov 24, 2017 •

edited

Loading

OndraM Nov 25, 2017 •

edited

Loading

jirinovak Nov 26, 2017 •

edited

Loading

OndraM Nov 26, 2017 •

edited

Loading

kubasimon commented Nov 27, 2017 •

edited

Loading

OndraM commented Nov 27, 2017 •

edited

Loading