[10.0][MIG] migrate module mail_forward #256

ecino · 2018-04-12T14:09:02Z

This takes the module mail_forward that was available in version 8.0 and updates it in order to work on version 10.0

pedrobaeza · 2018-04-12T15:22:29Z

mail_forward/README.rst

+------------
+
+* Emanuel Cino <ecino@compassion.ch>
+* Jairo Llopis <j.llopis@grupoesoc.es>


Please change the author to the new company: jairo.llopis@tecnativa.com

pedrobaeza · 2018-04-12T15:22:44Z

mail_forward/__manifest__.py

+    "version": "10.0.1.0.0",
+    "category": "Social Network",
+    "website": "https://grupoesoc.es",
+    "author": "Grupo ESOC Ingeniería de Servicios, "


Change also the company to the new one: Tecnativa

rafaelbn

Hi @ecino , thank!

I've tested and it work fine just one thing you should fix. After fordward the message, the chatter is not refreshed and user have to press F5... I think chatter should be refreshed in the same way that sending a new message happends

Let me know please

Thanks

ecino · 2018-04-16T06:58:39Z

@rafaelbn I added the functionnality in cac0a83.
However I'm not sure it's the most efficient way to refresh, as I'm not familiar with what is available in the javascript functions of the mail module. This requires as well #257 to work, because it uses the chat_manager.make_message function.
But in the end, it does what you suggested.

ecino · 2018-04-24T11:30:19Z

@rafaelbn The module is now working with #257 being merged.

rafaelbn

Tested in runbot functionally 👍

rafaelbn · 2018-04-26T16:16:31Z

mail_forward/__manifest__.py

+    "summary": "Add option to forward messages",
+    "version": "10.0.1.0.0",
+    "category": "Social Network",
+    "website": "https://grupoesoc.es",


@ecino please change this for the rightone as described in https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L9

rafaelbn · 2018-04-26T16:17:05Z

mail_forward/__manifest__.py

+    "version": "10.0.1.0.0",
+    "category": "Social Network",
+    "website": "https://grupoesoc.es",
+    "author": "Odoo Community Association (OCA)"


Don't change order please https://github.com/OCA/maintainer-tools/blob/master/template/module/__openerp__.py#L10

rafaelbn

Please @yajo as original author review this migration. Thanks!

rafaelbn · 2018-04-26T16:21:27Z

mail_forward/README.rst

+Contributors
+------------
+
+* Emanuel Cino <ecino@compassion.ch>


@ecino you should add you name after older contributors and not before

rafaelbn

Please @ecino check my comments. Thank you!

ecino · 2018-04-27T05:57:31Z

@rafaelbn Thanks for reviewing it. Corrections are done.

yajo · 2018-04-27T07:34:11Z

mail_forward/models/compose_message.py

-    @api.one
+    @api.multi
+    def send_mail_action(self):
+        return self.send_mail()


Remove this method.

yajo · 2018-04-27T07:36:18Z

mail_forward/static/src/js/mail_forward.js

