Issue 5581

Title
Improve validate error messages
Priority
feature
Status
resolved
Nosy list
acaubet, ced, guillemNaN, pokoli, resteve, reviewbot, roundup-bot, timitos, yangoon
Assigned to
ced
Keywords
review

Created on 2016-05-30.12:10:57 by guillemNaN, last changed 3 months ago by roundup-bot.

Messages

New changeset 484a7e89f662 by Cédric Krier in branch 'default':
Include record name and value in validation error message
https://hg.tryton.org/tryton-env/rev/484a7e89f662
New changeset 1b42f8379b1e by Cédric Krier in branch 'default':
Include record name and value in validation error message
https://hg.tryton.org/trytond/rev/1b42f8379b1e
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2020-12-17.17:49:13

Instead of try/except, I think it will be better to keep a list of created records during the transaction. Such that we could easily determine if the id will last of the rollback of the transaction or not. This would also help creating better warning key without volatile id.

Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2018-02-06.17:28:25
I'm wondering if we could not use the rec_name but under a try/except which will fallback to the id.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-09-23.14:51:58
El 22/09/16 a les 10:15, Cédric Krier ha escrit:
> On 2016-09-22 09:49, Sergi Almacellas Abellana wrote:
>>> > > Cédric Krier <cedric.krier@b2ck.com> added the comment:
>>> > > 
>>> > > I'm wondering if it should not be a logging feature instead of changing the error message because it is for debugging purpose.
>> > 
>> > Maybe we should add both options,
> For me, it is wrong to show ID to the end user.

I must admit that i'm not very convinced with the current proposal which includes the ID in the message as sometimes it wont be usufull for the user. Two examples: 

1. Fields inside One2Many as id is normally hiden. 
2. When failing record is a newly created record (already explained in the thread)
> 
>> > but in which log level are you thinking? Because for me using DEBUG should be so much intrusive.
> Maybe INFO because this is normally something that should never happen
> except if there is a bug or forged requests so it will not be too verbose.

Ok, so lets add it as INFO logging and forget about adding the ID in the error message.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-09-22.10:15:03
On 2016-09-22 09:49, Sergi Almacellas Abellana wrote:
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> > 
> > I'm wondering if it should not be a logging feature instead of changing the error message because it is for debugging purpose.
> 
> Maybe we should add both options,

For me, it is wrong to show ID to the end user.

> but in which log level are you thinking? Because for me using DEBUG should be so much intrusive.

Maybe INFO because this is normally something that should never happen
except if there is a bug or forged requests so it will not be too verbose.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-09-22.09:49:46
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> I'm wondering if it should not be a logging feature instead of changing the error message because it is for debugging purpose.

Maybe we should add both options, but in which log level are you thinking? Because for me using DEBUG should be so much intrusive.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-09-21.18:22:18
I'm wondering if it should not be a logging feature instead of changing the error message because it is for debugging purpose.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-09-21.18:00:14
I uploaded review26711002 which adds the id of the invalid record on the validate message.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-09-21.17:19:50
I assign the issue to myself in order to work on it.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-06-03.15:55:05
On 2016-06-03 15:17, Sergi Almacellas Abellana wrote:
> El 03/06/16 a les 15:00, Cédric Krier ha escrit:
> > On 2016-06-03 14:17, Sergi Almacellas Abellana wrote:
> >> >El 03/06/16 a les 14:05, Cédric Krier ha escrit:
> >>> > >On 2016-06-03 13:50, Sergi Almacellas Abellana wrote:
> >>>>> > >> >Showing the ID will be a great improvement but for me the rec_name is more clear for the end user. So maybe we can add both values, so in case the user can not find the record by the rec_name (it have changed) it can use the id.
> >>> > >I have explained previously that rec_name can not be used on invalid record.
> >> >And ID can not be used on new created records as the transaction will be
> >> >rolled back.
> > It can be used but of course after the transaction it will no longer be
> > present. But nothing can be done against that.
> 
> So in this case the rec_name will be usefull as will help you to 
> identify the new record that can not be created, specially when it's the 
> case of a batch processes that creates a lot of data.

