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

[10.0][ADD] website_sale_search_fuzzy. #169

Merged

Conversation

woodbrettm
Copy link

@woodbrettm woodbrettm commented Apr 8, 2017

Hi there,

This module adds a fuzzy search to product names for eCommerce purposes.

Depends

Resolves

Todo

  • Add post_init_hook tests (if hooks kept after discussion)
  • Update oca external dependencies text (add server-tools)

Cheers,
Brett

* Adds fuzzy search to product names
trgm_mod = env['trgm.index']

if trgm_mod._trgm_extension_exists() != 'installed':
raise UserError(_(
Copy link
Author

Choose a reason for hiding this comment

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

@lasley I'm doing env['trgm.index'].create({'field_id': prod_templ_name, 'index_type': 'gin'}) below here in this post init hook, adding the index to the product template name field.

I have a check here that ensures trgm is installed on postgres (used from base_search_fuzzy) before adding the index, however I don't think travis or runbot are configured to have it installed.

Is doing a post_init_hook the wrong way to go about it? Or not sure if some config files have to be edited to get trgm installed.

Perhaps doing the post_init_hook is correct and having a trgm check is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this logic is already included in base_search_fuzzy, it is superfluous here because there is no way for this module to be installed without it.

Copy link
Author

Choose a reason for hiding this comment

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

The function is used in base_search_fuzzy, however it seems the logic in the module only logs warnings if it isn't installed.

Seems like some tests are cancelled out if it isn't installed either.
https://github.com/OCA/server-tools/search?utf8=%E2%9C%93&q=_trgm_extension_exists&type=

Copy link

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

some nitpicking

mission is to support the collaborative development of Odoo features and
promote its widespread use.

To contribute to this module, please visit http://odoo-community.org.

Choose a reason for hiding this comment

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

https


Make sure to follow the installation instructions of base_search_fuzzy before
installing this module.

Choose a reason for hiding this comment

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

A screenshot would be nice to have

@elicoidal
Copy link

please correct travis+runbot

@lasley
Copy link
Contributor

lasley commented Apr 11, 2017

@BMW95 - dependency has been merged & RFC closed.

@woodbrettm woodbrettm force-pushed the feature/10.0/SMD-251-create-website_sale_search_fuzzy branch 2 times, most recently from 31b3cfc to c34cdb2 Compare April 28, 2017 00:41
Brett Wood added 2 commits April 27, 2017 18:16
* Move mock patch import location
* Add https to odoo website in readme
* Add link to base_search_fuzzy in readme
* Add test to ensure index on product templ name
@woodbrettm woodbrettm force-pushed the feature/10.0/SMD-251-create-website_sale_search_fuzzy branch from c34cdb2 to c889713 Compare April 28, 2017 01:17
@woodbrettm
Copy link
Author

@elicoidal thanks for the review. For the readme you requested a picture underneath the paragraph about the base_search_fuzzy instructions. Were you looking for a picture of the e-commerce view, or the base_search_fuzzy instructions? For the instructions, I added a link instead if that's okay :)

Other than that, ready for review!

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Minor comment in the hook, otherwise LGTM

@woodbrettm
Copy link
Author

Thanks @lasley :)

Copy link

@elicoidal elicoidal left a comment

Choose a reason for hiding this comment

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

LGTM

:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/113/10.0

Known Issues / Roadmap

Choose a reason for hiding this comment

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

Remove the section if empty

* Remove empty section headers
@woodbrettm
Copy link
Author

Updated

@lasley
Copy link
Contributor

lasley commented May 13, 2017

Good for squash & merge?

@woodbrettm
Copy link
Author

yup!

@lasley
Copy link
Contributor

lasley commented May 15, 2017

Can a PSC do the squash/merge honors whenever they get a chance please and thanks!

@lasley lasley merged commit d1487d8 into OCA:10.0 Jun 16, 2017
@lasley lasley deleted the feature/10.0/SMD-251-create-website_sale_search_fuzzy branch June 16, 2017 19:53
remytms pushed a commit to coopiteasy/e-commerce that referenced this pull request Feb 11, 2019
* [ADD] Add module website_sale_search_fuzzy.
* Adds fuzzy search to product names

* [IMP] website_sale_search_fuzzy: Fix readme, imports, add tests.
* Move mock patch import location
* Add https to odoo website in readme
* Add link to base_search_fuzzy in readme
* Add test to ensure index on product templ name

* [ADD] Add server-tools to oca-dependencies.

* [FIX] website_sale_search_fuzzy: Fix readme.
* Remove empty section headers
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