-            } else if (subject.length < 2) {
-                subject.push(_t("(No subject)"));
-            }
+            MessageModel.call('read', [message_id, read_fields], {context: session.user_context}).then(function (result) {


Please put this into a separate, proxied, class-level method.

Just for inheritance/maintainability. 😉

Not sure what you mean by proxied method. I'm no javascript expert, I just ported the module to v10.

yajo · 2018-04-27T07:46:36Z

mail_forward/static/src/js/mail_forward.js

+                this.trigger("message_forward", message_id);
+            };
+        },
+        thread_reload: function(){


I'm not sure I like this new feature.

I did a test from the Odoo inbox forwarding to a different object... If you are forwarding to a different object, why to reload? It should never get displayed...

It moves me above in the message list if I was browsing somewhere below, and clears the messages. Well... it's a confusing behavior.

@rafaelbn originally requested this feature. As I said, I'm no javascript expert so maybe I did not implement it the best way. Anyway I would need help to improve javascript.

yajo · 2018-04-27T07:48:01Z

I tested on runbot.

I go to events, and try to forward one message to another event. Events model don't appear in the dropdown list. Is this intentional?

AaronHForgeFlow

Tested on runbot and worked fine. Installed Sales and can forwarded messages between different SO successfully. However I don't see Even't models. Maybe it's been filtered somewhere. Can you check?

ecino · 2018-06-04T08:09:14Z

Hi @aheficent, sorry for the delay of my answer. You should add the Event model in the referenceable models (see the README of the module). You find the menu in Admin Settings -> Technical -> Database Structure -> Referenceable Models

yajo · 2018-06-04T12:08:08Z

Yikes! that was in the README! Bad me for not reading myself 😆

OK, then all you need to do is fix that ❌ in Travis. The JS improvements are needed, yes, but that can be done later.

Thanks!

AaronHForgeFlow · 2018-06-04T13:43:32Z

Thank you for your reply @ecino

I'm getting an error when forwarding messages between events. The trace:

File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 896, in call_button
action = self._call_kw(model, method, args, {})
File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 884, in _call_kw
return call_kw(request.env[model], method, args, kwargs)
File "/.repo_requirements/odoo/odoo/api.py", line 685, in call_kw
method = getattr(type(model), name)
AttributeError: type object 'mail_forward.compose_message' has no attribute 'send_mail_action'

ecino · 2018-06-04T15:14:47Z

Oops, that was after I removed the method as suggested by @yajo. It looks like the method is needed, so I will add it again. I will tell you when it's done.

ecino · 2018-06-29T09:34:13Z

The pull request is ready to go. Travis errors should be fixed with #288

yajo · 2018-07-02T08:19:24Z

Then please rebase to get those changes; right now bots are ❌

ecino · 2018-07-02T11:41:09Z

@yajo , successfully rebased, everything looks good. ✔️

yajo

I tested on runbot:

In messages view, clicking on the ⭐ or ✔️ icons, has no effect

When forwarding message to a partner, an error appears:

Odoo Server Error

Traceback (most recent call last):
  File "/.repo_requirements/odoo/odoo/http.py", line 642, in _handle_exception
    return super(JsonRequest, self)._handle_exception(exception)
  File "/.repo_requirements/odoo/odoo/http.py", line 684, in dispatch
    result = self._call_function(**self.params)
  File "/.repo_requirements/odoo/odoo/http.py", line 334, 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 327, in checked_call
    result = self.endpoint(*a, **kw)
  File "/.repo_requirements/odoo/odoo/http.py", line 942, in __call__
    return self.method(*args, **kw)
  File "/.repo_requirements/odoo/odoo/http.py", line 507, in response_wrap
    response = f(*args, **kw)
  File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 896, in call_button
    action = self._call_kw(model, method, args, {})
  File "/home/odoo/odoo-10.0/addons/web/controllers/main.py", line 884, in _call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/.repo_requirements/odoo/odoo/api.py", line 685, in call_kw
    method = getattr(type(model), name)
AttributeError: type object 'mail_forward.compose_message' has no attribute 'send_mail_action'

ecino · 2018-07-02T13:00:39Z

Maybe I dropped something during the rebase, sorry about that. I'll check it and let you know when it's fixed. Thanks for double checking it.

ecino · 2018-07-05T09:39:38Z

@yajo How do you access the runbot? I cannot find how to launch it, I just see the build logs.

yajo · 2018-07-05T11:03:41Z

There it is:

ecino · 2018-07-05T11:55:37Z

@yajo I fixed the forward bug (method send_action was not to be removed). However your other issues with the star and check icons seem not related with this module. I checked on the current runbot of the branch 10.0 and the same issue happens.

rafaelbn

Tested again functionally 👍

antoniocanovas · 2018-07-05T19:56:35Z

Tested functionality

pedrobaeza · 2018-07-06T07:49:34Z

@yajo please update your review

yajo · 2018-07-09T06:46:00Z

mail_forward/models/compose_message.py

+
+    @api.multi
+    def send_mail_action(self):
+        return self.send_mail()


Why to declare 2 methods to do the same? Am I missing something? 🤔 Otherwise, remove one of them please. 😉

The way odoo mail_compose_message is done works the same, if we don't declare them it will fail. I tried to remove this one but then it fails.

OK, I see now. However, you are repeating here the same code as upstream, so it still makes no sense that it fails... (You should only override a method if anything changes)

Besides, it breaks inheritance too. In case this were needed, it should be:

return super(MailComposeMessage, self).send_mail()

Could you double-check this please?

I know it's weird but when I removed that send_mail_action method, Travis was failing so I don't know why but this method is needed. If I do as you suggest, we will bypass our send_mail method which is not good. The other solution I can do is to call super of send_mail_action. I know the code seems weird to only call super method but if you want me I can remove it again and you will see by yourself that tests are failing without it.

Oh snap, excuse me! It was right as it was. I didn't realize this was a _inherits model, not an _inherit one.

This kind of inheritance inherits fields, but not methods. Possibly calling super() will not work, and the correct code is the one you had indeed.

Just please add a comment similar to upstream's, so reviewers can understand the reasoning behind that code.

Thanks, and sorry!

No worries, I didn't know as well that _inherits don't inherit the methods. Now it's clear why it's needed. I added the comment and restore the previous version. Thanks for your review.

yajo

Awesome, thanks!

CO-1483

ecino mentioned this pull request Apr 12, 2018

Migration to version 10.0 #95

Closed

21 tasks

ecino force-pushed the 10.0-mig-mail_forward branch from 562f9a9 to 4de116d Compare April 12, 2018 14:34

pedrobaeza reviewed Apr 12, 2018

View reviewed changes

pedrobaeza added this to the 10.0 milestone Apr 12, 2018

pedrobaeza added the needs review label Apr 12, 2018

ecino force-pushed the 10.0-mig-mail_forward branch 4 times, most recently from 903f39e to d6f188a Compare April 13, 2018 14:03

rafaelbn reviewed Apr 13, 2018

View reviewed changes

ecino force-pushed the 10.0-mig-mail_forward branch from cac0a83 to 0818e3f Compare April 24, 2018 11:27

rafaelbn approved these changes Apr 26, 2018

View reviewed changes

rafaelbn reviewed Apr 26, 2018

View reviewed changes

rafaelbn requested changes Apr 26, 2018

View reviewed changes

ecino force-pushed the 10.0-mig-mail_forward branch from cf6ae00 to 4ff4dc1 Compare April 27, 2018 05:56

yajo requested changes Apr 27, 2018

View reviewed changes

AaronHForgeFlow reviewed Apr 27, 2018

View reviewed changes

ecino force-pushed the 10.0-mig-mail_forward branch from 4ff4dc1 to 98c3539 Compare June 4, 2018 08:47

yajo added work in progress and removed needs review labels Jun 5, 2018

ecino force-pushed the 10.0-mig-mail_forward branch from 98c3539 to f663bf2 Compare June 28, 2018 14:11

ecino force-pushed the 10.0-mig-mail_forward branch from f663bf2 to 77e71a7 Compare July 2, 2018 08:53

yajo requested changes Jul 2, 2018

View reviewed changes

ecino force-pushed the 10.0-mig-mail_forward branch from 77e71a7 to e584401 Compare July 5, 2018 11:22

rafaelbn added needs review and removed work in progress labels Jul 5, 2018

rafaelbn approved these changes Jul 5, 2018

View reviewed changes

yajo requested changes Jul 9, 2018

View reviewed changes

ecino force-pushed the 10.0-mig-mail_forward branch 2 times, most recently from 2a8533e to cd66216 Compare July 9, 2018 07:43

yajo approved these changes Jul 9, 2018

View reviewed changes

Jairo Llopis and others added 2 commits July 9, 2018 14:22

[ADD] mail_forward

1d68d25

[MIG] module mail_forward: Migration to version 10.0

092a691

CO-1483

pedrobaeza force-pushed the 10.0-mig-mail_forward branch from cd66216 to 092a691 Compare July 9, 2018 12:23

pedrobaeza merged commit 15d2e40 into OCA:10.0 Jul 9, 2018

ecino deleted the 10.0-mig-mail_forward branch March 4, 2020 14:44

[10.0][MIG] migrate module mail_forward #256

[10.0][MIG] migrate module mail_forward #256

Conversation

ecino commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn left a comment

Choose a reason for hiding this comment

ecino commented Apr 16, 2018

ecino commented Apr 24, 2018

rafaelbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn left a comment

Choose a reason for hiding this comment

ecino commented Apr 27, 2018

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

yajo commented Apr 27, 2018

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

ecino commented Jun 4, 2018

yajo commented Jun 4, 2018

AaronHForgeFlow commented Jun 4, 2018

ecino commented Jun 4, 2018

ecino commented Jun 29, 2018

yajo commented Jul 2, 2018

ecino commented Jul 2, 2018

yajo left a comment • edited Loading

Choose a reason for hiding this comment

ecino commented Jul 2, 2018

ecino commented Jul 5, 2018

yajo commented Jul 5, 2018

ecino commented Jul 5, 2018

rafaelbn left a comment

Choose a reason for hiding this comment

antoniocanovas commented Jul 5, 2018

pedrobaeza commented Jul 6, 2018

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

yajo left a comment

Choose a reason for hiding this comment

yajo left a comment •

edited

Loading