It will be not helpful if it crashes during the generation of the first
error message. You can not access blindly methods on an invalid record.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-06-03.15:17:14
El 03/06/16 a les 15:00, Cédric Krier ha escrit:
> On 2016-06-03 14:17, Sergi Almacellas Abellana wrote:
>> >El 03/06/16 a les 14:05, Cédric Krier ha escrit:
>>> > >On 2016-06-03 13:50, Sergi Almacellas Abellana wrote:
>>>>> > >> >Showing the ID will be a great improvement but for me the rec_name is more clear for the end user. So maybe we can add both values, so in case the user can not find the record by the rec_name (it have changed) it can use the id.
>>> > >I have explained previously that rec_name can not be used on invalid record.
>> >And ID can not be used on new created records as the transaction will be
>> >rolled back.
> It can be used but of course after the transaction it will no longer be
> present. But nothing can be done against that.

So in this case the rec_name will be usefull as will help you to 
identify the new record that can not be created, specially when it's the 
case of a batch processes that creates a lot of data.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-06-03.15:00:13
On 2016-06-03 14:17, Sergi Almacellas Abellana wrote:
> El 03/06/16 a les 14:05, Cédric Krier ha escrit:
> > On 2016-06-03 13:50, Sergi Almacellas Abellana wrote:
> >> >Showing the ID will be a great improvement but for me the rec_name is more clear for the end user. So maybe we can add both values, so in case the user can not find the record by the rec_name (it have changed) it can use the id.
> > I have explained previously that rec_name can not be used on invalid record.
> And ID can not be used on new created records as the transaction will be 
> rolled back.

It can be used but of course after the transaction it will no longer be
present. But nothing can be done against that.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-06-03.14:17:35
El 03/06/16 a les 14:05, Cédric Krier ha escrit:
> On 2016-06-03 13:50, Sergi Almacellas Abellana wrote:
>> >Showing the ID will be a great improvement but for me the rec_name is more clear for the end user. So maybe we can add both values, so in case the user can not find the record by the rec_name (it have changed) it can use the id.
> I have explained previously that rec_name can not be used on invalid record.
And ID can not be used on new created records as the transaction will be 
rolled back.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-06-03.14:05:07
On 2016-06-03 13:50, Sergi Almacellas Abellana wrote:
> Showing the ID will be a great improvement but for me the rec_name is more clear for the end user. So maybe we can add both values, so in case the user can not find the record by the rec_name (it have changed) it can use the id. 

I have explained previously that rec_name can not be used on invalid record.
Author: [hidden] (pokoli) Tryton committer Tryton translator
Date: 2016-06-03.13:50:54
It will be great to improve all the errors that are raised on validate. 

Showing the ID will be a great improvement but for me the rec_name is more clear for the end user. So maybe we can add both values, so in case the user can not find the record by the rec_name (it have changed) it can use the id. 

For domain validation records, maybe we can provide a expression explaining the domain, like done on the GTK Client. BTW, i will treat this as a separate issue.
Author: [hidden] (guillemNaN)
Date: 2016-06-03.13:25:36
2016-06-03 13:10 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:

> Cédric Krier <cedric.krier@b2ck.com> added the comment:
> 
> On 2016-06-03 12:51, Guillem Barba wrote:
> > The situation that motivated this issue is a cron that process lot of
> > invoices for lot of companies.
> > The error raised usually is a domain problem or a required field error.
> > These errors doesn't  show which record fail.
> > Without more information that "in some of the thousands invoices there is
> > one that field X is missing", it so hard to find which is the problematic
> > invoice.
> > To have the company that failed allows the user to reduce the invoices to
> > check.
> 
> It does not sound like a real solution.

It works pretty well in our use case (the invoices are processed and committed in blocks of 20, so the user only need to check the first 20 of the reported company).
Any way, it was the solution available modifying less the core code (in 3.4 the patch was very easy because the send email action is done inside the context per company).

> > Another solution is to put the record rec_name in the domain/field required
> > error messages. Provably this solution will solve more UX problems.
> 
> Yes that sounds like a better solution.
> But usually rec_name getter is considering that the record is valid. So
> maybe the id is better.

I change the name of the issue to do this improvement.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-06-03.13:10:10
On 2016-06-03 12:51, Guillem Barba wrote:
> The situation that motivated this issue is a cron that process lot of
> invoices for lot of companies.
> The error raised usually is a domain problem or a required field error.
> These errors doesn't  show which record fail.
> Without more information that "in some of the thousands invoices there is
> one that field X is missing", it so hard to find which is the problematic
> invoice.
> To have the company that failed allows the user to reduce the invoices to
> check.

