[10.0][MIG] base_search_mail_content #180

MiquelRForgeFlow · 2017-07-10T11:18:06Z

Standard migration.

yajo · 2017-08-07T08:09:29Z

base_search_mail_content/models/mail_thread.py

+    def _compute_message_content(self):
+        """ We don't really need to show any content. This field is to be
+        used only by searches"""
+        return ''


Compute fields don't return, they just set self.field=something. Just pass please.

yajo · 2017-08-07T08:10:59Z

base_search_mail_content/models/mail_thread.py

+    return res
+
+
+models.BaseModel.fields_view_get = _custom_fields_view_get


This monkey-patching should not get accepted now on v10. Override it as usual with _inherit = "base" instead (it is a new v10 feature).

yajo · 2017-08-07T08:11:28Z

base_search_mail_content/models/res_partner.py

+class Partner(models.Model):
+
+    _name = 'res.partner'
+    _inherit = ['res.partner', 'mail.thread']


Why this file? res.partner already inherits from mail.thread by default... Isn't it?

I don't think so.

It already inherits or you wouldn't have chatter on partners, so this is not needed.

Ok. Let's see if it works now without the file 👍

yajo · 2017-08-21T08:38:25Z

base_search_mail_content/models/mail_thread.py

+        search='_search_message_content')
+
+
+class BaseModel(object):


I bet this shouldn't inherit from object

yajo · 2017-08-21T08:38:59Z

base_search_mail_content/models/mail_thread.py

+
+class BaseModel(object):
+
+    _inherit = 'base'


Shouldn't this be mail.thread instead? So it should be merged with the class above... 🤔

Can you explain a bit more? You said before that we should override it with _inherit = "base". 🤔

Yeah, I know 😆

Sorry for confusing. I said that before because it's the new way to avoid monkey patches, but after seeing this now, I realized that you only need this code for addons that inherit from mail.thread, so possibly adding the method there should be better.

yajo · 2017-08-21T08:39:42Z

base_search_mail_content/models/mail_thread.py

+        res = super(BaseModel, self)._fields_view_get(
+            view_id=view_id, view_type=view_type, toolbar=toolbar,
+            submenu=submenu)
+        if view_type == 'search' and self._fields.get('message_content'):


If you add the method in mail.thread, then you don't need to check and self._fields.get('message_content'), since it would always be true.

yajo · 2017-08-21T08:51:06Z

base_search_mail_content/models/mail_thread.py

