Skip to content
Permalink
Browse files

[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.

closes #28301

Signed-off-by: Jérémy Kersten (jke) <jke@openerp.com>
  • Loading branch information...
rdeodoo committed Mar 12, 2019
1 parent 9608d45 commit f8efc9a957c28a428bb8ed994815e856b1f5e805
Showing with 223 additions and 17 deletions.
  1. +35 −17 addons/website_sale/models/website.py
  2. +188 −0 addons/website_sale/tests/test_website_sale_pricelist.py
@@ -60,35 +60,50 @@ def _get_pl_partner_order(self, country_code, show_visible, website_pl, current_
:param int order_pl: the current cart pricelist
:returns: list of pricelist ids
"""
def _check_show_visible(pl):
""" If `show_visible` is True, we will only show the pricelist if
one of this condition is met:
- The pricelist is `selectable`.
- The pricelist is either the currently used pricelist or the
current cart pricelist, we should consider it as available even if
it might not be website compliant (eg: it is not selectable anymore,
it is a backend pricelist, it is not active anymore..).
"""
return (not show_visible or pl.selectable or pl.id in (current_pl, order_pl))

# Note: 1. pricelists from all_pl are already website compliant (went through
# `_get_website_pricelists_domain`)
# 2. do not read `property_product_pricelist` here as `_get_pl_partner_order`
# is cached and the result of this method will be impacted by that field value.
# Pass it through `partner_pl` parameter instead to invalidate the cache.

# If there is a GeoIP country, find a pricelist for it
self.ensure_one()
pricelists = self.env['product.pricelist']
if country_code:
for cgroup in self.env['res.country.group'].search([('country_ids.code', '=', country_code)]):
for group_pricelists in cgroup.pricelist_ids:
if not show_visible or group_pricelists.selectable or group_pricelists.id in (current_pl, order_pl):
pricelists |= group_pricelists
pricelists |= cgroup.pricelist_ids.filtered(
lambda pl: pl._is_available_on_website(self.id) and _check_show_visible(pl)
)

partner = self.env.user.partner_id.sudo(user=self.env.user)
is_public = self.user_id.id == self.env.user.id
if not is_public and (not pricelists or (partner_pl or partner.property_product_pricelist.id) != website_pl):
# `property_product_pricelist` is already multi-website compliant
pricelists |= partner.property_product_pricelist
# no GeoIP or no pricelist for this country
if not country_code or not pricelists:
pricelists |= all_pl.filtered(lambda pl: _check_show_visible(pl))

if not pricelists: # no pricelist for this country, or no GeoIP
pricelists |= all_pl.filtered(lambda pl: not show_visible or pl.selectable or pl.id in (current_pl, order_pl))
if not show_visible and not country_code:
pricelists |= all_pl.filtered(lambda pl: pl.sudo().code)
# if logged in, add partner pl (which is `property_product_pricelist`, might not be website compliant)
is_public = self.user_id.id == self.env.user.id
if not is_public:
pricelists |= pricelists.browse(partner_pl).filtered(lambda pl: pl._is_available_on_website(self.id) and _check_show_visible(pl))

# This method is cached, must not return records! See also #8795
return pricelists.filtered(lambda pl: pl._is_available_on_website(self.id)).ids
return pricelists.ids

# DEPRECATED (Not used anymore) -> Remove me in master (saas12.3)
def _get_pl(self, country_code, show_visible, website_pl, current_pl, all_pl):
pl_ids = self._get_pl_partner_order(country_code, show_visible, website_pl, current_pl, all_pl)
return self.env['product.pricelist'].browse(pl_ids)

def get_pricelist_available(self, show_visible=False):

def _get_pricelist_available(self, req, show_visible=False):
""" Return the list of pricelists that can be used on website for the current user.
Country restrictions will be detected with GeoIP (if installed).
:param bool show_visible: if True, we don't display pricelist where selectable is False (Eg: Code promo)
@@ -101,18 +116,21 @@ def get_pricelist_available(self, show_visible=False):
else:
# In the weird case we are coming from the backend (https://github.com/odoo/odoo/issues/20245)
website = len(self) == 1 and self or self.search([], limit=1)
isocountry = request and request.session.geoip and request.session.geoip.get('country_code') or False
isocountry = req and req.session.geoip and req.session.geoip.get('country_code') or False
partner = self.env.user.partner_id
last_order_pl = partner.last_website_so_id.pricelist_id
partner_pl = partner.sudo(user=self.env.user).property_product_pricelist
pricelists = website._get_pl_partner_order(isocountry, show_visible,
website.user_id.sudo().partner_id.property_product_pricelist.id,
request and request.session.get('website_sale_current_pl') or None,
req and req.session.get('website_sale_current_pl') or None,
website.pricelist_ids,
partner_pl=partner_pl and partner_pl.id or None,
order_pl=last_order_pl and last_order_pl.id or None)
return self.env['product.pricelist'].browse(pricelists)

def get_pricelist_available(self, show_visible=False):
return self._get_pricelist_available(request, show_visible)

def is_pricelist_available(self, pl_id):
""" Return a boolean to specify if a specific pricelist can be manually set on the website.
Warning: It check only if pricelist is in the 'selectable' pricelists or the current pricelist.
@@ -6,6 +6,24 @@
from mock import patch
from odoo.tests.common import HttpCase, TransactionCase

''' /!\/!\
Calling `get_pricelist_available` after setting `property_product_pricelist` on
a partner will not work as expected. That field will change the output of
`get_pricelist_available` but modifying it will not invalidate the cache.
Thus, tests should not do:
self.env.user.partner_id.property_product_pricelist = my_pricelist
pls = self.get_pricelist_available()
self.assertEqual(...)
self.env.user.partner_id.property_product_pricelist = another_pricelist
pls = self.get_pricelist_available()
self.assertEqual(...)
as `_get_pl_partner_order` cache won't be invalidate between the calls, output
won't be the one expected and tests will actually not test anything.
Try to keep one call to `get_pricelist_available` by test method.
'''


class TestWebsitePriceList(TransactionCase):

@@ -136,6 +154,176 @@ def get_request_website():
self.addCleanup(patcher.stop)


class DotDict(dict):
"""dot.notation access to dictionary attributes"""
def __getattr__(*args):
val = dict.get(*args)
return DotDict(val) if type(val) is dict else val


class TestWebsitePriceListAvailable(TransactionCase):
# This is enough to avoid a mock (request.session/website do not exist during test)
def get_pricelist_available(self, show_visible=False, website_id=1, country_code=None, website_sale_current_pl=None):
request = DotDict({
'website': self.env['website'].browse(website_id),
'session': {
'geoip': {
'country_code': country_code,
},
'website_sale_current_pl': website_sale_current_pl,
},
})
return self.env['website']._get_pricelist_available(request, show_visible)

def setUp(self):
super(TestWebsitePriceListAvailable, self).setUp()
Pricelist = self.env['product.pricelist']
Website = self.env['website']

# Set up 2 websites
self.website = Website.browse(1)
self.website2 = Website.create({'name': 'Website 2'})

# Remove existing pricelists and create new ones
Pricelist.search([]).write({'active': False})
self.backend_pl = Pricelist.create({
'name': 'Backend Pricelist',
'website_id': False,
})
self.generic_pl_select = Pricelist.create({
'name': 'Generic Selectable Pricelist',
'selectable': True,
'website_id': False,
})
self.generic_pl_code = Pricelist.create({
'name': 'Generic Code Pricelist',
'code': 'GENERICCODE',
'website_id': False,
})
self.generic_pl_code_select = Pricelist.create({
'name': 'Generic Code Selectable Pricelist',
'code': 'GENERICCODESELECT',
'selectable': True,
'website_id': False,
})
self.w1_pl = Pricelist.create({
'name': 'Website 1 Pricelist',
'website_id': self.website.id,
})
self.w1_pl_select = Pricelist.create({
'name': 'Website 1 Pricelist Selectable',
'website_id': self.website.id,
'selectable': True,
})
self.w1_pl_code = Pricelist.create({
'name': 'Website 1 Pricelist Code',
'website_id': self.website.id,
'code': 'W1CODE',
})
self.w1_pl_code_select = Pricelist.create({
'name': 'Website 1 Pricelist Code Selectable',
'website_id': self.website.id,
'code': 'W1CODESELECT',
'selectable': True,
})
self.w2_pl = Pricelist.create({
'name': 'Website 2 Pricelist',
'website_id': self.website2.id,
})

simulate_frontend_context(self)

def test_get_pricelist_available(self):
# all_pl = self.backend_pl + self.generic_pl_select + self.generic_pl_code + self.generic_pl_code_select + self.w1_pl + self.w1_pl_select + self.w1_pl_code + self.w1_pl_code_select + self.w2_pl

# Test get all available pricelists
pls_to_return = self.generic_pl_select + self.generic_pl_code + self.generic_pl_code_select + self.w1_pl + self.w1_pl_select + self.w1_pl_code + self.w1_pl_code_select
pls = self.get_pricelist_available()
self.assertEqual(pls, pls_to_return, "Every pricelist having the correct website_id set or (no website_id but a code or selectable) should be returned")

# Test get all available and visible pricelists
pls_to_return = self.generic_pl_select + self.generic_pl_code_select + self.w1_pl_select + self.w1_pl_code_select
pls = self.get_pricelist_available(show_visible=True)
self.assertEqual(pls, pls_to_return, "Only selectable pricelists website compliant (website_id False or current website) should be returned")

def test_property_product_pricelist_for_inactive_partner(self):
# `_get_partner_pricelist_multi` should consider inactive users when searching for pricelists.
# Real case if for public user. His `property_product_pricelist` need to be set as it is passed
# through `_get_pl_partner_order` as the `website_pl` when searching for available pricelists
# for active users.
public_partner = self.env.ref('base.public_partner')
self.assertFalse(public_partner.active, "Ensure public partner is inactive (purpose of this test)")
pl = public_partner.property_product_pricelist
self.assertEqual(len(pl), 1, "Inactive partner should still get a `property_product_pricelist`")


class TestWebsitePriceListAvailableGeoIP(TestWebsitePriceListAvailable):
def setUp(self):
super(TestWebsitePriceListAvailableGeoIP, self).setUp()
# clean `property_product_pricelist` for partner for this test (clean setup)
self.env['ir.property'].search([('res_id', '=', 'res.partner,%s' % self.env.user.partner_id.id)]).unlink()

# set different country groups on pricelists
c_EUR = self.env.ref('base.europe')
c_BENELUX = self.env.ref('website_sale.benelux')
self.BE = self.env.ref('base.be')
NL = self.env.ref('base.nl')
c_BE = self.env['res.country.group'].create({'name': 'Belgium', 'country_ids': [(6, 0, [self.BE.id])]})
c_NL = self.env['res.country.group'].create({'name': 'Netherlands', 'country_ids': [(6, 0, [NL.id])]})

(self.backend_pl + self.generic_pl_select + self.generic_pl_code + self.w1_pl_select).write({'country_group_ids': [(6, 0, [c_BE.id])]})
(self.generic_pl_code_select + self.w1_pl + self.w2_pl).write({'country_group_ids': [(6, 0, [c_BENELUX.id])]})
(self.w1_pl_code).write({'country_group_ids': [(6, 0, [c_EUR.id])]})
(self.w1_pl_code_select).write({'country_group_ids': [(6, 0, [c_NL.id])]})

# pricelist | selectable | website | code | country group |
# ----------------------------------------------------------------------|
# backend_pl | | | | BE |
# generic_pl_select | V | | | BE |
# generic_pl_code | | | V | BE |
# generic_pl_code_select | V | | V | BENELUX |
# w1_pl | | 1 | | BENELUX |
# w1_pl_select | V | 1 | | BE |
# w1_pl_code | | 1 | V | EUR |
# w1_pl_code_select | V | 1 | V | NL |
# w2_pl | | 2 | | BENELUX |

# available pl for website 1 for GeoIP BE (anything except website 2, backend and NL)
self.website1_be_pl = self.generic_pl_select + self.generic_pl_code + self.w1_pl_select + self.generic_pl_code_select + self.w1_pl + self.w1_pl_code

def test_get_pricelist_available_geoip(self):
# Test get all available pricelists with geoip and no partner pricelist (ir.property)

# property_product_pricelist will also be returned in the available pricelists
self.website1_be_pl += self.env.user.partner_id.property_product_pricelist

pls = self.get_pricelist_available(country_code=self.BE.code)
self.assertEqual(pls, self.website1_be_pl, "Only pricelists for BE and accessible on website should be returned, and the partner pl")

def test_get_pricelist_available_geoip2(self):
# Test get all available pricelists with geoip and a partner pricelist (ir.property) not website compliant
self.env.user.partner_id.property_product_pricelist = self.backend_pl
pls = self.get_pricelist_available(country_code=self.BE.code)
self.assertEqual(pls, self.website1_be_pl, "Only pricelists for BE and accessible on website should be returned as partner pl is not website compliant")

def test_get_pricelist_available_geoip3(self):
# Test get all available pricelists with geoip and a partner pricelist (ir.property) website compliant (but not geoip compliant)
self.env.user.partner_id.property_product_pricelist = self.w1_pl_code_select
self.website1_be_pl += self.env.user.partner_id.property_product_pricelist
pls = self.get_pricelist_available(country_code=self.BE.code)
self.assertEqual(pls, self.website1_be_pl, "Only pricelists for BE and accessible on website should be returned, plus the partner pricelist as it is website compliant")

def test_get_pricelist_available_geoip4(self):
# Test get all available with geoip and visible pricelists + promo pl
pls_to_return = self.generic_pl_select + self.w1_pl_select + self.generic_pl_code_select
# property_product_pricelist will also be returned in the available pricelists
pls_to_return += self.env.user.partner_id.property_product_pricelist

current_pl = self.w1_pl_code
pls = self.get_pricelist_available(country_code=self.BE.code, show_visible=True, website_sale_current_pl=current_pl.id)
self.assertEqual(pls, pls_to_return + current_pl, "Only pricelists for BE, accessible en website and selectable should be returned. It should also return the applied promo pl")


class TestWebsitePriceListHttp(HttpCase):
def test_get_pricelist_available_multi_company(self):
''' Test that the `property_product_pricelist` of `res.partner` is not

0 comments on commit f8efc9a

Please sign in to comment.
You can’t perform that action at this time.