Skip to content
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

[FW][FIX] base, account: remove group from all users when removing settings #41126

Conversation

@fw-bot
Copy link
Contributor

fw-bot commented Nov 29, 2019

Adding the implied users in a group is done automatically when adding
the implied_ids. Moreover it is done in SQL, bypassing record rules.

Asymmetrically, the users are not removed when removing a group; it is
done manually. However, since this is done via the ORM,
it takes record rules into account so some users might not be visible.

The standard case where this happens is multi-company.

Take the case of some mutually exclusive groups, TaxB2B and TaxB2C,
and a user U in company C1 and C2, in TaxB2B.
In company C2, change the settings to TaxB2C.
U is in another company, so U is not removed from TaxB2B.
But then U is added to taxB2C, which causes a validation error.

opw 2118337

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

Forward-Port-Of: #41022
Forward-Port-Of: #41010

@robodoo robodoo added the seen 🙂 label Nov 29, 2019
@fw-bot

This comment has been minimized.

Copy link
Contributor Author

fw-bot commented Nov 29, 2019

Ping @len-odoo
This PR targets master and is the last of the forward-port chain.

To merge the full chain, say

@fw-bot r+

More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port

@robodoo robodoo added the forwardport label Nov 29, 2019
Copy link
Contributor

len-odoo left a comment

fw-bot r+

@robodoo robodoo added the r+ 👌 label Nov 29, 2019
@C3POdoo C3POdoo added the OE label Nov 29, 2019
@len-odoo

This comment has been minimized.

Copy link
Contributor

len-odoo commented Dec 2, 2019

by DLE: Fixed by something like:
self.addCleanup(lambda: setattr(self.env.envs, 'envs', envs))
in SavepointCase's setup.

See with RCO/DLE.

@len-odoo len-odoo force-pushed the odoo-dev:master-12.0-opw2118337-taxgroups-len-TDmr-fw branch from bafacac to 5d94d86 Dec 2, 2019
@fw-bot

This comment has been minimized.

Copy link
Contributor Author

fw-bot commented Dec 2, 2019

This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @robodoo)

@robodoo robodoo removed the r+ 👌 label Dec 2, 2019
@d-fence

This comment has been minimized.

Copy link
Contributor

d-fence commented Dec 3, 2019

@len-odoo Hello, this Forwardport is targeting master and was created before the freeze saas-13.1. This means that you have cherry-pick it in saas-13.1 and use the ignore command for the forward-port bot.

len-odoo added 2 commits Nov 27, 2019
Initial issue solved by 3eb06a2
We keep the test since it ensures that settings can be saved properly.

X-original-commit: 2c1b92f
…ere installed/uninstalled

Execute the tests from ea574fe and 9e9ac5c,
which are in a SavepointCase.
The second test fails on a cache_miss.
Whats happens is the following:
 - the second test starts with the environment from the beginning of the
   first test. However its envs has a reference to environments that
   were created during the first test.
 - when computing the field, the ORM finds the value in a zombie
   environment, so does not recompute it
 - when trying to get the value, the value is not in cache

The issue is that going through res.config.settings's execute resets the
envs.

One solution is to put:
    self.addCleanup(lambda: setattr(self.env.envs, 'envs', envs))
in SavepointCase's setup.

However in that case the reset is not needed at all:
it's necessary only if there were some modules to install.
Unfortunately to_install and to_uninstall are not symmetric:
to_uninstall contains only modules that should be uninstalled, whereas
to_install contains everything, and only filters later what actions
really need to be performed.

In this case, we can rely on the result of the installation process.
@len-odoo len-odoo force-pushed the odoo-dev:master-12.0-opw2118337-taxgroups-len-TDmr-fw branch from 5d94d86 to 74b054b Dec 4, 2019
@robodoo robodoo added the CI 🤖 label Dec 4, 2019
Copy link
Contributor

len-odoo left a comment

robodoo rebase-ff r+

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Dec 5, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo added the r+ 👌 label Dec 5, 2019
robodoo pushed a commit that referenced this pull request Dec 5, 2019
…ere installed/uninstalled

Execute the tests from ea574fe and 9e9ac5c,
which are in a SavepointCase.
The second test fails on a cache_miss.
Whats happens is the following:
 - the second test starts with the environment from the beginning of the
   first test. However its envs has a reference to environments that
   were created during the first test.
 - when computing the field, the ORM finds the value in a zombie
   environment, so does not recompute it
 - when trying to get the value, the value is not in cache

The issue is that going through res.config.settings's execute resets the
envs.

One solution is to put:
    self.addCleanup(lambda: setattr(self.env.envs, 'envs', envs))
in SavepointCase's setup.

However in that case the reset is not needed at all:
it's necessary only if there were some modules to install.
Unfortunately to_install and to_uninstall are not symmetric:
to_uninstall contains only modules that should be uninstalled, whereas
to_install contains everything, and only filters later what actions
really need to be performed.

In this case, we can rely on the result of the installation process.

closes #41126

Signed-off-by: Nans Lefebvre (len) <len@odoo.com>
@robodoo robodoo closed this Dec 5, 2019
@robodoo robodoo deployed to merge Dec 5, 2019 Active
@len-odoo len-odoo deleted the odoo-dev:master-12.0-opw2118337-taxgroups-len-TDmr-fw branch Dec 5, 2019
bbh-odoo added a commit to odoo-dev/odoo that referenced this pull request Dec 9, 2019
…ere installed/uninstalled

Execute the tests from ea574fe and 9e9ac5c,
which are in a SavepointCase.
The second test fails on a cache_miss.
Whats happens is the following:
 - the second test starts with the environment from the beginning of the
   first test. However its envs has a reference to environments that
   were created during the first test.
 - when computing the field, the ORM finds the value in a zombie
   environment, so does not recompute it
 - when trying to get the value, the value is not in cache

The issue is that going through res.config.settings's execute resets the
envs.

One solution is to put:
    self.addCleanup(lambda: setattr(self.env.envs, 'envs', envs))
in SavepointCase's setup.

However in that case the reset is not needed at all:
it's necessary only if there were some modules to install.
Unfortunately to_install and to_uninstall are not symmetric:
to_uninstall contains only modules that should be uninstalled, whereas
to_install contains everything, and only filters later what actions
really need to be performed.

In this case, we can rely on the result of the installation process.

closes odoo#41126

Signed-off-by: Nans Lefebvre (len) <len@odoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.