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] website_sale: limit pricelist to it's website if set #28301

Closed
wants to merge 6 commits into
base: 12.0
from

Conversation

Projects
None yet
5 participants
@rdeodoo
Copy link
Contributor

rdeodoo commented Oct 30, 2018

Before this commit:

  1. Non Public User would get the main company pricelist returned when calling
    get_current_pricelist. As this is saved in an ir.property
    (property_product_pricelist) that is not website dependant, that pricelist
    would always be returned on every website.
    This is a problem if that pricelist is website limited, you should not be
    able to use it on other websites.
  2. Public User would not have a valid pricelist on other website than the
    default one. Indeed, every pricelists are restricted that default website.
    It would result in prices shown as "0" (without currency symbol) on the shop
  3. Pricelists set as selectable without a website_id set would not be
    selectable

This commit:

  1. We override and check that pricelists returnes by the ir.property are
    indeed available on the current website.
  2. Remove the website restriction on Public Pricelist so other websites have
    at least one pricelist available.
  3. pricelist_ids on website now return all available pricelists including
    the ones that are generic (no website set)

Part of this commit fixes part of #27861
Github-25109 (Removed from this PR and will be merged in master)
#30743
#30284
#29553

task-1896113
task-1919358
opw-1911789
opw-1935785

@robodoo robodoo added the seen 🙂 label Oct 30, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch Oct 30, 2018

@C3POdoo C3POdoo added the RD label Oct 30, 2018

@robodoo robodoo added the CI 🤖 label Oct 30, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch Oct 31, 2018

@robodoo robodoo removed the CI 🤖 label Oct 31, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch Oct 31, 2018

@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

rdeodoo commented Oct 31, 2018

Before merge rewrite link to PR (missing hashtag to avoid noise)

@robodoo robodoo added the CI 🤖 label Oct 31, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch Nov 22, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 22, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch Nov 22, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 22, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch Nov 26, 2018

@robodoo robodoo removed the CI 🤖 label Nov 26, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch 2 times, most recently Nov 26, 2018

@rdeodoo rdeodoo changed the title 12.0 fix pricelist mw rde [FIX] website_sale: limit pricelist to it's website if set Nov 26, 2018

@rdeodoo rdeodoo requested a review from JKE-be Nov 26, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 26, 2018

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch 2 times, most recently Nov 27, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 27, 2018

@robodoo robodoo removed the CI 🤖 label Dec 20, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 13, 2019

rdeodoo added some commits Feb 12, 2019

[FIX] website, website_sale: create utility method to check if frontend
Before this commit, at multiple places to check if we were in a frontend
context we would check if `request and hasattr(request, 'website')` and
then access `request.website`.

Now, we call an utility method to get that website if we are in a frontend
context. If not the method will return False.

This method will be usefull for incoming pricelist fixes & tests.
Indeed, we will have more check of that kind and this will be easier to mock as
mocking this method will be enough, instead of mocking every method doing that
if condition to simulate a frontend context (to do website tests in python
wihout having to go frontend with a tour).
[FIX] product, website_sale: clean dead code
The `_get_pl()` method introduced with bcb3956 was replaced by
`_get_pl_partner_order()` in afe1d20.
After that, it looks like this method was never called again.

Plus, add a missing ensure_one() (not critical).
[FIX] website_sale: limit pricelist to it's website if set
Before this commit:
1. Non Public User would get the main company pricelist returned when calling
   `get_current_pricelist`. As this is saved in an `ir.property`
   (`property_product_pricelist`) that is not website dependant, that pricelist
   would always be returned on every website.
   This is a problem if that pricelist is website limited, you should not be
   able to use it on other websites.
2. Public User would not have a valid pricelist on other website than the
   default one. Indeed, every pricelists are restricted that default website.
   It would result in prices shown as "0" (without currency symbol) on the shop
3. Pricelists set as selectable without a website_id set would not be
   selectable
4. Public User would not get a fallback pricelist when reading its
   `property_product_pricelist`. Indeed, the public user's partner is not
   active and since the refactoring of this field compute method to be a multi,
   it would end up doing a search() instead of a browse(). Thus, it would find
   a pricelist for the public user but it would not set it.

This commit:
1. We override and check that pricelists returnes by the `ir.property` are
   indeed available on the current website.
2. Remove the website restriction on `Public Pricelist` so other websites have
   at least one pricelist available.
3. `pricelist_ids` on `website` now return all available pricelists including
   the ones that are generic (no website set)
4. We search for inactive partners too.

Part of this commit fixes part of 27861
Github-25109
[FIX] website(_sale): allow to use and access website pricelist
Before this commit, if the website's company was different than the user
company, the ecommerce would use the user's company instead of its own
pricelists.

Step to reproduce:
  - Set company2 to website1
  - Create a pricelist1 for company1 and a pricelist2 for company2
  - With an user from company1, visit the website1.
  - The pricelist used will be the one from the user, pricelist1 and not the
    pricelist2 from company2 which is the website1 company.
[FIX] website_sale: prevent compute search to be run in sudo
Before this commit, when accessing an user's pricelist, it would raise a 500
error in multi-company environment.
Indeed, when doing `self.env.user.partner_id.property_product_pricelist`, even
if self.env is not un sudo mode, the compute from `property_product_pricelist`
is executed as sudo. Since this method is then doing a `search()`, it won't use
the multi-company pricelist ir.rule. It will return all the pricelist,
including the ones from other companies.
Then, when trying to read that pricelist, it will raise an access error.

