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 mail private add channels #182

Merged
merged 3 commits into from
Aug 11, 2019

Conversation

Ommo73
Copy link

@Ommo73 Ommo73 commented Mar 19, 2019

Added ability to select channels for private message sending

mail_private/static/src/js/mail_private.js Outdated Show resolved Hide resolved
mail_private/models.py Outdated Show resolved Hide resolved
mail_private/models.py Show resolved Hide resolved
@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from 1c48b2e to 44f0d3c Compare March 20, 2019 10:10
@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch 4 times, most recently from aa38c5a to f8a26ed Compare March 21, 2019 18:37
Copy link
Collaborator

@yelizariev yelizariev left a comment

Choose a reason for hiding this comment

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

My comments from #183 (review) can be applied here too

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from f8a26ed to a6896a8 Compare March 22, 2019 12:33
@Ommo73
Copy link
Author

Ommo73 commented Mar 22, 2019

My comments from #183 (review) can be applied here too

I corrected it

@@ -1,10 +1,13 @@
# -*- coding: utf-8 -*-
# Copyright 2018 Ruslan Ronzhin <https://it-projects.info/team/rusllan/>

Choose a reason for hiding this comment

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

Find all contributors

Copy link
Author

Choose a reason for hiding this comment

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

I added other authors

@@ -1,3 +1,7 @@
`1.1.5`

Choose a reason for hiding this comment

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

1.1.0