It does not sound like a real solution.

> Another solution is to put the record rec_name in the domain/field required
> error messages. Provably this solution will solve more UX problems.

Yes that sounds like a better solution.
But usually rec_name getter is considering that the record is valid. So
maybe the id is better.
Author: [hidden] (guillemNaN)
Date: 2016-06-03.12:51:42
2016-06-03 12:30 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:

>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> On 2016-06-03 12:10, Guillem Barba wrote:
> > 2016-06-03 10:37 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
> > > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> > >
> > > I don't think company is more important than any other context data.
> So it
> > > is the all context that needs to be put. But then we start to have a
> > > security concern because we are leaking data by emails.
> > >
> >
> > I disagree.
> > The company is a very important concept in the ERP. There are lot of data
> > (the main data) that are related to a company.
>
> It is not more important than any other data. But of course, if you are
> using the client to find the issue, you will have difficulties because
> of company record rules. But for me, it is the record rules the problem,
> not the cron message.
> If you talk about related data then it is probably the user Model who
> has the more links.
>

The situation that motivated this issue is a cron that process lot of
invoices for lot of companies.
The error raised usually is a domain problem or a required field error.
These errors doesn't  show which record fail.
Without more information that "in some of the thousands invoices there is
one that field X is missing", it so hard to find which is the problematic
invoice.
To have the company that failed allows the user to reduce the invoices to
check.

Another solution is to put the record rec_name in the domain/field required
error messages. Provably this solution will solve more UX problems.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-06-03.12:30:06
On 2016-06-03 12:10, Guillem Barba wrote:
> 2016-06-03 10:37 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:
> > Cédric Krier <cedric.krier@b2ck.com> added the comment:
> >
> > I don't think company is more important than any other context data. So it
> > is the all context that needs to be put. But then we start to have a
> > security concern because we are leaking data by emails.
> >
> 
> I disagree.
> The company is a very important concept in the ERP. There are lot of data
> (the main data) that are related to a company.

It is not more important than any other data. But of course, if you are
using the client to find the issue, you will have difficulties because
of company record rules. But for me, it is the record rules the problem,
not the cron message.
If you talk about related data then it is probably the user Model who
has the more links.

> And about security concert, the company is a public field of all models.
> At least, in the e-mail we send the database name and the traceback with
> paths, it could be more dangerous than the company name... I think

My concern about security was not about company but about the all
context.
About the URL, there is no sensitive data in it.
Author: [hidden] (guillemNaN)
Date: 2016-06-03.12:10:06
2016-06-03 10:37 GMT+02:00 Cédric Krier <issue_tracker@tryton.org>:

>
> Cédric Krier <cedric.krier@b2ck.com> added the comment:
>
> I don't think company is more important than any other context data. So it
> is the all context that needs to be put. But then we start to have a
> security concern because we are leaking data by emails.
>

I disagree.
The company is a very important concept in the ERP. There are lot of data
(the main data) that are related to a company.
And about security concert, the company is a public field of all models.
At least, in the e-mail we send the database name and the traceback with
paths, it could be more dangerous than the company name... I think

An alternative is to save in the cron the "execution" record (similar that
is done in BaBI module, but in that module the execution is more important
and provably less often than in cron) with an identifier and all relevant
information and in the e-mail send this identifier to be able to see the
traceback and the other information inside the ERP.

> Also you have the last company run written on the cron user.
>

No, because the send e-mail action is done after rollback.
I thought to move rollback after send e-mail, but despite the current
implementation of send e-mail doesn't save any data, I think it's not a
good idea to do this action before the rollback, because someone could want
to extend send e-mail saving something.

I think the only option is to have a function that executes the cron (like
the old "_callback") that is called in run() and run_once() an is the
function extended by company module.
Author: [hidden] (ced) Tryton committer Tryton translator
Date: 2016-06-03.10:37:30
I don't think company is more important than any other context data. So it is the all context that needs to be put. But then we start to have a security concern because we are leaking data by emails.

Also you have the last company run written on the cron user.
Author: [hidden] (guillemNaN)
Date: 2016-05-31.12:30:05
The current implementation of cron execution, using "run_once", doesn't allow this change because the send_error_message() is called out of "run_once" function and this functions is what is extended in company module.

