[FIX] pos_pricelist: FP taxes with multi-company. Fixes #84

[pedrobaeza]

The object account.fiscal.position.tax doesn't have multi-company rules,
which leads to error in environments with multi-company and these records,
because it finally accesses to the parent fiscal position, which on contrary
has access rules, provoking an access error.

This commit added the field company_id to the model, and add the corresponding
access rule.

[pedrobaeza]

Please review @AdilHoumadi @legalsylvain

[legalsylvain]

is related field stored by default ? if not, security rule will not be applied on computed field ? Or not ?
(sorry if my question is not relevant).

[pedrobaeza]

It's not stored by default, and I have made it on purpose, because the direct access to this field is very strange, so I didn't want to put it as another regular field to be stored. But I can put it if you think so. I'm not sure if when accesing through the one2many field, the security rule is also applied.

Security rules are applied without problem anyway.

[legalsylvain]

Thanks for your clarification.

Security rules are applied without problem anyway.

It's the most important ! I didn't know that security rules could be applied on non stored computed field. Thanks.

[legalsylvain]

👍
Thanks for your fix.

regards.

[pedrobaeza]

Anyone more can review this?

[StefanRijnhart]

I have to say, this solution feels very 'fundamental'. Can't you just put in a domain in https://github.com/pedrobaeza/pos/blob/8.0-pos_pricelist-fp_tax_multi_company/pos_pricelist/static/src/js/models.js#L670?

Unless you really think there is a need for the regular company security measures on this model, but then this is not the right place.

[legalsylvain]

Hi @StefanRijnhart,
I don't know if you read the related Bug report #84.
Agree that it's not the good place, but it's the better place. (or we could image a simple "pos_multicompany" that manage correctly multi company for Point Of Sale. because there other similar problem on other models, or "account_multicompany").

I agree with that is a "fundamental" solution, but I'm not in favour to write domain in JS file.
-> It's not the good place, (security rules should not be in client side);
-> above all it's not very easy to overwrite. 'ir.rule' is done for that purpose;

Regards.

[pedrobaeza]

As stated by @legalsylvain, this is the less worst solution we found, because the domain at client level is indeed not the best option (and I don't know if I have access to the company id at that level). Putting this in a general module was also discarded because no other module uses directly this object till now, and it requires more work to be done.

[legalsylvain]

@StefanRijnhart. Are you ok with what we ( @pedrobaeza and I) said ?
Otherwise, which solution do you find more elegant ?

kind regards.

[StefanRijnhart]

Yes sorry, I'm still missing 50% email notifications from Github. If you agree on this, it is fine. Technically the fix looks good to me. 👍

[legalsylvain]

Thanks for your review.
Thanks @pedrobaeza.