Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(143)

Issue 39051002: notification_email: Add fallback users for the recipients

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by nicoe
Modified:
6 days ago
Reviewers:
pokoli, ced, reviewbot
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 15

Patch Set 2 : fix remarks #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -33 lines) Patch
M CHANGELOG View 1 1 chunk +3 lines, -1 line 0 comments Download
M notification.py View 1 6 chunks +44 lines, -0 lines 1 comment Download
M tests/test_notification_email.py View 1 6 chunks +73 lines, -29 lines 0 comments Download
M view/email_form.xml View 1 chunk +9 lines, -3 lines 1 comment Download

Messages

Total messages: 12
nicoe
1 month ago (2017-12-22 15:57:55 UTC) #1
reviewbot
https://codereview.tryton.org/39051002/diff/1/tests/test_notification_email.py#newcode251 tests/test_notification_email.py:251: E302 expected 2 blank lines, found 1 URL: https://codereview.tryton.org/39051002
1 month ago (2017-12-22 16:21:10 UTC) #2
ced
Missing changelog entry. https://codereview.tryton.org/39051002/diff/1/notification.py File notification.py (right): https://codereview.tryton.org/39051002/diff/1/notification.py#newcode59 notification.py:59: fallback_recipients = fields.Many2One( Why is it ...
1 month ago (2017-12-22 22:45:23 UTC) #3
nicoe
fix remarks
3 weeks, 5 days ago (2017-12-28 13:25:54 UTC) #4
reviewbot
flake8 OK URL: https://codereview.tryton.org/39051002
3 weeks, 5 days ago (2017-12-28 13:59:23 UTC) #5
nicoe
https://codereview.tryton.org/39051002/diff/1/notification.py File notification.py (right): https://codereview.tryton.org/39051002/diff/1/notification.py#newcode59 notification.py:59: fallback_recipients = fields.Many2One( On 2017/12/22 22:45:23, ced wrote: > ...
3 weeks, 5 days ago (2017-12-28 14:12:46 UTC) #6
ced
https://codereview.tryton.org/39051002/diff/1/notification.py File notification.py (right): https://codereview.tryton.org/39051002/diff/1/notification.py#newcode59 notification.py:59: fallback_recipients = fields.Many2One( On 2017/12/28 14:12:46, nicoe wrote: > ...
3 weeks, 5 days ago (2017-12-28 14:37:09 UTC) #7
ced
OK to keep plurals on fallback field names. https://codereview.tryton.org/39051002/diff/20001/notification.py File notification.py (right): https://codereview.tryton.org/39051002/diff/20001/notification.py#newcode85 notification.py:85: 'invisible': ...
3 weeks ago (2018-01-02 11:53:16 UTC) #8
pokoli
I'm wondering if it's really required to have 3 fields for fallback. Won't be enought ...
6 days, 1 hour ago (2018-01-18 09:43:44 UTC) #9
ced
On 2018/01/18 09:43:44, pokoli wrote: > I'm wondering if it's really required to have 3 ...
6 days, 1 hour ago (2018-01-18 10:01:42 UTC) #10
pokoli
On 2018/01/18 10:01:42, ced wrote: > On 2018/01/18 09:43:44, pokoli wrote: > > I'm wondering ...
6 days ago (2018-01-18 10:10:24 UTC) #11
ced
6 days ago (2018-01-18 10:42:01 UTC) #12
On 2018/01/18 10:10:24, pokoli wrote:
> On 2018/01/18 10:01:42, ced wrote:
> > On 2018/01/18 09:43:44, pokoli wrote:
> > > I'm wondering if it's really required to have 3 fields for fallback. 
> > > 
> > > Won't be enought to have one fallback user and send to him in case of any
> > > address fails?
> > 
> > They have different purposes.
> > Also can you define what is "address fails"?
> 
> Any recipient that does not have the email address. 

Then for me, this behavior is not useful. For example, I could not care that the
BCC is empty as far the the To is filled. So I do not want to receive a fallback
for missing BCC but I do want for missing TO.

> I understand they have different purposes for sending, but I do not see any
> different purposes for fallback. For me the fallback is to notice that
something
> went wrong. 

Not wrong because the module behaves as expected. It is just to send the email
any way.

> Indeed, we should probably modify the message to send a text indicating that
> something went wrong, otherwise is not possible to differentiate if this is a
> fallback message or a normal message. Maybe sending a fallback text and
> attaching the originial message will  be the best option.

It will not be possible if we try to have the behavior explained above.
For me, the email address is enough to know manual action must be taken.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld bc8b922