In a multicompany database, with 40 companies, know in which company the cron execution has failed is a basic information to solve de problem... because the common problem is a domain error that doesn't give too much information.
Author: [hidden] (guillemNaN)
Date: 2016-05-30.12:10:55
The e-mail that is send when a cron tasks fails give very little information.
For example, in a multicompany environment it doesn't says which company fails, what makes hard to find the problem.

In 'trytond', split the send_error_message() function in ir.cron to facilitate extend it.
In 'company' module, put the failed company in the e-mail
History
Date User Action Args
2022-08-06 23:06:15roundup-botsetmessages: + msg77623
2022-08-06 23:06:07roundup-botsetmessages: + msg77622
nosy: + roundup-bot
status: testing -> resolved
2022-07-15 15:50:07reviewbotsetmessages: + msg77337
2022-07-15 14:44:58yangoonsetnosy: + yangoon
2022-07-15 14:20:54reviewbotsetmessages: + msg77335
2022-07-15 14:10:21cedsetstatus: in-progress -> testing
2022-07-15 14:04:55cedsetassignedto: pokoli -> ced
component: + trytond
superseder: - Statement line party should be required if account has requires party
2021-12-16 11:30:05acaubetsetnosy: + acaubet
2020-12-17 17:49:13cedsetmessages: + msg63328
2020-11-16 07:45:19timitossetnosy: + timitos
2020-11-15 20:17:58cedlinkissue9835 superseder
2018-02-07 17:20:13reviewbotsetmessages: + msg38277
2018-02-06 17:28:25cedsetmessages: + msg38246
2016-09-23 14:51:58pokolisetstatus: testing -> in-progress
messages: + msg28960
2016-09-22 10:15:04cedsetmessages: + msg28920
2016-09-22 09:59:11reviewbotsetmessages: + msg28919
2016-09-22 09:49:46pokolisetmessages: + msg28918
2016-09-21 18:30:16reviewbotsetnosy: + reviewbot
messages: + msg28908
2016-09-21 18:30:16reviewbotsetreviews: 26711002
keyword: + review
2016-09-21 18:22:18cedsetmessages: + msg28907
2016-09-21 18:00:14pokolisetstatus: in-progress -> testing
messages: + msg28906
2016-09-21 17:19:50pokolisetstatus: chatting -> in-progress
assignedto: guillemNaN -> pokoli
superseder: + Statement line party should be required if account has requires party
messages: + msg28904
2016-06-03 15:55:05cedsetmessages: + msg26094
2016-06-03 15:17:15pokolisetmessages: + msg26093
2016-06-03 15:00:13cedsetmessages: + msg26092
2016-06-03 14:17:36pokolisetmessages: + msg26091
2016-06-03 14:05:07cedsetmessages: + msg26090
2016-06-03 13:50:55pokolisetmessages: + msg26089
title: Add the record ID in domain and required field error messages -> Improve validate error messages
component: - trytond, company
keyword: - review
nosy: + pokoli
2016-06-03 13:26:35guillemNaNsetreviews: 25341002, 21011002 -> (no value)
2016-06-03 13:25:37guillemNaNsetmessages: + msg26088
title: Report on email about the company whose task failed -> Add the record ID in domain and required field error messages
2016-06-03 13:21:13guillemNaNsetfiles: - unnamed
2016-06-03 13:10:11cedsetmessages: + msg26087
2016-06-03 12:51:42guillemNaNsetfiles: + unnamed
messages: + msg26084
2016-06-03 12:30:07cedsetmessages: + msg26082
2016-06-03 12:11:00guillemNaNsetfiles: - unnamed
2016-06-03 12:10:07guillemNaNsetfiles: + unnamed
messages: + msg26080
2016-06-03 10:37:31cedsetnosy: + ced
messages: + msg26078
2016-05-31 12:45:49guillemNaNsetreviews: 25341002, 21011002
keyword: + review
2016-05-31 12:40:42guillemNaNsetcomponent: + trytond, company
2016-05-31 12:30:07guillemNaNsetstatus: unread -> chatting
messages: + msg26003
2016-05-30 22:31:06restevesetnosy: + resteve
2016-05-30 12:10:57guillemNaNcreate