[MIG][11.0] mail_digest #225

simahawk · 2018-01-18T14:03:15Z

NOTE: I've tried to squash the 2 OCA transbot commits but I get tons of conflicts...

Depending on the request might fail nevertheless the website should be there to be found. Fallback to search and use it if possible for digest' site name

When creating digest records within the context of mail composer (and possibly other contexts) you'll have a `default_template_id` key in the context which is going to override our safe default. This is going to break email generation because the template will be completely wrong and unpredictable. Lesson learned :)

grindtildeath

It seems, there's something wrong with message and mail_id params, as a few are unused, Otherwise, looks good !

grindtildeath · 2018-01-19T08:05:46Z

mail_digest/data/ir_cron.xml

@@ -3,25 +3,23 @@
    <data noupdate="1">


grindtildeath · 2018-01-19T08:11:53Z

mail_digest/models/mail_digest.py

@@ -77,37 +75,37 @@ def create_or_update(self, partners, message, subtype_id=None):
        """
        subtype_id = subtype_id or message.subtype_id


Looks like this can be removed, as it isn't used in the function.

grindtildeath · 2018-01-19T08:15:15Z

mail_digest/models/mail_digest.py

            digest.message_ids |= message
        return True

    @api.model
-    def _get_by_partner(self, partner, mail_id=False):
-        """Retrieve digest record for given partner.
+    def _get_by_user(self, user, mail_id=False):


mail_id should be renamed mail

grindtildeath · 2018-01-19T08:15:45Z

mail_digest/models/mail_digest.py

        :param mail_id: `mail.mail` record for further filtering.

        By default we lookup for pending digest without notification yet.
        """
        domain = [
-            ('partner_id', '=', partner.id),
+            ('user_id', '=', user.id),
            ('mail_id', '=', mail_id),


mail_id is then mail.id

grindtildeath · 2018-01-19T08:16:27Z

mail_digest/models/mail_digest.py

            ('mail_id', '=', mail_id),
        ]
        return self.search(domain, limit=1)

    @api.model
-    def _get_or_create_by_partner(self, partner, message=None, mail_id=False):
-        """Retrieve digest record or create it by partner.
+    def _get_or_create_by_user(self, user, message=None, mail_id=False):


Again mail_id > mail, message=None to remove, as it's not used

grindtildeath · 2018-01-19T08:17:12Z

