-
-
Notifications
You must be signed in to change notification settings - Fork 26
[16.0][REF] cooperator: moving from legal_form to partner_company_type #167
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
base: 16.0
Are you sure you want to change the base?
Conversation
bf495c1 to
36ec27f
Compare
huguesdk
left a comment
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.
mostly lgtm, thanks for this, @mihien!
small but important problem: after filling the /become_company_cooperator form, the selected company type is not set on the subscription request. for this to work, you need to modify cooperator_website/views/subscription_template.xml and replace any occurence of company_type by partner_company_type_id.
cooperator/tests/test_cooperator.py
Outdated
| # an existing type cannot be used because they are only defined in | ||
| # localization modules. |
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 comment should be removed.
l10n_be_cooperator/__manifest__.py
Outdated
| "name": "Cooperators Belgium", | ||
| "summary": "Cooperators Belgium Localization", | ||
| "version": "16.0.1.2.1", | ||
| "version": "16.0.3.0.0", |
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.
| "version": "16.0.3.0.0", | |
| "version": "16.0.2.0.0", |
|
|
||
|
|
||
| def migrate(cr, version): | ||
|
|
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.
there shouldn’t be a blank line here.
| # module. We're using the fact that the label of the legal_form selection | ||
| # field is the same as the ids of the records in l10n_be_company_types. |
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.
| # module. We're using the fact that the label of the legal_form selection | |
| # field is the same as the ids of the records in l10n_be_company_types. | |
| # module. We're using the fact that the values of the legal_form selection | |
| # field are the same as the xml ids of the records in l10n_be_partner_company_type. |
|
|
||
|
|
||
| def migrate(cr, version): | ||
|
|
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.
| # Remove the old values in res_partner.legal_form to prepare the database for | ||
| # those fields to be deleted. Otherwise the values added by add_selection | ||
| # will not be valid anymore but would still be present in the db. | ||
| cr.execute( | ||
| """ | ||
| UPDATE res_partner | ||
| SET legal_form = NULL | ||
| """ | ||
| ) | ||
|
|
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.
is this needed? the column will be deleted no matter what. same remark for subscription_request.company_type.
| company_types_records = ( | ||
| request.env["res.partner.company.type"].sudo().search([]) | ||
| ) | ||
| company_types = [(record.id, record.name) for record in company_types_records] |
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 it would be a good addition to also display the abbreviation next to the name:
| company_types = [(record.id, record.name) for record in company_types_records] | |
| company_types = [ | |
| ( | |
| record.id, | |
| "{name} ({abbr})".format(name=record.name, abbr=record.shortcut), | |
| ) | |
| for record in company_types_records | |
| ] |
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.
actually, i’m not so sure about this. maybe this should be changed directly in the partner_company_type module (for example, having a “display name” field), to ensure consistency (also in the internal partner forms, for example).
| values["company_name"] = partner.name | ||
| values["company_email"] = partner.email | ||
| values["company_type"] = partner.legal_form | ||
| values["partner_company_type_id"] = partner.partner_company_type_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.
| values["partner_company_type_id"] = partner.partner_company_type_id | |
| values["partner_company_type_id"] = partner.partner_company_type_id.id |
| values["products"] = products | ||
| fields_desc = sub_req_obj.sudo().fields_get(["company_type", "gender"]) | ||
| values["company_types"] = fields_desc["company_type"]["selection"] | ||
| values["company_types"] = self.get_company_types() |
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.
| values["company_types"] = self.get_company_types() | |
| values["partner_company_type_ids"] = self.get_company_types() |
3648c66 to
ed5e7be
Compare
20351b0 to
b582938
Compare
huguesdk
left a comment
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.
just a few things to clean up, but apart from that, lgtm. thanks for this important (and tricky) improvement, @mihien!
| if not values.get("activities_country_id"): | ||
| if company.default_country_id: | ||
| values["activities_country_id"] = company.default_country_id.id | ||
| else: | ||
| values["activities_country_id"] = "20" | ||
| values["activities_country_id"] = 20 |
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 part is obsolete (i didn’t know about this field and it is used nowhere anymore) and can be removed.
6dce7b0 to
70d41dc
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.
i did the finishing touches myself so we can deploy. i added your changes to l10n_ch_cooperator here (after merging #146) and added a migration script.
i changed my mind about changes in readme files: pre-commit generates these changes anyway, so logically they should be added. but we need to ensure that the maintainer-tools repo used locally by pre-commit is up to date (and used with a recent python version), otherwise it could introduce changes that would be reverted after the merge.
70d41dc to
818bb8a
Compare
* adapt subscription form to changes in cooperator_website. * fix layout of bank account number and vat fields.
818bb8a to
11afe6a
Compare
The current legal_form field that represents the company types is currently hard coded in each localization module and can not be modified by the user to add or remove a value.
Replacing this value by the OCA module partner_company_type, makes this field more generic. Simplifying localization which now requires just either a module data or can be added directly from the user interface.
edit (by @huguesdk): this depends on: