-
Couldn't load subscription status.
- Fork 30.2k
[IMP] various: improve user error wording #178821
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
[IMP] various: improve user error wording #178821
Conversation
| honest_move.action_post() | ||
|
|
||
| with self.assertRaisesRegex(UserError, 'not balanced'), self.env.cr.savepoint(): | ||
| with self.assertRaisesRegex(UserError, 'unbalanced'), self.env.cr.savepoint(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this; not balanced seems better.
| '!', ('company_id', 'child_of', company.id), | ||
| ], limit=1): | ||
| raise UserError(_("You can't set a different company on your analytic account since there are some analytic items linked to it.")) | ||
| raise UserError(_("No company-switching allowed for your analytic account while there are some linked analytic items around! It's a recipe for an analytical disaster!")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace by "You can't change the company of an analytic account having analytic items around! It's a recipe for an analytical disaster!""
| raise UserError(_('Impossible number %s: too many digits.', number)) | ||
| else: | ||
| raise UserError(_('Impossible number %s: probably invalid number of digits.', number)) | ||
| raise UserError(_("The format of the number %s is incorrect unless you're dialing aliens! Let's fix it.", number)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace by "The phone number %s is incorrect! Let's fix it - you are not dialing aliens."
8f8dbf4 to
f073493
Compare
407ecb6 to
cfafbf7
Compare
|
Hello @ticodoo @william-andre @dylankiss I have incorporated the suggestions discussed with Jihane. Kindly let me know if anything is missing or if you have any additional suggestions cc @xavierbol |
cfafbf7 to
f332521
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing important left for the accounting scope
1a0736c to
9576c89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure you correctly apply all of my grammar change suggestions. I don't have time to review all of the strings again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space + grammar issue + maybe better wording?:
| "The journal entry is not balanced," | |
| "with %(debit_total)s on the debit side and %(credit_total)s on the credit side. We need some equilibrium here!\n\n" | |
| "The journal entry is not balanced, " | |
| "with %(debit_total)s on the debit side and %(credit_total)s on the credit side, there needs to be an equilibrium!\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ticodoo Maybe another suggestion?
| "The journal entry is not balanced," | |
| "with %(debit_total)s on the debit side and %(credit_total)s on the credit side. We need some equilibrium here!\n\n" | |
| "The journal entry is not balanced. " | |
| "With %(debit_total)s on the debit side and %(credit_total)s on the credit side, we've disrupted the equilibrium!\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"we've disrupted the equilibrium" sounds a bit weird here. The comma/period change is fine. Doesn't really make a difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't post thin air! doesn't work. There's no way to fit in "air" into this sentence without it sounding weird/forced. It needs to be a completely different sentence.
Here's an AI generated option that sounds much better: "Even magicians can't post nothing!"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you applied my suggestion incorrectly. Same comment for line below
| raise UserError(_("Oops! You can't change the period/account for posted entries! Only the draft ones are up for such an adventure like that!")) | |
| raise UserError(_("Oops! You can't change the period/account for posted entries! Only the draft ones are up for an adventure like that!")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect grammar:
| raise UserError(_("You can't change the company of an analytic account having analytic items around! It's a recipe for an analytical disaster!")) | |
| raise UserError(_("You can't change the company of an analytic account that has analytic items lying around! It's a recipe for an analytical disaster!")) |
That being said, not sure if "lying around" is what the original change was going for. If not, then the more straightforward:
| raise UserError(_("You can't change the company of an analytic account having analytic items around! It's a recipe for an analytical disaster!")) | |
| raise UserError(_("You can't change the company of an analytic account that already has analytic items! It's a recipe for an analytical disaster!")) |
is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, but will be terrible in certain countries/cultures where smiling isn't as common as it is in the US/Belgium.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "Update All" button wording is a bit confusing. You're not updating all the records, only the ones you have selected. So maybe this is better?
| Update All | |
| Update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <p t-if="props.resIds.length > 1">Ready to make those web pages disappear into thin air? Are you sure?\n It will be gone forever!\n | |
| <span class="text-warning">But before deleting them, make sure you update all links referring to them. That way, your customers won't bump into the not-so-famous 404 error page.</span> | |
| </p> | |
| <p t-else="">Ready to make this web page disappear into thin air? Are you sure?\n It will be gone forever!\n | |
| <span class="text-warning">But before deleting it, make sure you update all links referring to it. That way, your customers won't bump into the not-so-famous 404 error page.</span> | |
| <p t-if="props.resIds.length > 1">Ready to make these web pages disappear into thin air? Are you sure?\n They will be gone forever!\n | |
| <span class="text-warning">But before deleting, make sure you update all links referring to them. That way, your customers won't bump into the not-so-famous 404 error page.</span> | |
| </p> | |
| <p t-else="">Ready to make this web page disappear into thin air? Are you sure?\n It will be gone forever!\n | |
| <span class="text-warning">But before deleting, make sure you update all links referring to it. That way, your customers won't bump into the not-so-famous 404 error page.</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'Uh-oh! You can’t delete a survey used as a Course Certification! Otherwise, students might think diplomas just grow on trees.\n' | |
| 'The courses that need it are:\n%s', | |
| 'Uh-oh! You can’t delete surveys used as a Course Certification! Otherwise, students might think diplomas just grow on trees.\n' | |
| 'The courses that need them are:\n%s', |
odoo/orm/models.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Our troublemaker is: %(model_display)s\n" | |
| "Blame the following constraint: %(field_display)s\n" | |
| "How about archiving the record instead?", | |
| "The troublemaker is: %(model_display)s\n" | |
| "Thanks to the following constraint: %(field_display)s\n" | |
| "How about archiving the record instead?", |
odoo/orm/models.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| lines.append(_("To avoid a mess, no company crossover allowed!")) | |
| lines.append(_("To avoid a mess, no company crossover is allowed!")) |
Shouldn't this kind of tasks be done by native (probably from the US) speakers...? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions, but also some important functional issues. Please check them carefully 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you directly raising a UserError here and below instead of appending it to the list of errors to show in the end? Because all the rest is left the same 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise UserError(_("Oops! You can't change the period/account for posted entries! Only the draft ones are up for such an adventure like that!")) | |
| raise UserError(_("Oops! You can only change the period or account for posted entries! Other ones aren't up for an adventure like that!")) |
Oops, seems like you totally changed the meaning of this message. The original message says that you can only change the period or account for posted journal items, not for other ones! Please correct this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ticodoo Maybe another suggestion?
| "The journal entry is not balanced," | |
| "with %(debit_total)s on the debit side and %(credit_total)s on the credit side. We need some equilibrium here!\n\n" | |
| "The journal entry is not balanced. " | |
| "With %(debit_total)s on the debit side and %(credit_total)s on the credit side, we've disrupted the equilibrium!\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above. Why raising here already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise ValidationError(_("Once a document is posted, its type is set in stone and you can't change it anymore.")) | |
| raise ValidationError(_("Once a document has been posted once, its type is set in stone and you can't change it anymore.")) |
You lost some nuance in the message. The type can't be changed anymore if the entry has ever been posted before. So even if they reset it to draft afterwards, it would still remain immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise UserError(error_message % state_description_values.get(self[:1].state)) | |
| raise UserError("You can't delete a time off request that is in %(leave_state)s state.", leave_state=state_description_values.get(self[:1].state)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ticodoo Maybe even this?
| raise UserError(_("You can't delete a time off which is in the past")) | |
| raise UserError(_("You can't delete a time off request that is in the past.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise UserError(error_message % (state_description_values.get(holiday.state),)) | |
| raise UserError("You can't delete a time off request that is in %(leave_state)s state.", leave_state=state_description_values.get(holiday.state)) |
addons/lunch/models/lunch_order.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to validate if this is only for food, and not for drinks too. Maybe this is better? @ticodoo
| raise ValidationError(_('Oh no! You don’t have enough money in your wallet to order that food! Contact your lunch manager to add some money in your wallet.')) | |
| raise ValidationError(_('Oh no! You don’t have enough money in your wallet to order your selected lunch! Contact your lunch manager to add some money to your wallet.')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also for drinks. If we want to keep it generic, we can make it your selected items, in case someone is using the lunch app...for other non-lunch meals/things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, this line looks extremely awkward in context
Totally agree. I don't really understand the purpose in this context 🤔
9576c89 to
d3e3ead
Compare
|
@robodoo r+ |
1 similar comment
|
@robodoo r+ |
|
This PR is already reviewed, reviewing it again is useless. |
87cb46f to
8a24aec
Compare
e3119e1 to
c40d7f7
Compare
|
Hello @ppr-odoo Can you rebase your branch? We will surely merge that this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot we directly write those messages in the template as before? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check the result in the pot file.
Having it in template with a t-esc in the middle makes separate translation phrases for each part such as "are valid for this update." (alone by itself) which makes no sense for translators.
Not only is it hard to understand, but different languages need the counter to be at different places in the phrase.
It's important to never concatenate parts of translations together in code (or in template) as those usually only work in English.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. 🙂
Thanks for the explanation! 🙂
Before this commit error messages in odoo are boring and not much attractive to user. Those were like odoo is preventing them from doing something user want to do. In this commit, we have modified the message to be a more friendly and humorous tone, making it less tedious and more enjoyable. It aims to enhance the user experience and ensure that interactions with any application are both pleasant and informative. As a result, the messages have been clear, short, easy to understand and informative. task-3422664 Co-authored-by: Kartik Chavda <kcv@odoo.com> Co-authored-by: Kamlesh Pathekar (kpt) <kpt@odoo.com> Co-authored-by: Aman Patel (ampa) <ampa@odoo.com>
c40d7f7 to
fbede01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robodoo r+
Before this commit error messages in odoo are boring and not much attractive to user. Those were like odoo is preventing them from doing something user want to do. In this commit, we have modified the message to be a more friendly and humorous tone, making it less tedious and more enjoyable. It aims to enhance the user experience and ensure that interactions with any application are both pleasant and informative. As a result, the messages have been clear, short, easy to understand and informative. task-3422664 closes #178821 Related: odoo/enterprise#69284 Signed-off-by: Xavier Bol (xbo) <xbo@odoo.com> Co-authored-by: Kartik Chavda <kcv@odoo.com> Co-authored-by: Kamlesh Pathekar (kpt) <kpt@odoo.com> Co-authored-by: Aman Patel (ampa) <ampa@odoo.com>

Before this commit error messages in odoo are boring and not much attractive to user. Those were like odoo is preventing them from doing something user want to do.
In this commit, we have modified the message to be a more friendly and humorous tone, making it less tedious and more enjoyable. It aims to enhance the user experience and ensure that interactions with any application are both pleasant and informative. As a result, the messages have been clear, short, easy to understand and informative.
Task link
task-3422664