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

[SearchBundle][ProductBundle] Add multilingual in elasticsearch engine and check on product availability #4525

Merged
merged 11 commits into from
Mar 31, 2016

Conversation

Niiko
Copy link

@Niiko Niiko commented Mar 16, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Related tickets fixes #4312
License MIT
Doc PR -

Here is my work on multilingual search with elasticsearch engine.

Here is my fos_elastica configuration

fos_elastica:
     clients:
         elasticsearch:
            servers:
                - { host: 127.0.0.1, port: 9200, logger: true }
     indexes:
         sylius:
             client: elasticsearch
             finder: ~
             settings:
                 analysis:
                    analyzer:
                        custom_search_analyzer:
                            type: custom
                            tokenizer: standard
                            filter   : [lowercase, asciifolding, synonym]
                        custom_index_analyzer:
                            type: custom
                            tokenizer: standard
                            filter   : [lowercase, asciifolding, synonym, custom_filter]
                    filter:
                        custom_filter:
                            type: nGram
                            min_gram: 1
                            max_gram: 20
                            token_chars: [letter, digit]
                        elision:
                            type:     elision
                            articles: [l, m, t, qu, n, s, j, d]
                        synonym:
                            type: synonym
                            synonyms: synonym.txt
             types:
                 product:
                     search_analyzer: custom_search_analyzer
                     index_analyzer: custom_index_analyzer
                     mappings:
                         id: { type: integer, index: not_analyzed }
                         name: { type: string }
                         description: { type: string }
                         shortDescription: { type: string }
                         slug: { type: string, index: not_analyzed }
                         translations:
                             type: nested
                             properties:
                                 locale: { type: string, index: not_analyzed }
                                 name: { type: string }
                                 metaKeywords: { type: string }
                                 metaDescription: { type: string }
                         masterVariant:
                             type: nested
                             properties:
                                 sku: { type: string }
                                 availableOn: { type: date, index: not_analyzed }
                                 availableUntil: { type: date }
                                 availableOnDemand: { type: boolean }
                                 presentation: { type: string }
                                 onHand: { type: integer, index: not_analyzed }
                                 onHold: { type: integer, index: not_analyzed }
                         variants:
                             type: nested
                             properties:
                                 sku: { type: string }
                                 availableOn: { type: date, index: not_analyzed }
                                 availableUntil: { type: date }
                                 availableOnDemand: { type: boolean }
                                 presentation: { type: string }
                                 onHand: { type: integer, index: not_analyzed }
                                 onHold: { type: integer, index: not_analyzed }
                         attributes:
                             type: nested
                             properties:
                                 value: { type: string }
                         channels: { type: string, index: not_analyzed }
                         taxons: { type: string, index: not_analyzed }
                         enabled: { type: boolean }
                     persistence:
                         driver: orm
                         model: Sylius\Component\Core\Model\Product
                         provider: ~
                         listener: ~
                         finder: ~

In order to work, the product type in fos_elastica config has to have properties : masterVariant, variants and enabled. So, if accepted, i can PR a minimum configuration for Sylius-docs.

I had also a check on availability of products with three checks :

  • AvailableOn property of variant/masterVariant has to be lower than "now"
  • AvailableUntil property of variant/masterVariant has to be null OR greater than "now"
  • AvailableOnDemand property of variant/masterVariant has to be defined as true OR onHand property greater than 0

I have a question on last one, can we consider if onHand > 0 means product is available, or onHand - onHold > 0 ?

@okwinza
Copy link
Contributor

okwinza commented Mar 17, 2016

Overall its 👍 but IMO the title is a bit misleading since there is more than just multilingual support.

$engine = null;
foreach ($syliusSearchConfigs as $syliusSearchConfig) {
if (isset($syliusSearchConfig['engine'])) {
$engine = $syliusSearchConfig['engine'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you add break; in this if? Cause now last one wins, with break first one wins.

@Niiko Niiko changed the title [SearchBundle][ProductBundle] Add multilingual in elasticsearch engine [SearchBundle][ProductBundle] Add multilingual in elasticsearch engine and check on product availability Mar 17, 2016
@Niiko
Copy link
Author

Niiko commented Mar 17, 2016

@okwinza You're right, changed title according to that

$elasticaProductListenerDefinition->addArgument(new Reference('fos_elastica.object_persister.' . $index . '.product'));
$elasticaProductListenerDefinition->setTags($tags);

$container->setDefinition('sylius_product.listener.product_update', $elasticaProductListenerDefinition);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong, cause every index will overwrite the definition "over & over", so if you have two, it will write for both, but second one will overwrite definition of the first one.

Copy link
Author

Choose a reason for hiding this comment

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

Right, i can add a check on that to add only one time.

[Edit] i refactor that definition to avoid extra loops, and wrong service definition (overwrite)

@pjedrzejewski
Copy link
Member

Product bundle should not depend on SyliusSearchBundle. Can we somehow decouple it in this PR?

}

/**
* Add available products filter for variants (or master variant)
Copy link
Member

Choose a reason for hiding this comment

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

Redundant comment

@@ -166,4 +173,47 @@ private function prependVariation(ContainerBuilder $container, array $config)
],
]);
}

/**
* Add elastica product listener
Copy link
Member

Choose a reason for hiding this comment

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

Remove all this redundant comments

@Niiko
Copy link
Author

Niiko commented Mar 18, 2016

@pjedrzejewski I can move prependElasticaProductListener to the core ? ElasticaProductListener too.

@Niiko
Copy link
Author

Niiko commented Mar 29, 2016

ping @pjedrzejewski :)

@pjedrzejewski
Copy link
Member

Looks good to me, we are going to change SearchBundle a lot, to simplify how it works. Could you move this from Core to SearchBundle? That's appropriate place I think.

@pjedrzejewski
Copy link
Member

Thanks for your work!

@Niiko
Copy link
Author

Niiko commented Mar 29, 2016

yes of course

@Niiko
Copy link
Author

Niiko commented Mar 30, 2016

Done @pjedrzejewski ;)

@pjedrzejewski pjedrzejewski merged commit 6049c11 into Sylius:master Mar 31, 2016
@pjedrzejewski
Copy link
Member

Great work Nicolas, you can't even imagine how helpful this PR is, opening and RFC in few minutes with explanation. :D Thanks!

@Niiko
Copy link
Author

Niiko commented Mar 31, 2016

My pleasure :)

@pjedrzejewski
Copy link
Member

See #4659, I rambled a bit, but I hope you get the idea. :D

@koemeet
Copy link
Contributor

koemeet commented Apr 14, 2016

@Niiko After upgrading, this PR caused the following error to appear when deleting a product:

ActionRequestValidationException[Validation Failed: 1: id is missing;2: id is missing;]

Any idea? I am using the same mapping as you are using for a product. Using Elastic Search 1.7, what version are you using?

I've found that in the postRemove lifecycle hook, the objects id is null, I think this is causing the trouble.

@Niiko
Copy link
Author

Niiko commented Apr 15, 2016

My bad, sorry

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
[SearchBundle][ProductBundle] Add multilingual in elasticsearch engine and check on product availability
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.

[RFC][SearchBundle] Wich way to handle multilingual in elasticsearch engine
8 participants