Conversation
DorianMAG
left a comment
There was a problem hiding this comment.
Thx for the migration.
Functional test on runboat.
LGTM
JulienMartinez
left a comment
There was a problem hiding this comment.
Functional test on runboat.
LGTM
thx
flotho
left a comment
There was a problem hiding this comment.
Hi, thanks,
successfully tested on runboat
|
This PR has the |
7fbb070 to
c7497f3
Compare
|
Hi, thanks for your review! I've just made a small change. |
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: web-16.0/web-16.0-web_select_all_companies Translate-URL: https://translation.odoo-community.org/projects/web-16-0/web-16-0-web_select_all_companies/
c7497f3 to
4840b6f
Compare
|
/ocabot migration web_select_all_companies |
tarteo
left a comment
There was a problem hiding this comment.
Not sure if we should remove translation files, I couldn't test it on Runboat, but as 3 people already reviewed I assume it's okay!
| This module allows you to select or deselect all the companies in a | ||
| single click. | ||
|
|
||
|  |
There was a problem hiding this comment.
Don't you want to add your videos here?
HekkiMelody
left a comment
There was a problem hiding this comment.
Aside from the small fixes to the readme that have already been pointed out,
Code and functional review, LGTM
SirPyTech
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I tried it and works great, nice work! 👏
I have reviewed the code and I'm a bit worried of the missing super calls in the overrides, please let me know what you think about those.
I have also reviewed the commit history and some commits from 16.0 are missing, while others should still be squashed.
There was a problem hiding this comment.
chore: This migration is missing commits d7c496f and following from 16.0, please include them in the correct point of the history.
There was a problem hiding this comment.
chore: Thanks for squashing some of the administrative commits, I see there are still a few
They have to be squashed too, according to https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate.
There was a problem hiding this comment.
praise: Thanks for updating the documentation! That's something most developers skip.
There was a problem hiding this comment.
question: Maybe you'd like to include this video in the README? Otherwise please remove it.
There was a problem hiding this comment.
question: Maybe you'd like to include this video in the README? Otherwise please remove it.
| class ResUsers(models.Model): | ||
| _inherit = "res.users" | ||
|
|
||
| default_company_ids = fields.Many2many( |
There was a problem hiding this comment.
question: This is a new (welcome!) feature, would it be possible to implement it in its own [IMP] commit so it would be easier to backport?
| } | ||
| } | ||
|
|
||
| this.companyService.setCompanies(selectedIds, false); |
There was a problem hiding this comment.
suggestion: I'm not that expert as a frontend developer, but I see no super call in this function, would it be possible to add one so the inheritance chain is kept?
You could just set selectedCompaniesIds here and let super's call do the setCompanies, what do you think?
For reference, the function apply in super is https://github.com/odoo/odoo/blob/faa3c4bcabd523bbe5ea0221bcb95597ea004a69/addons/web/static/src/webclient/switch_company_menu/switch_company_menu.js#L51.
| ); | ||
| }, this.constructor.toggleDelay); | ||
| } | ||
| async confirm() { |
There was a problem hiding this comment.
suggestion: Same inheritance considerations of the apply override.
Changes
default_company_idsfield to res.users that will control which companies are selected by default