-
-
Notifications
You must be signed in to change notification settings - Fork 867
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
[18.0][MIG] partner_multi_relation: Migration to 18.0 #1895
[18.0][MIG] partner_multi_relation: Migration to 18.0 #1895
Conversation
* API of _auto_init * Menu and groups stuff has moved from base to sales_team in odoo 10
* Fix error on search when leaf is (1, '=', 1) * Another comparison fix with search arguments
* Update README * Python 2to3 * Move relation menus items under contacts * Fix date comparison * Fix cache invalidate on relation write * Bump module version * Remove test_relation_type_unlink_dberror This test is not required because it tests a standard behavior of SQL. If a foreign key column is not nullable, then the foreign row can not be deleted. * Remove utf8 encoding comment * Add missing dependency for partner_multi_relation * Add missing active_test in view action
* Bug Fix * Enable additional columns on res_partner_relation_all. Enable to add additional columns from res_partner_relation in the sql view of res_partner_relation_all. * Only require inverse_name when relation type is not symmetric * Remove deprecated @api.one and replace openerp with odoo * Add website to manifest * Prevent disabling allow_self if reflexive relations exist * Enable deleting/ending invalid relations after deactivating reflexivity. * Handle case of invalid future reflexive relations
Currently translated at 86.6% (84 of 97 strings) Translation: partner-contact-12.0/partner-contact-12.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-12-0/partner-contact-12-0-partner_multi_relation/es/
[IMP] Flake8
Fix for error when opening a contact and clicking the smart button. Error Information: File "/opt/odoo/v12/src/partner-contact/partner_multi_relation/models/res_partner.py", line 187, in action_view_relations action['context'].\ AttributeError: 'str' object has no attribute 'update'
Currently translated at 46.4% (45 of 97 strings) Translation: partner-contact-12.0/partner-contact-12.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-12-0/partner-contact-12-0-partner_multi_relation/it/
…unction and no computed function defined
Currently translated at 100.0% (97 of 97 strings) Translation: partner-contact-13.0/partner-contact-13.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-13-0/partner-contact-13-0-partner_multi_relation/es_AR/
In Odoo v14, ir.actions.act_window have only access rights for admin while a user had read access in previous versions. Odoo provides now a function (_for_xml_id) that returns the action read with sudo.
Currently translated at 100.0% (97 of 97 strings) Translation: partner-contact-14.0/partner-contact-14.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-14-0/partner-contact-14-0-partner_multi_relation/pt/
[UPD] Update partner_multi_relation.pot
Currently translated at 85.4% (82 of 96 strings) Translation: partner-contact-16.0/partner-contact-16.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-16-0/partner-contact-16-0-partner_multi_relation/sl/
Currently translated at 100.0% (96 of 96 strings) Translation: partner-contact-16.0/partner-contact-16.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-16-0/partner-contact-16-0-partner_multi_relation/es/
Currently translated at 100.0% (96 of 96 strings) Translation: partner-contact-16.0/partner-contact-16.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-16-0/partner-contact-16-0-partner_multi_relation/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: partner-contact-17.0/partner-contact-17.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-17-0/partner-contact-17-0-partner_multi_relation/
Currently translated at 100.0% (95 of 95 strings) Translation: partner-contact-17.0/partner-contact-17.0-partner_multi_relation Translate-URL: https://translation.odoo-community.org/projects/partner-contact-17-0/partner-contact-17-0-partner_multi_relation/sv/
311cf71
to
63bed45
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.
LGTM, left some comments about _() param formatting.
self.env._( | ||
f"The {side} partner is not applicable for this " | ||
f"relation type." | ||
) |
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.
I think the best way to format parameters into a translated string is using:
_("Example %s", params), I am not sure how it works with an fstring or if it works at all.
This also appears in other places, and if the string inside the _() changes, then I think the .po file should also be updated accordingly.
Example:
_("Warning for %s", self.name)
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.
Translation work with fstrings, however this change was unnecessary here, as it would generate additional work to change it in translations.
Regarding _()
vs 'self.env._()` - the second option offers small performance improvements and is advised to use that while migrating the code to version 18
odoo/odoo#174844
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.
Hi Marcin, sorry, maybe I didn't explain myself very well. I meant only that the proper way to parse args into a translated string, AFAIK, is by providing the args/kwargs to the _ method, it doesn't matter if it's for the old way:
"_()" or the new "self.env._()".
Here is the doctstring of v18 self.env._ method:
"""Translate the term using current environment's language.
Usage:
self.env._("hello world") # dynamically get module name
self.env._("hello %s", "test")
self.env._(LAZY_TRANSLATION)
:param source: String to translate or lazy translation
:param ...: args or kwargs for templating
:return: The transalted string
"""
self.env._("hello %s", "test") <--- This is the one I was refering to, instead of doing
self.env._("hello %s") % "test" or .format, or an fstring... These will also work of course, but the _ method already provides options for this, so we should use them instead IMO.
This is another example with kwargs:
self.env._("%(name)s's Timesheets and Planning Analysis", name=self.name)
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.
My bad, I misunderstood you here. I agree with what you wrote and where it was easy fix not requiring changes in translation I went with your suggestions. Where it requires changes in translation files I didn't touch it to avoid problems with updating po files, which could easily lead to incorrect translations. I don't want to break something that is working 😅
Thank you for the great explanation.
63bed45
to
5a3c6f7
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.
LGTM!
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 PR has the |
/ocabot migration partner_multi_relation |
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.
Code review
if category and category.id not in partner.category_id.ids: | ||
raise ValidationError( | ||
_( | ||
self.env._( |
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 use translation function arguments instead of format.
assert "name" in vals, ( | ||
"Name_inverse missing in vals to create relation type. Vals: %s." % vals | ||
) | ||
assert ( |
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 are in an Odoo tests context, I would have used standard unitttest functions instead
in various ways. | ||
""" | ||
|
||
# pylint: disable=invalid-name |
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.
Do those pylint disables still applicable ?
) | ||
if operator not in SUPPORTED_OPERATORS: | ||
raise exceptions.ValidationError( | ||
self.env._('Unsupported search operator "%s"') % operator |
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 use translation arguments instead.
for contact in self: | ||
relation_model = self.env["res.partner.relation.all"] | ||
relation_ids = relation_model.search( | ||
[ |
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.
Putting this domain in action domain will avoid a search().
|
||
:return: a recordset of res.partner.relation. | ||
""" | ||
self.env.cr.execute( |
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.
I'm wondering if flush with fields should be done.
"type {relation_type}. There are existing reflexive " | ||
"relations defined for the following partners: " | ||
"{partners}" | ||
).format( |
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 use translation function arguments instead.
"""Delete existing reflexive relations for these relation types.""" | ||
for relation_type in self: | ||
relations = relation_type._get_reflexive_relations() | ||
relations.unlink() |
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 as above
if rec.handle_invalid_onchange == "delete": | ||
# Automatically delete relations, so existing relations | ||
# do not prevent unlink of relation type: | ||
relations = relation_model.search([("type_id", "=", rec.id)]) |
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.
I would have done one search with records in that case [('type_id' , 'in', self.filtered(lambda rel_type: rel_type.handle_invalid_onchange == "delete").ids)]
|
||
|
||
class TestPartnerRelationCommon(common.TransactionCase): | ||
def setUp(self): |
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 convert this to classmethod for performances reasons.
This PR is going to be replaced by this other one here. The reason is that the account that was used to open this one is not going to be used anymore. |
No description provided.