[FIX] core, website_slides: incrementing public views

There was two issues regarding the slides public views counter

1. If the `public_views` is set to `NULL` in database,
`increment_fields_skiplock` wasn't properly incrementing the count.
Indeed, in SQL, doing NULL + 1 returns NULL
```sql
16.0=# SELECT NULL + 1;
 ?column?
----------

(1 row)
```
To have the result we expect, COALESCE must be used
```sql
16.0=# SELECT COALESCE(NULL, 0) + 1;
 ?column?
----------
        1
(1 row)
```

2. There is a mechanism, using the session,
supposed to prevent incrementing the public views
counter when a same user visits multiple times the same slide.
However, since 84d17e57e8
the visited slide was never actually added in the session,
because it was adding the slide id in a copy of the set
in session rather than adding in the set from the session.
Or, as this commit does, to re-assign the new set in the session.

closes odoo/odoo#119370

X-original-commit: fa5962d6f08979017842985004b3ec43416ee181
Signed-off-by: Denis Ledoux (dle) <dle@odoo.com>

addons/website_slides/controllers/main.py

@@ -66,12 +66,20 @@ class WebsiteSlides(WebsiteProfile):
 
     def _set_viewed_slide(self, slide, quiz_attempts_inc=False):
         if not slide.channel_id.is_member:
-            # set(...) ensures backward compatibility for sessions created earlier using a list.
-            # TODO: remove for v16.1.
-            viewed_slides = set(request.session.setdefault('viewed_slides', set()))
+            if not isinstance(request.session.get('viewed_slides'), dict):
+                # Compatibility layer with Odoo 15.0,
+                # where `viewed_slides` are stored as `list` in sessions.
+                # For performance concerns, `viewed_slides` is changed to a dict,
+                # but sessions coming from Odoo 15.0 after an upgrade should still be compatible.
+                # This compatibility layer regarding `viewed_slides` must remain from Odoo 16.0 and above,
+                # as this is possible to do a jump of multiple versions in one go,
+                # and carry the sessions with the upgrade.
+                # e.g. upgrade from Odoo 15.0 to 18.0.
+                request.session.viewed_slides = dict.fromkeys(request.session.get('viewed_slides', []), 1)
+            viewed_slides = request.session['viewed_slides']
             if slide.id not in viewed_slides:
                 if tools.sql.increment_fields_skiplock(slide, 'public_views', 'total_views'):
-                    viewed_slides.add(slide.id)
+                    viewed_slides[slide.id] = 1
                     request.session.touch()
         else:
             slide.action_set_viewed(quiz_attempts_inc=quiz_attempts_inc)

addons/website_slides/tests/test_statistics.py

@@ -8,7 +8,7 @@ from dateutil.relativedelta import relativedelta
 from odoo import fields
 from odoo.addons.website_slides.tests import common
 from odoo.exceptions import UserError
-from odoo.tests import tagged
+from odoo.tests import HttpCase, tagged
 from odoo.tests.common import users
 from odoo.tools import mute_logger, float_compare
 
@@ -178,3 +178,23 @@ class TestSlideStatistics(common.SlidesCase):
         self.assertEqual(category.total_slides, 1, 'The first category should contain 1 slide')
         self.assertEqual(other_category.total_slides, 1, 'The other category should contain 1 slide')
         self.assertEqual(self.channel.total_slides, 3, 'The channel should still contain 3 slides')
+
+@tagged('functional')
+class TestHttpSlideStatistics(HttpCase, common.SlidesCase):
+    @classmethod
+    def setUpClass(cls):
+        super(TestHttpSlideStatistics, cls).setUpClass()
+        cls.slide.is_preview = True
+
+    def test_slide_statistics_views(self):
+        self.assertEqual(self.slide.public_views, 0)
+        self.assertEqual(self.slide.total_views, 0)
+        # Open the slide a first time. Must increase the views by 1
+        self.url_open(f'/slides/slide/{self.slide.id}')
+        self.assertEqual(self.slide.public_views, 1)
+        self.assertEqual(self.slide.total_views, 1)
+        # Open the slide a second time.
+        # As it's the same session, it must not increase the views anymore
+        self.url_open(f'/slides/slide/{self.slide.id}')
+        self.assertEqual(self.slide.public_views, 1)
+        self.assertEqual(self.slide.total_views, 1)

odoo/addons/test_performance/models/models.py

@@ -75,6 +75,7 @@ class Mozzarella(models.Model):
 
     value = fields.Integer(default=0, required=True)
     value_plus_one = fields.Integer(compute="_value_plus_one", required=True, store=True)
+    value_null_by_default = fields.Integer()
 
     @api.depends('value')
     def _value_plus_one(self):

odoo/addons/test_performance/tests/test_performance.py

@@ -758,3 +758,20 @@ class TestIncrementFieldsSkipLock(TransactionCase):
 
         self.assertEqual(self.other_record.value, 10, "other_record should not have been updated.")
         self.assertEqual(self.other_record.value_plus_one, 11, "other_record should not have been updated.")
+
+    def test_increment_fields_skiplock_null_field(self):
+        """Test that incrementing a field with a NULL value in database works.
+        When an integer is NULL in database, the ORM automatically converts it to 0.
+        However, increment_fields_skiplock is a special tool using raw sql and by-passing the ORM"""
+        # First, ensure our value is NULL in database
+        self.env.cr.execute("SELECT value_null_by_default FROM test_performance_mozzarella WHERE id = %s", (self.record.id,))
+        [value] = self.env.cr.fetchone()
+        self.assertIsNone(value)
+        self.assertEqual(self.record.value_null_by_default, 0)
+        # Then, increment its count.
+        with self.assertQueryCount(1):
+            sql.increment_fields_skiplock(self.record, 'value_null_by_default')
+        # Invalidate the cache regarding the value of `value_null_by_default` for our record to force fetching from database
+        # as `increment_fields_skiplock` only does raw SQL and doesn't assign the new value in the cache
+        self.record.invalidate_recordset(['value_null_by_default'])
+        self.assertEqual(self.record.value_null_by_default, 1)

odoo/tools/sql.py

@@ -382,7 +382,7 @@ def increment_fields_skiplock(records, *fields):
     """).format(
         table=Identifier(records._table),
         sets=SQL(', ').join(map(
-            SQL('{0} = {0} + 1').format,
+            SQL('{0} = COALESCE({0}, 0) + 1').format,
             map(Identifier, fields)
         ))
     )