+            operators = {'!=': '=', 'not like': 'like',
+                         'not ilike': 'ilike', 'not in': 'in'}
+            operator = operators[operator]
+        domain = [('model', '=', self._name), '|', '|', '|', '|',


It seems to me that it would be much cleaner and more efficient to do something like:

domain = [('model', '=', self._name)] if operator not in expression.NEGATIVE_TERM_OPERATORS: domain += ["|"] * 4 domain += [('record_name', operator, value), ('subject', operator, value), ('body', operator, value), ('email_from', operator, value), ('reply_to', operator, value)]

yajo · 2017-08-21T08:51:58Z

base_search_mail_content/models/mail_thread.py

+    message_content = fields.Text(
+        string='Message Content',
+        help='Message content, to be used only in searches',
+        compute="_compute_message_content",


Maybe you can compute=lambda self: False instead. Just a suggestion.

yajo · 2017-08-21T10:32:37Z

base_search_mail_content/models/mail_thread.py

@@ -22,42 +22,34 @@ def _search_message_content(self, operator, value):
            operators = {'!=': '=', 'not like': 'like',
                         'not ilike': 'ilike', 'not in': 'in'}
            operator = operators[operator]


Delete all above this in the method.

yajo · 2017-08-21T10:32:54Z

base_search_mail_content/models/mail_thread.py

+                   ('subject', operator, value),
+                   ('body', operator, value),
+                   ('email_from', operator, value),
+                   ('reply_to', operator, value)]
        recs = self.env['mail.message'].search(domain)
        return [('id', main_operator, recs.mapped('res_id'))]


Delete these 2 lines and return domain

yajo · 2017-08-21T10:46:19Z

base_search_mail_content/__manifest__.py

+    "data": ["data/trgm_index_data.xml",
+             "views/trgm_index_view.xml"],
+    "depends": ["mail",
+                "base_search_fuzzy"],


Is this really a dependency?

Yes.

mail is because the inherited mail.thread class

base_search_fuzzy, a module in server-tools repository, is because the inherited trgm_index_view.xml view

yajo

Code looks good, but UI fails.

yajo · 2017-08-21T11:52:29Z

base_search_mail_content/models/mail_thread.py

+            for node in doc.xpath("//field[1]"):
+                # Add message_content in search view
+                elem = etree.Element('field', {
+                    'name': 'message_content',


This does not seem to be working, maybe yes it was required to inherit from base instead.

So... return again to:

class BaseModel(object): _inherit = 'base'

🤔?

So it seems... Remember that reviewers can be wrong, you should test what you do and let it as it was if new patch does not work 😉

yajo · 2017-08-21T11:53:04Z

base_search_mail_content/models/mail_thread.py

+
+    def _search_message_content(self, operator, value):
+
+        domain = [('model', '=', self._name)]


This seems to break the UI:

Odoo Server Error Traceback (most recent call last): File "/.repo_requirements/odoo/odoo/http.py", line 640, in _handle_exception return super(JsonRequest, self)._handle_exception(exception) File "/.repo_requirements/odoo/odoo/http.py", line 677, in dispatch result = self._call_function(**self.params) File "/.repo_requirements/odoo/odoo/http.py", line 333, in _call_function return checked_call(self.db, *args, **kwargs) File "/.repo_requirements/odoo/odoo/service/model.py", line 101, in wrapper return f(dbname, *args, **kwargs) File "/.repo_requirements/odoo/odoo/http.py", line 326, in checked_call result = self.endpoint(*a, **kw) File "/.repo_requirements/odoo/odoo/http.py", line 935, in __call__ return self.method(*args, **kw) File "/.repo_requirements/odoo/odoo/http.py", line 506, in response_wrap response = f(*args, **kw) File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 827, in search_read return self.do_search_read(model, fields, offset, limit, domain, sort) File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 849, in do_search_read offset=offset or 0, limit=limit or False, order=sort or False) File "/.repo_requirements/odoo/odoo/models.py", line 4678, in search_read records = self.search(domain or [], offset=offset, limit=limit, order=order) File "/.repo_requirements/odoo/odoo/models.py", line 1518, in search res = self._search(args, offset=offset, limit=limit, order=order, count=count) File "/.repo_requirements/odoo/odoo/addons/base/res/res_partner.py", line 633, in _search count=count, access_rights_uid=access_rights_uid) File "/.repo_requirements/odoo/odoo/models.py", line 4220, in _search query = self._where_calc(args) File "/.repo_requirements/odoo/odoo/models.py", line 4019, in _where_calc e = expression.expression(domain, self) File "/.repo_requirements/odoo/odoo/osv/expression.py", line 643, in __init__ self.parse() File "/.repo_requirements/odoo/odoo/osv/expression.py", line 821, in parse raise ValueError("Invalid field %r in leaf %r" % (left, str(leaf))) ValueError: Invalid field 'model' in leaf "<osv.ExtendedLeaf: ('model', '=', 'res.partner') on res_partner (ctx: )>"

And what do you propose? 😮

Possibly changing this to domain=[] fixes it.

MiquelRForgeFlow · 2017-09-07T10:50:28Z

@yajo , could you please update your review?

yajo · 2017-09-07T11:10:37Z

I feel current code will not work, have you tested it?

MiquelRForgeFlow · 2017-09-07T12:19:17Z

I have just tested, but it has some issues that should be fixed

rafaelbn · 2017-09-07T15:50:48Z

Please @mreficent ping me when module is ready for a functional test! thanks

Introduce search of html fields Create indexes Update Readme

MiquelRForgeFlow · 2017-09-07T18:01:08Z

@yajo @rafaelbn Issues solved! Now it's ready functional! 🍏

rafaelbn · 2017-09-07T22:18:09Z

Hi @mreficent thank you!!! you should improve test as it has 42.85% of diff hit (target 98.81%)

I will wait until @yajo finish his review ;-) for testing

elicoidal

non-blocking suggestions

elicoidal · 2017-09-08T00:19:41Z

base_search_mail_content/README.rst

+This module depends on the module 'base_search_fuzzy' to ensure that
+searches on emails are based on indexes. Please read carefully the install
+instructions:
+https://github.com/OCA/server-tools/blob/10.0/base_search_fuzzy/README.rst