@@ -1,3 +1,6 @@
/* Copyright 2018 Ruslan Ronzhin <https://it-projects.info/team/rusllan/>

Choose a reason for hiding this comment

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

copyrights are totally wrong

{
"name": """Internal Messaging""",
"summary": """Send private messages to specified recipients, regardless of who are in followers list.""",
"category": "Discuss",
"images": ['images/mail_private_image.png'],
"version": "10.0.1.0.1",
"version": "10.0.1.1.1",

Choose a reason for hiding this comment

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

10.0.1.1.0

@@ -1,4 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<!--Copyright 2017 Artyom Losev <https://github.com/ArtyomLosev>
Copyright 2018 Kolushov Alexandr <https://it-projects.info/team/KolushovAlexandr>
Copyright 2019 Artem Rafailov <https://it-projects.info/team/Ommo73/>

Choose a reason for hiding this comment

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

Why did you set yourself here if there are no any changes by you?

get_checked_channels_ids: function () {
var self = this;
var checked_channels = [];
this.$('.o_composer_suggested_partners:eq(1) input:checked').each(function() {

Choose a reason for hiding this comment

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

Rename or add a new class/id. It confuses, the method is about channels, but we looking through the partners

// for module mail_private
if (self.options.is_private) {
message.context.is_private = true;
message.context.channel_ids = self.get_checked_channels_ids();

Choose a reason for hiding this comment

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

Try to find a way to do it like for partner_ids, not via context.

@@ -177,6 +181,12 @@ var MailComposer = composer.BasicComposer.extend({
message.subtype = 'mail.mt_note';
}

// for module mail_private
if (self.options.is_private) {

Choose a reason for hiding this comment

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

Not sure that it's needed here. Is there any reason for that? In 12v I remember was a function that redefines values to a new object without is_private attribute.

Choose a reason for hiding this comment

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

Is there any way to avoid updating mail_base?

# TDE CHECK: add partners / channels as arguments to be able to notify a message with / without computation ??
self.ensure_one() # tde: not sure, just for testinh, will see
partners = self.env['res.partner'] | self.partner_ids
channels = self.env['mail.channel'].search([('id', '=', self._context['channel_ids'])])

Choose a reason for hiding this comment

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

use 'browse' not search.


# TDE CHECK: add partners / channels as arguments to be able to notify a message with / without computation ??
self.ensure_one() # tde: not sure, just for testinh, will see
partners = self.env['res.partner'] | self.partner_ids

Choose a reason for hiding this comment

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

Don't understand this construction because
self.env['res.partner'] | self.partner_ids = self.partner_ids

Choose a reason for hiding this comment

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

Could someone explain?

Choose a reason for hiding this comment

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

I can guess that self.partner_ids may be undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty self.partner_ids is the same as self.env['res.partner'].
Was this code copy-paster from odoo? If so, one can try to make a PR that fix this

Copy link
Author

Choose a reason for hiding this comment

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

@yelizariev yes this code copy-paster from odoo


@api.multi
def _notify_mail_private(self, force_send=False, send_after_commit=True, user_signature=True):
""" Add the related record followers to the destination partner_ids if is not a private message.

Choose a reason for hiding this comment

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

also, instead of this comment, it will be good to write a comment that the method was copy-pasted from odoo and describe the difference from the origin (and show where the code was changed)

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch 2 times, most recently from 124fd91 to 36e573a Compare March 29, 2019 06:49
@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from 36e573a to 4655aa6 Compare April 1, 2019 10:40
@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from 4655aa6 to add565a Compare April 25, 2019 11:45
mail_private/models.py Outdated Show resolved Hide resolved
mail_private/models.py Outdated Show resolved Hide resolved
mail_private/models.py Outdated Show resolved Hide resolved
mail_private/models.py Outdated Show resolved Hide resolved
mail_private/models.py Outdated Show resolved Hide resolved
@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch 2 times, most recently from 3650cc7 to 41bc443 Compare April 26, 2019 11:55

if (options && options.is_private) {
self.internal_users_ids = options.internal_ids
Copy link

Choose a reason for hiding this comment

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

Missing semicolon.


if (options && options.is_private) {
self.internal_users_ids = options.internal_ids
Copy link

Choose a reason for hiding this comment

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

Missing semicolon semi

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from 41bc443 to 1696029 Compare April 26, 2019 12:54
'parent_id': parent_id,
'subtype_id': subtype_id,
'partner_ids': [(4, pid) for pid in partner_ids],
'channel_ids': [(4, pid) for pid in kwargs['channel_ids']]

Choose a reason for hiding this comment

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

Update kwargs at first and then call the super with updated kwargs. No need for copy-paste here

Choose a reason for hiding this comment

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

kwargs['channel_ids'] = [(4, pid) for pid in kwargs['channel_ids']]
super(MailThread, self).message_post(idk, kwargs)

kwargs['channel_ids'] = [(4, pid) for pid in kwargs['channel_ids']]
return super(MailThread, self).message_post(body, subject, message_type,
subtype, parent_id, attachments,
content_subtype, **kwargs)
Copy link

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

content_subtype='html', **kwargs):
kwargs['channel_ids'] = [(4, pid) for pid in kwargs['channel_ids']]
return super(MailThread, self).message_post(body, subject, message_type,
subtype, parent_id, attachments,
Copy link

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

# Copyright 2019 Artem Rafailov <https://it-projects.info/team/Ommo73/>
# License LGPL-3.0 (https://www.gnu.org/licenses/lgpl.html).

from odoo import models, fields, api, exceptions, _, tools
Copy link

Choose a reason for hiding this comment

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

'odoo._' imported but unused
'odoo.exceptions' imported but unused
'odoo.tools' imported but unused

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch 2 times, most recently from 5dca30f to cc93723 Compare May 13, 2019 12:35
@@ -52,13 +65,14 @@ Chatter.include({
open_composer: function (options) {
var self = this;
this._super.apply(this, arguments);
if (options && options.is_private) {

Choose a reason for hiding this comment

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

Artem, keep the commit history clean. Each PR you give me to check has this kind of problems. Use git diff and check sent code here. 3 places need to be changed in this PR.

this.composer.options.is_private = options.is_private;

_.each(self.recipients_for_internal_message, function (partner) {
self.composer.suggested_partners.push({
checked: (partner.user_ids.length > 0),
checked: (self.internal_users_ids.indexOf(partner.user_ids[0]) !== -1),

Choose a reason for hiding this comment

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

and again, why do you check only the first id? There might be several of them


// Fetch partner ids
return new Model('mail.followers').call(
'read', [follower_ids, ['channel_id']]).then(function (res_partners) {

Choose a reason for hiding this comment

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

it's about channels, not partners. Update all variable names in the function

}

// for module mail_private
if (self.options.is_private) {

Choose a reason for hiding this comment

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

you can do it without a total method redefinition

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch 2 times, most recently from 1004659 to ef59080 Compare May 14, 2019 08:38
this.composer.options.is_private = options.is_private;

_.each(self.recipients_for_internal_message, function (partner) {
self.composer.suggested_partners.push({
checked: (partner.user_ids.length > 0),
checked: (self.internal_users_ids.some(r=> partner.user_ids.includes(r))),
Copy link

Choose a reason for hiding this comment

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

'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').

this.composer.options.is_private = options.is_private;

_.each(self.recipients_for_internal_message, function (partner) {
self.composer.suggested_partners.push({
checked: (partner.user_ids.length > 0),
checked: (self.internal_users_ids.some(r=> partner.user_ids.includes(r))),
Copy link

Choose a reason for hiding this comment

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

Expected parentheses around arrow function argument arrow-parens

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch 4 times, most recently from b8db881 to 00a0127 Compare May 15, 2019 11:34
if (self.options.is_private) {
self.context.is_private = true;
}
return this._super()
Copy link

Choose a reason for hiding this comment

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

Missing semicolon.

if (self.options.is_private) {
self.context.is_private = true;
}
return this._super()
Copy link

Choose a reason for hiding this comment

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

Missing semicolon semi

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from 00a0127 to b45f5b7 Compare May 15, 2019 11:35
Copy link

@Ramil-Mukhametzyanov Ramil-Mukhametzyanov left a comment

Choose a reason for hiding this comment

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

Change the button name to "Add Channels" in the pop-up window

Copy link

@Ramil-Mukhametzyanov Ramil-Mukhametzyanov left a comment

Choose a reason for hiding this comment

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

If a message is sent to a task follower via "Send internal message" (for example, from demo to admin), it is noted in the task discussion, rather than being sent.
An error occurred when admin clicked "Send internal message" in this example, but it has not been reproduced.

@Ommo73
Copy link
Author

Ommo73 commented May 21, 2019

Change the button name to "Add Channels" in the pop-up window

This part of the interface does not apply to our module.

@Ramil-Mukhametzyanov
Copy link

If a message is sent to a task follower via "Send internal message" (for example, from demo to admin), it is noted in the task discussion, rather than being sent.
An error occurred when admin clicked "Send internal message" in this example, but it has not been reproduced.

@Ramil-Mukhametzyanov
Copy link

If a message is sent to a task follower via "Send internal message" (for example, from demo to admin), it is noted in the task discussion, rather than being sent.
This part works the same way in current module version. It's OK.

An error occurred when admin clicked "Send internal message" in this example, but it has not been reproduced.
The error rises when the button is double clicked. It's also OK.

Copy link

@Ramil-Mukhametzyanov Ramil-Mukhametzyanov left a comment

Choose a reason for hiding this comment

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

Messages sent to a channel via "Send internal message" do not appear on the Discuss dashboard.
And even messages sent via "New message" don't appear there after that if the page is not reloaded.

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from b6082ce to d9cd643 Compare June 13, 2019 17:13
});
});
return checked_channels;
},
Copy link

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.
Expected an identifier and instead saw ','.
Missing semicolon.

return this.users_ids;
},

get_checked_channels_ids: function () {
Copy link

Choose a reason for hiding this comment

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

Label 'get_checked_channels_ids' on function statement.
Missing name in function declaration.

return users_ids;
});
return this.users_ids;
},
Copy link

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.
Expected an identifier and instead saw ','.
Missing semicolon.

});
},

get_internal_users_ids: function () {
Copy link

Choose a reason for hiding this comment

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

Label 'get_internal_users_ids' on function statement.
Missing name in function declaration.

});
});
});
},
Copy link

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.
Expected an identifier and instead saw ','.
Missing semicolon.

}
},

get_channels_for_internal_message: function () {
Copy link

Choose a reason for hiding this comment

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

Label 'get_channels_for_internal_message' on function statement.
Missing name in function declaration.

@@ -101,20 +124,86 @@ Chatter.include({
});
});
});
}
},
Copy link

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.
Expected an identifier and instead saw ','.
Missing semicolon.

@@ -33,32 +41,38 @@ Chatter.include({
}).fail(function () {
// todo: display notification
});
},
},
Copy link

Choose a reason for hiding this comment

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

Expected an assignment or function call and instead saw an expression.
Expected an identifier and instead saw ','.
Missing semicolon.

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from d9cd643 to 843c7dd Compare July 17, 2019 13:28
Copy link
Collaborator

@yelizariev yelizariev left a comment

Choose a reason for hiding this comment

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

Why there are two commits with the same message?

@Ommo73
Copy link
Author

Ommo73 commented Jul 30, 2019

Why there are two commits with the same message?

It's because mail_private module refers to the mail_base module. I add one new function that is in the commit message, but changes occur in two modules.

@yelizariev
Copy link
Collaborator

Fix this lint please:

mail_base/models.py:19: [W0622(redefined-builtin), MailMessage.write] Redefining built-in 'id'

@yelizariev
Copy link
Collaborator

It seems that Agrolait user is selected automatically, while he is not Internal Users
screenshot-github.aaakk.us.kg-2019 07 31-13_23_52

@Ommo73 Ommo73 force-pushed the 10.0-mail_private-add_channels branch from 843c7dd to 1ccc657 Compare August 2, 2019 13:42
@yelizariev
Copy link
Collaborator

I approve to merge it now

@itpp-bot itpp-bot merged commit 5a610a4 into itpp-labs:10.0 Aug 11, 2019
@itpp-bot
Copy link
Contributor

Approved by @yelizariev

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.

5 participants