Now, we correctly execute the compute (and so the search) in the user env, it
won't get pricelists from other companies.
[FIX] website_sale: fix _get_pl_partner_order
1. Encapsulate `show_visible` to avoid duplicate same code in filtered and to
   make it more easy to read.
2. When GEOIP is enabled, `_get_pl_partner_order` would retrive the available
   pricelists for that country group.
   We should also ensure the returned pricelists are also available on that
   website
3. Rewrite the `country_code` condition in one line thanks to 1.
4. !! Do not use `property_product_pricelist` in the method as it is not cached
   and now it depends website. Anyway it was useless as it was a duplicate of
   `partner_pl`.
5. Add `is_public` to the if condition as it was actually implicit as if not
   public it and no pricelists it would go in previous condition and set the
   pricelist from `property_product_pricelist`.
6. The last if condition (`not country_code`) has always been subject to fixes
   and always broke a flow to fix another one (see 60300fc and e11908d, both
   were wrong).
   This condition refactoring should still preserves the beavior fixes
   mentionned in both commits and fix the flow where a logged in user would not
   get selectable pricelists if he has a partner_pl (property_product_pricelist)
   before entering the all_pl filtered.

@rdeodoo rdeodoo force-pushed the odoo-dev:12.0-fix-pricelist-mw-rde branch to b408e17 Mar 14, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 14, 2019

@JKE-be

This comment has been minimized.

Copy link
Contributor

JKE-be commented Mar 18, 2019

@robodoo r+ rebase-ff

@robodoo robodoo added the r+ 👌 label Mar 18, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 18, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo closed this in f8efc9a Mar 18, 2019

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Mar 18, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 18, 2019

Merged, thanks!

@nim-odoo nim-odoo deleted the odoo-dev:12.0-fix-pricelist-mw-rde branch Mar 18, 2019

rdeodoo referenced this pull request Mar 18, 2019

[FIX] website, website_sale: create utility method to check if frontend
Before this commit, at multiple places to check if we were in a frontend
context we would check if `request and hasattr(request, 'website')` and
then access `request.website`.

Now, we call an utility method to get that website if we are in a frontend
context. If not the method will return False.

This method will be usefull for incoming pricelist fixes & tests.
Indeed, we will have more check of that kind and this will be easier to mock as
mocking this method will be enough, instead of mocking every method doing that
if condition to simulate a frontend context (to do website tests in python
wihout having to go frontend with a tour).

rdeodoo referenced this pull request Mar 18, 2019

[FIX] product, website_sale: clean dead code
The `_get_pl()` method introduced with bcb3956 was replaced by
`_get_pl_partner_order()` in afe1d20.
After that, it looks like this method was never called again.

Plus, add a missing ensure_one() (not critical).

rdeodoo referenced this pull request Mar 18, 2019

[FIX] website_sale: limit pricelist to it's website if set
Before this commit:
1. Non Public User would get the main company pricelist returned when calling
   `get_current_pricelist`. As this is saved in an `ir.property`
   (`property_product_pricelist`) that is not website dependant, that pricelist
   would always be returned on every website.
   This is a problem if that pricelist is website limited, you should not be
   able to use it on other websites.
2. Public User would not have a valid pricelist on other website than the
   default one. Indeed, every pricelists are restricted that default website.
   It would result in prices shown as "0" (without currency symbol) on the shop
3. Pricelists set as selectable without a website_id set would not be
   selectable
4. Public User would not get a fallback pricelist when reading its
   `property_product_pricelist`. Indeed, the public user's partner is not
   active and since the refactoring of this field compute method to be a multi,
   it would end up doing a search() instead of a browse(). Thus, it would find
   a pricelist for the public user but it would not set it.

This commit:
1. We override and check that pricelists returnes by the `ir.property` are
   indeed available on the current website.
2. Remove the website restriction on `Public Pricelist` so other websites have
   at least one pricelist available.
3. `pricelist_ids` on `website` now return all available pricelists including
   the ones that are generic (no website set)
4. We search for inactive partners too.

Part of this commit fixes part of 27861
Github-25109

rdeodoo referenced this pull request Mar 18, 2019

[FIX] website(_sale): allow to use and access website pricelist
Before this commit, if the website's company was different than the user
company, the ecommerce would use the user's company instead of its own
pricelists.

Step to reproduce:
  - Set company2 to website1
  - Create a pricelist1 for company1 and a pricelist2 for company2
  - With an user from company1, visit the website1.
  - The pricelist used will be the one from the user, pricelist1 and not the
    pricelist2 from company2 which is the website1 company.

rdeodoo referenced this pull request Mar 18, 2019

[FIX] website_sale: prevent compute search to be run in sudo
Before this commit, when accessing an user's pricelist, it would raise a 500
error in multi-company environment.
Indeed, when doing `self.env.user.partner_id.property_product_pricelist`, even
if self.env is not un sudo mode, the compute from `property_product_pricelist`
is executed as sudo. Since this method is then doing a `search()`, it won't use
the multi-company pricelist ir.rule. It will return all the pricelist,
including the ones from other companies.
Then, when trying to read that pricelist, it will raise an access error.

Now, we correctly execute the compute (and so the search) in the user env, it
won't get pricelists from other companies.
@rdeodoo

This comment has been minimized.

Copy link
Contributor Author

rdeodoo commented Mar 18, 2019

@JKE-be @nim-odoo Thanks a million times for the review(s).

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.