mail_digest/models/mail_digest.py

        :param message: `mail.message` to include in digest
        :param mail_id: `mail.mail` record to set on digest
        """
-        existing = self._get_by_partner(partner, mail_id=mail_id)
+        existing = self._get_by_user(user, mail_id=mail_id)


mail_id=mail_id > mail=mail

grindtildeath · 2018-01-19T08:29:31Z

mail_digest/models/res_users.py

+            ('user_id', '=', self.id)
+        ], limit=1)
+        if self.env.context.get('foo'):
+            import pdb; pdb.set_trace()


Seems this should be removed.

simahawk · 2018-01-19T09:04:00Z

@grindtildeath obsolete params removed. tnx :)

grindtildeath

Two <data>XML tags to remove and it's fine for me 👍

grindtildeath · 2018-01-19T09:40:05Z

mail_digest/data/ir_cron.xml

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>
+    <data noupdate="1">


Still this tag to remove.

grindtildeath · 2018-01-19T09:40:19Z

mail_digest/data/config_param.xml

@@ -0,0 +1,9 @@
+<odoo>
+    <data noupdate="1">


Still this tag to remove.

simahawk · 2018-01-19T09:44:15Z

@grindtildeath done

mpanarin

LGTM

tschanzt · 2018-01-29T15:50:35Z

mail_digest/models/mail_digest.py


    @api.model
-    def create_or_update(self, partners, message, subtype_id=None):
+    def create_or_update(self, partners, message):


is there any reason to still use the partner here? specially since you then get the user_id from the partner.

some reasons:

this method is called by res.partner _notify method as notification are sent to users but attached to partners. I like to keep the 2 things tied;

message/email recipients are partners (as in std notifications). Again like to keep this in line.

ok, that makes sense.

hbrunn · 2018-02-01T09:16:09Z

it's quite hard for me to separate the old commits from the new ones, @simahawk, can you check this?

simahawk · 2018-02-01T09:37:45Z

@hbrunn the last 3 commits. The former are improvements coming from v10 that were not ported yet.

hbrunn · 2018-02-01T09:43:34Z

thanks, I was doing something stupid before. I miss a migration script, but given we don't have rules about that to my knowledge, I won't block on this. Any reason not to provide one? Should be just a couple of SQL statements and make the upgrade experience much better.

simahawk · 2018-02-01T09:51:35Z

@hbrunn main reason, honestly: not needed for my project and not sure I can spend time on it.
I provided info in the README on how to achieve that... in case someone is willing to help. That could be even me in the next future 😉

hbrunn · 2018-02-01T10:06:06Z

@simahawk fair enough. But you inspired me to make this PR: OCA/maintainer-tools#324

simahawk and others added 10 commits January 17, 2018 14:58

[add] mail_digest

cd9abb8

OCA Transbot updated translations from Transifex

486397c

[MIG][v10] mail_digest

b012328

[imp] mail_digest: related state is readonly

09567ad

[imp] mail_digest: enforce template domain to type=qweb

9ae8c93

[imp] mail_digest: fallback to default website

b8f1eb8

Depending on the request might fail nevertheless the website should be there to be found. Fallback to search and use it if possible for digest' site name

[fix] mail_digest: manifest website

38d4817

mail_digest: upgrade to 10.0.1

8ab9092

OCA Transbot updated translations from Transifex

da6df1e

pedrobaeza added this to the 11.0 milestone Jan 18, 2018

grindtildeath suggested changes Jan 19, 2018

View reviewed changes

simahawk force-pushed the 11-mig-mail_digest branch 2 times, most recently from 674ad2a to 305fce8 Compare January 19, 2018 09:03

simahawk force-pushed the 11-mig-mail_digest branch from 305fce8 to 59974e6 Compare January 19, 2018 09:40

grindtildeath reviewed Jan 19, 2018

View reviewed changes

simahawk added 3 commits January 19, 2018 10:43

[MIG][11.0] mail_digest

6d310f5

mail_digest: update translations (new fields)

332e885

mail_digest: improve README

2e1e331

simahawk force-pushed the 11-mig-mail_digest branch from 59974e6 to 2e1e331 Compare January 19, 2018 09:44

grindtildeath approved these changes Jan 19, 2018

View reviewed changes

mpanarin approved these changes Jan 25, 2018

View reviewed changes

tschanzt reviewed Jan 30, 2018

View reviewed changes

tschanzt approved these changes Jan 30, 2018

View reviewed changes

hbrunn added the needs review label Feb 1, 2018

hbrunn merged commit d8fd94d into OCA:11.0 Feb 1, 2018

simahawk mentioned this pull request Feb 1, 2018

[ADD] a word about migrations OCA/maintainer-tools#324

Merged

emagdalenaC2i mentioned this pull request Jun 22, 2018

Migration to version 11.0 #203

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG][11.0] mail_digest #225

[MIG][11.0] mail_digest #225

simahawk commented Jan 18, 2018

grindtildeath left a comment

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

simahawk commented Jan 19, 2018

grindtildeath left a comment

grindtildeath Jan 19, 2018

grindtildeath Jan 19, 2018

simahawk commented Jan 19, 2018

mpanarin left a comment

tschanzt Jan 29, 2018

simahawk Jan 30, 2018

tschanzt Jan 30, 2018

hbrunn commented Feb 1, 2018

simahawk commented Feb 1, 2018

hbrunn commented Feb 1, 2018

simahawk commented Feb 1, 2018 •

edited

Loading

hbrunn commented Feb 1, 2018

		@@ -77,37 +75,37 @@ def create_or_update(self, partners, message, subtype_id=None):
		"""
		subtype_id = subtype_id or message.subtype_id

[MIG][11.0] mail_digest #225

[MIG][11.0] mail_digest #225

Conversation

simahawk commented Jan 18, 2018

grindtildeath 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

Choose a reason for hiding this comment

simahawk commented Jan 19, 2018

grindtildeath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simahawk commented Jan 19, 2018

mpanarin 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

hbrunn commented Feb 1, 2018

simahawk commented Feb 1, 2018

hbrunn commented Feb 1, 2018

simahawk commented Feb 1, 2018 • edited Loading

hbrunn commented Feb 1, 2018

simahawk commented Feb 1, 2018 •

edited

Loading