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

[FIX] mail: allow to assign an activity to an user currently in anoth… #30810

Closed
wants to merge 1 commit into
base: 12.0
from

Conversation

Projects
None yet
4 participants
@len-odoo
Copy link
Contributor

len-odoo commented Feb 4, 2019

…er company

Let users A, B be in in companies C1 and C2,
such that A is currently in C1 and B in C2.
User A tries to assign to B an activity on record R belonging to C1.
It fails because at the time of the check B is not in the correct company,
even if B could handle the activity after switching company.

Therefore before checking the record rule we switch the user company
to the record company if needed.

The _check_access_assignation was added in commit 96a223a,
to avoid activities to be created for records that the target user can't read.
Previous use-case is a legitimate one where we want to bypass this restriction.
Note that the systray notification widget is not company dependent,
and there is no way to remove a notification on a record the user can't read.*

In case where the record is company dependent and on a different company,
we loop on all the user's available companies to check if one would convene,
so that it doesn't impact the standard case.
Note that we use the do_in_draft environment to avoid writing useless lines.

*Better solution for master is to give the user some more explicit message
if reading the record, suggesting to switch company,
or to allow to dismiss the notification altogether.

opw 1933862

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

@len-odoo len-odoo requested a review from nim-odoo Feb 4, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

Moreover, you are missing the child companies... So if you want to go in this direction, you need to loop on all the companies. But I am not convinced we should do it.

Show resolved Hide resolved addons/mail/models/mail_activity.py Outdated
Show resolved Hide resolved addons/mail/models/mail_activity.py Outdated

@C3POdoo C3POdoo added the OE label Feb 4, 2019

@len-odoo len-odoo force-pushed the odoo-dev:12.0-opw1933862-chck-len branch Feb 4, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 4, 2019

@len-odoo

This comment has been minimized.

Copy link
Contributor Author

len-odoo commented Feb 4, 2019

Updated for the shortest sensible possible version I found including al's suggestions.

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

Mouais...

addons/mail/models/mail_activity.py Outdated
target_user = activity.user_id
current_company = target_user.company_id
target_record = self.env[activity.res_model].browse(activity.res_id)
if hasattr(target_record, 'company_id') and (

This comment has been minimized.

@nim-odoo

nim-odoo Feb 5, 2019

Contributor

if not target_record._filter_access_rules('read') and...

Show resolved Hide resolved addons/mail/models/mail_activity.py Outdated
Show resolved Hide resolved addons/mail/models/mail_activity.py Outdated

@nim-odoo nim-odoo requested a review from Xavier-Do Feb 5, 2019

@len-odoo len-odoo force-pushed the odoo-dev:12.0-opw1933862-chck-len branch Feb 6, 2019

@robodoo robodoo removed the CI 🤖 label Feb 6, 2019

@len-odoo

This comment has been minimized.

Copy link
Contributor Author

len-odoo commented Feb 6, 2019

Note that do_in_draft seemed to imply that _filter_access_rules('read') was wrong (it checked the right with the user's company instead of the one we wanted).
Anyway previous version was completely false.

@robodoo robodoo added the CI 🤖 label Feb 6, 2019

[FIX] mail: allow to assign an activity to an user currently in anoth…
…er company

Let users A, B be in in companies C1 and C2,
such that A is currently in C1 and B in C2.
User A tries to assign to B an activity on record R belonging to C1.
It fails because at the time of the check B is not in the correct company,
even if B could handle the activity after switching company.

The _check_access_assignation was added in commit 96a223a,
to avoid activities to be created for records that the target user can't read.
Previous use-case is a legitimate one where we want to bypass this restriction.
Note that the systray notification widget is not company dependent,
and there is no way to remove a notification on a record the user can't read.*

In case where the record is company dependent and on a different company,
we skip the record rules check.
It is assumed that the check would fail because or company-related record rules.
It will create some unwanted activities, but only the minimal number we can let
through while allowing for all legitimate flows.

*Better solution for master is to give the user some more explicit message
if reading the record, suggesting to switch company,
or to allow to dismiss the notification altogether.

opw 1933862

@len-odoo len-odoo force-pushed the odoo-dev:12.0-opw1933862-chck-len branch to 190f89b Feb 22, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 22, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

probably the least worst solution

@len-odoo

This comment has been minimized.

Copy link
Contributor Author

len-odoo commented Feb 26, 2019

robodoo r+

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 26, 2019

Merged, thanks!

@len-odoo len-odoo deleted the odoo-dev:12.0-opw1933862-chck-len branch Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.