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

[16.0] [ADD] multisearch_field #967

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

TB-Ph35
Copy link

@TB-Ph35 TB-Ph35 commented Nov 7, 2024

This module add multi-searching with separator for all search.

It add to model.search() code to enable searching with separator.
example:

  1. you are searching "Tom|kelly;hart"
  2. If the separator are "|" and ";"
  3. it will search for "Tom|kelly;hart", "Tom|kelly", "Tom", "kelly;hart", "kelly" and "hart"

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Nice work ! thanks for sharing.

some minor remarks inline. Otherwise, LGTM.

@@ -0,0 +1,6 @@
To configure the separator you must:
Copy link
Contributor

Choose a reason for hiding this comment

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

better to put this part in a CONFIGURE.rst section.

In the USAGE file you can put a print screen. (something like "before" / "after").

@@ -0,0 +1 @@
* This module allows mulit_searching for all search.
Copy link
Contributor

Choose a reason for hiding this comment

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

could you elaborate the description ? say exactly what the modules does.
(there are some base_search_xxx module in the OCA, like base_name_search_improved, base_search_fuzzy, etc...)

else:
domain_list.append(dom)
for _ in range(nbr):
domain_list.append("|")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can manipulate all the domain with expression.OR and expression.AND. It will be more readable I think.

@@ -0,0 +1,15 @@
{
"name": "MultiSearch",
Copy link
Contributor

Choose a reason for hiding this comment

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

too generic title in my opinion.

@bealdav
Copy link
Member

bealdav commented Nov 7, 2024

Thanks @TB-Ph35 for this contribution.

Regarding the module name, it's not an easy topic, isn't it?
If we want to make the name meaningful, we need to add meaningful terms and remove some.
I guess base_ doesn't help to understand what the module does.

What the module does, if I understood correctly, is to allow searching for multiple values ​​of a particular field.

separator is the way to do it, not what it does

To make this understandable with the module name, some candidates could be:

  • field_multisearch
  • multisearch_field

another suggestion?

@legalsylvain
Copy link
Contributor

FYI, my remarks regarding the name was about the value "name" in the manifest, not the technical name.

@TB-Ph35
Copy link
Author

TB-Ph35 commented Nov 7, 2024

Thanks for all the comment.

With those comment, i have made changes. But not the names module. It is not easy like you said, and that's really not my game. I will think about it.

@@ -0,0 +1,15 @@
{
"name": "Field MultiSearch with separotor",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"name": "Field MultiSearch with separotor",
"name": "Field MultiSearch with separator",

typo.

@PaulGoubert
Copy link

LGTM. Thank you for your work, Thomas.

I tried on the Customer Invoice by doing a search with:

Just one remark: once we want to copy and paste several lines from an XLS sheet, we don't have a separator, but only a space.
So in that case, we have to add the separator and remove the space before copying to make the search work.
Capture d’écran 2024-11-07 à 15 24 03
It could be very interesting to have the space consideration in the search, or even as a separator itself.

@PaulGoubert
Copy link

Concerning the name, for my point of view, the main thing is to have multisearch in the module name. Then as @bealdav mentionned, field_multisearch or multisearch_field look good for me.

If the Space is concidered as a "separator", the name separator is not required in the module name.

@TB-Ph35
Copy link
Author

TB-Ph35 commented Nov 7, 2024

I will look for adding the space. But now it can be add to the list of separator because for some search it broke the search. Like for self.env["resource.calendat.leaves"]. It split datetimeformat and that make this error :

psycopg2.errors.InvalidDatetimeFormat: invalid input syntax for type timestamp: "16:23:06"
LINE 1: ...')) OR ("resource_calendar_leaves"."date_from" <= '16:23:06'...

if i don't understand it wrong

@PaulGoubert
Copy link

The search with space work fine now with / before the search.
For product for ex : /E-COM06 E-COM07 E-COM08

Thank you @TB-Ph35

Copy link

@PaulGoubert PaulGoubert left a comment

Choose a reason for hiding this comment

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

Functionnal review OK

@TB-Ph35 TB-Ph35 force-pushed the 16.0-base_multi_search_separator branch from 6a6f4c0 to 1fac86e Compare November 12, 2024 21:11
Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

Great job Thomas, really useful module, some typo comments here.
Could you crop your images to focus on parts without white page color to see that you want to show.
Probably squash commits could be nice
Thanks a lot.

This module allows mulit_searching for all search.
For that, it add code before the search function.

It will allows you to search for multi_value separate by the character that you
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It will allows you to search for multi_value separate by the character that you
It will allow you to search for multiple values ​​separated by the character you

For that, it add code before the search function.

It will allows you to search for multi_value separate by the character that you
have configure.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
have configure.
have configured.

@@ -0,0 +1,6 @@
<odoo noupdate="1">
<record id="multi_search_separator" model="ir.config_parameter">
<field name="key">multi_search_separator_base</field>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<field name="key">multi_search_separator_base</field>
<field name="key">multi_search_separator</field>

fields"""
if self.env["ir.model.access"].check_access_rights("read", raise_exception=False):
list_separator = self.env["ir.config_parameter"].get_param(
"multi_search_separator_base"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"multi_search_separator_base"
"multi_search_separator"

To configure the separator you must:
* be in dev_mode
* go to Settings / technical / system parameter
* search multi_search_separator_base
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* search multi_search_separator_base
* search multi_search_separator


class TestMultisearch(TransactionCase):
def setUp(self):
super(TestMultisearch, self).setUp()

This comment was marked as outdated.

@TB-Ph35 TB-Ph35 force-pushed the 16.0-base_multi_search_separator branch from 1fac86e to 42d551e Compare November 14, 2024 13:57
@TB-Ph35 TB-Ph35 changed the title 16.0 base multi search separator [16.0] [ADD] multisearch_field Nov 14, 2024
Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

Nice, thanks a lot

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants