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
Member

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 Ommo73:10.0-mail_private-add_channels branch from 1c48b2e to 44f0d3c Mar 20, 2019

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch 4 times, most recently from aa38c5a to f8a26ed Mar 20, 2019

@yelizariev
Copy link
Member

left a comment

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

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch from f8a26ed to a6896a8 Mar 22, 2019

@Ommo73

This comment has been minimized.

Copy link
Member Author

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/>

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

Find all contributors

This comment has been minimized.

Copy link
@Ommo73

Ommo73 Mar 25, 2019

Author Member

I added other authors

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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",

This comment has been minimized.

Copy link
@KolushovAlexandr
@@ -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/>

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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() {

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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();

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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) {

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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.

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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'])])

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

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

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 23, 2019

Member

Could someone explain?

This comment has been minimized.

Copy link
@Ramil-Mukhametzyanov

Ramil-Mukhametzyanov May 15, 2019

Member

I can guess that self.partner_ids may be undefined.

This comment has been minimized.

Copy link
@yelizariev

yelizariev May 16, 2019

Member

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

This comment has been minimized.

Copy link
@Ommo73

Ommo73 May 17, 2019

Author Member

@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.

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr Mar 24, 2019

Member

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 Ommo73:10.0-mail_private-add_channels branch 2 times, most recently from 124fd91 to 36e573a Mar 25, 2019

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch from 36e573a to 4655aa6 Apr 1, 2019

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch from 4655aa6 to add565a Apr 25, 2019

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 Ommo73:10.0-mail_private-add_channels branch 2 times, most recently from 3650cc7 to 41bc443 Apr 25, 2019


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

This comment has been minimized.

Copy link
@hound

hound bot Apr 26, 2019

Missing semicolon.


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

This comment has been minimized.

Copy link
@hound

hound bot Apr 26, 2019

Missing semicolon semi

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch from 41bc443 to 1696029 Apr 26, 2019

'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']]

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr May 8, 2019

Member

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

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr May 8, 2019

Member

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)

This comment has been minimized.

Copy link
@hound

hound bot May 13, 2019

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,

This comment has been minimized.

Copy link
@hound

hound bot May 13, 2019

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

This comment has been minimized.

Copy link
@hound

hound bot May 13, 2019

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

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch 2 times, most recently from 5dca30f to cc93723 May 13, 2019

@@ -52,13 +65,14 @@ Chatter.include({
open_composer: function (options) {
var self = this;
this._super.apply(this, arguments);
if (options && options.is_private) {

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr May 13, 2019

Member

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),

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr May 13, 2019

Member

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) {

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr May 13, 2019

Member

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

}

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

This comment has been minimized.

Copy link
@KolushovAlexandr

KolushovAlexandr May 13, 2019

Member

you can do it without a total method redefinition

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch 2 times, most recently from 1004659 to ef59080 May 14, 2019

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))),

This comment has been minimized.

Copy link
@hound

hound bot May 14, 2019

'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))),

This comment has been minimized.

Copy link
@hound

hound bot May 14, 2019

Expected parentheses around arrow function argument arrow-parens

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch 3 times, most recently from b8db881 to 00a0127 May 14, 2019

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

This comment has been minimized.

Copy link
@hound

hound bot May 15, 2019

Missing semicolon.

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

This comment has been minimized.

Copy link
@hound

hound bot May 15, 2019

Missing semicolon semi

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

@Ramil-Mukhametzyanov
Copy link
Member

left a comment

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

@Ramil-Mukhametzyanov
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

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.

@Ramil-Mukhametzyanov
Copy link
Member

left a comment

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 Ommo73:10.0-mail_private-add_channels branch from b6082ce to d9cd643 Jun 13, 2019

});
});
return checked_channels;
},

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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 () {

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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

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

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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

});
},

get_internal_users_ids: function () {

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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

});
});
});
},

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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 () {

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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

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

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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
});
},
},

This comment has been minimized.

Copy link
@hound

hound bot Jun 13, 2019

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

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch from d9cd643 to 843c7dd Jul 17, 2019

@yelizariev
Copy link
Member

left a comment

Why there are two commits with the same message?

@Ommo73

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Fix this lint please:

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

@yelizariev

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

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

Ommo73 added 2 commits Mar 19, 2019

@Ommo73 Ommo73 force-pushed the Ommo73:10.0-mail_private-add_channels branch from 843c7dd to 1ccc657 Aug 2, 2019

@yelizariev

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

I approve to merge it now

@itpp-bot itpp-bot merged commit 5a610a4 into it-projects-llc:10.0 Aug 11, 2019

5 of 6 checks passed

Travis CI - Pull Request Build Failed
Details
Hound No violations found. Woof!
ci/branches Branch names are correct
Details
ci/runbot runbot build 03270-182-c045b3 (runtime 228s)
Details
codecov/patch/backend Coverage not affected when comparing df22626...c045b3d
Details
codecov/patch/tests Coverage not affected when comparing df22626...c045b3d
Details
@itpp-bot

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

Approved by @yelizariev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.