Maybe better:

`install instructions <https://github.com/OCA/server-tools/blob/10.0/base_search_fuzzy/README.rst>`_.

elicoidal · 2017-09-08T00:20:00Z

base_search_mail_content/README.rst

+* Lois Rilo Antelo <lois.rilo@eficent.com>
+* Aaron Henriquez <ahenriquez@eficent.com>
+
+


remove line

yajo · 2017-09-08T07:52:40Z

base_search_mail_content/models/mail_thread.py

+        search='_search_message_content')
+
+
+class BaseModel(object):


I can't seem to do quick search of message contents. Most likely this should inherit from models.AbstractModel instead, although I think you already fixed this before... 🤔

Postgresql decides when is best to use trigram indexes. You will need to install this module into a DB with a rather large number of emails.

To check if the trigram indexes are being used, you must:

start odoo with --log-level=debug_sql

Do the search that you want to analyze - e.g. search for emails from the leads.

Copy the query and run in command line EXPLAIN ANALYZE (my_query):
EXPLAIN ANALYZE (SELECT "mail_message".id FROM "mail_message" WHERE (("mail_message"."model" = 'res.partner') AND ((((("mail_message"."record_name"::text ilike 'dear') OR ("mail_message"."subject"::text ilike 'dear')) OR ("mail_message"."body"::text ilike 'dear')) OR ("mail_message"."email_from"::text ilike 'dear')) OR ("mail_message"."reply_to"::text ilike 'dear'))) ORDER BY "mail_message"."id" DESC);

This will tell you if trigram indexes are being used.

JordiBForgeFlow · 2017-09-14T15:05:17Z

the field message_content was not being added to the search view. I have fixed that, and now the module is 🌟 🌟 🌟

MiquelRForgeFlow · 2017-09-14T15:40:13Z

@yajo @rafaelbn update review please 🍏

yajo

Excelent!

Please do the final squash

MiquelRForgeFlow · 2017-09-15T08:44:14Z

Squash done

JordiBForgeFlow · 2017-09-15T09:08:18Z

@rafaelbn can you review?

pedrobaeza · 2017-09-15T09:11:13Z

Is there the possibility of improving test coverage?

rafaelbn

Tested functionaly 👍 but codecov/patch — 38.46% of diff hit (target 98.81%) must be increased please! This is a great funcionality

ping @mreficent @jbeficent

yajo · 2017-09-19T11:52:50Z

base_search_mail_content/tests/test_base_search_mail_content.py

+        self.thread_obj = self.env["mail.thread"]
+
+    def test_base_search_mail_content(self):
+        res = self.thread_obj._search_message_content('not ilike', 'xxxyyyzzz')


