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

addons/website_sale/models/website.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.

addons/website_sale/tests/test_website_sale_pricelist.py

@@ -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