Actually the test should be self.thread_obj.search([("message_content", ...

yajo · 2017-09-19T11:53:01Z

base_search_mail_content/tests/test_base_search_mail_content.py

+
+    def test_base_search_mail_content(self):
+        res = self.thread_obj._search_message_content('not ilike', 'xxxyyyzzz')
+        self.assertEqual(res[0][2], [], "You have a message with xxxyyyzzz :O")


and then here assertFalse

MiquelRForgeFlow · 2017-09-19T15:11:43Z

There is not codecov/patch now 😂

rafaelbn

Tested functionaly 👍

rafaelbn · 2017-09-19T17:03:22Z

@mreficent please could answer @yajo 's comments ? I wish tomorrow we can merge this one! Great work!

MiquelRForgeFlow · 2017-09-19T17:07:05Z

Which comments? All @yajo's comments have been attended I think.

yajo · 2017-09-20T07:34:25Z

Cool! could you squash the migration commits to merge please? 😊

MiquelRForgeFlow · 2017-09-20T09:04:28Z

Done! codecov/patch at 100%

MiquelRForgeFlow force-pushed the 10.0-mig-base_search_mail_content branch 3 times, most recently from 20cb978 to c8eb14c Compare July 10, 2017 12:05

yajo added the needs review label Jul 10, 2017

yajo added this to the 10.0 milestone Jul 10, 2017

pedrobaeza force-pushed the 10.0-mig-base_search_mail_content branch from c8eb14c to 256d275 Compare August 5, 2017 15:13

pedrobaeza approved these changes Aug 5, 2017

View reviewed changes

yajo requested changes Aug 7, 2017

View reviewed changes

yajo requested changes Aug 21, 2017

View reviewed changes

rafaelbn self-requested a review August 21, 2017 15:42

serpentcs-dev1 and others added 4 commits September 7, 2017 19:14

[ADD] renamed module to base_search_mail_content

22e092b

[IMP] search by email_from and reply_to

8dec5c9

Introduce search of html fields Create indexes Update Readme

[IMP] advanced search for negative expression

a3fbea0

OCA Transbot updated translations from Transifex

b590533

MiquelRForgeFlow force-pushed the 10.0-mig-base_search_mail_content branch 2 times, most recently from bfbcaf7 to 0d144d4 Compare September 7, 2017 17:31

elicoidal approved these changes Sep 8, 2017

View reviewed changes

yajo requested changes Sep 8, 2017

View reviewed changes

MiquelRForgeFlow force-pushed the 10.0-mig-base_search_mail_content branch from 0d144d4 to 11b5ee8 Compare September 8, 2017 09:01

yajo approved these changes Sep 15, 2017

View reviewed changes

MiquelRForgeFlow force-pushed the 10.0-mig-base_search_mail_content branch from d4c8e64 to f7eeb11 Compare September 15, 2017 08:19

rafaelbn reviewed Sep 18, 2017

View reviewed changes

yajo requested changes Sep 19, 2017

View reviewed changes

MiquelRForgeFlow force-pushed the 10.0-mig-base_search_mail_content branch 2 times, most recently from 3070674 to 110b28d Compare September 19, 2017 13:56

rafaelbn approved these changes Sep 19, 2017

View reviewed changes

yajo approved these changes Sep 20, 2017

View reviewed changes

[MIG] base_search_mail_content: Migration to 10.0

b36457e

MiquelRForgeFlow force-pushed the 10.0-mig-base_search_mail_content branch from 110b28d to b36457e Compare September 20, 2017 08:32

pedrobaeza merged commit a2ecf4a into OCA:10.0 Sep 20, 2017

pedrobaeza mentioned this pull request Sep 20, 2017

Migration to version 10.0 #95

Closed

21 tasks

MiquelRForgeFlow deleted the 10.0-mig-base_search_mail_content branch September 20, 2017 09:13

		return res


		models.BaseModel.fields_view_get = _custom_fields_view_get


		def _search_message_content(self, operator, value):

		domain = [('model', '=', self._name)]

		* Lois Rilo Antelo <lois.rilo@eficent.com>
		* Aaron Henriquez <ahenriquez@eficent.com>

[10.0][MIG] base_search_mail_content #180

[10.0][MIG] base_search_mail_content #180

Conversation

MiquelRForgeFlow commented Jul 10, 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

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

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

MiquelRForgeFlow Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

yajo left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiquelRForgeFlow commented Sep 7, 2017

yajo commented Sep 7, 2017

MiquelRForgeFlow commented Sep 7, 2017 • edited Loading

rafaelbn commented Sep 7, 2017

MiquelRForgeFlow commented Sep 7, 2017 • edited Loading

rafaelbn commented Sep 7, 2017

elicoidal left a 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

MiquelRForgeFlow Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordiBForgeFlow commented Sep 14, 2017

MiquelRForgeFlow commented Sep 14, 2017

yajo left a comment

Choose a reason for hiding this comment

MiquelRForgeFlow commented Sep 15, 2017

JordiBForgeFlow commented Sep 15, 2017

pedrobaeza commented Sep 15, 2017

rafaelbn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiquelRForgeFlow commented Sep 19, 2017

rafaelbn left a comment

Choose a reason for hiding this comment

rafaelbn commented Sep 19, 2017

MiquelRForgeFlow commented Sep 19, 2017 • edited Loading

yajo commented Sep 20, 2017

MiquelRForgeFlow commented Sep 20, 2017

MiquelRForgeFlow Aug 21, 2017 •

edited

Loading

MiquelRForgeFlow commented Sep 7, 2017 •

edited

Loading

MiquelRForgeFlow commented Sep 7, 2017 •

edited

Loading

MiquelRForgeFlow Sep 8, 2017 •

edited

Loading

rafaelbn left a comment •

edited

Loading

MiquelRForgeFlow commented Sep 19, 2017 •

edited

Loading