[FW][FIX] knowledge: correctly handle writer check when removing memberships

This commit fixes the check that verifies that the article always has a member
with write access on the article.

Indeed, the current code was using a database request to check for members with
write access in a "api.constrains" check, where the data being updated is not
flushed into database yet.

It now correctly uses fields access directly, as stated by the method docstring.
For convenience, we extracted the "_has_writer_member" method to be re-usable.

An additional test case has been added to enforce the behavior.

Task-2859616

X-original-commit: odoo/odoo@ccdfb791f8
Part-of: odoo/odoo#95471

addons/knowledge/models/knowledge_article.py

@@ -140,17 +140,7 @@ class Article(models.Model):
         Ǹote: computation is done in Py instead of using optimized SQL queries
         because value are not yet in DB at this point."""
         for article in self:
-            def has_write_member(a, child_members=False):
-                if not child_members:
-                    child_members = self.env['knowledge.article.member']
-                article_members = a.article_member_ids
-                if any(m.permission == 'write' and m.partner_id not in child_members.mapped('partner_id')
-                       for m in article_members):
-                    return True
-                if a.parent_id and not a.is_desynchronized:
-                    return has_write_member(a.parent_id, article_members | child_members)
-                return False
-            if article.inherited_permission != 'write' and not has_write_member(article):
+            if article.inherited_permission != 'write' and not article._has_write_member():
                 raise ValidationError(_("The article '%s' needs at least one member with 'Write' access.", article.display_name))
 
     @api.constrains('parent_id')
@@ -1230,19 +1220,55 @@ class Article(models.Model):
 
             writable_self = self.sudo()
             self_member = writable_self.article_member_ids.filtered(lambda m: m.partner_id == self.env.user.partner_id)
-            member_command = [(2, member.id) for member in writable_self.article_member_ids if member.partner_id != self.env.user.partner_id]
 
+            # first add or update the new membership with 'write' access
             if self_member:
-                member_command.append((1, self_member.id, {'permission': 'write'}))
+                write_member_command = [(1, self_member.id, {'permission': 'write'})]
             else:
-                member_command.append((0, 0, {
+                write_member_command = [(0, 0, {
                     'partner_id': self.env.user.partner_id.id,
                     'permission': 'write'
-                }))
-            article_values['article_member_ids'] = member_command
+                })]
+            # then remove other members as the article is moved to private
+            article_values['article_member_ids'] = [
+                (2, member.id)
+                for member in writable_self.article_member_ids if member.partner_id != self.env.user.partner_id
+            ] + write_member_command
 
         return writable_self.with_context(knowledge_member_skip_writable_check=True).write(article_values)
 
+    def _has_write_member(self, partners_to_exclude=False, members_to_exclude=False):
+        """ Method allowing to check if this article still has at least one member
+        with write access. Typically used during constraints checks.
+
+        Please note that this method is *not* optimized and should be avoided by
+        using ``_get_article_member_permissions`` instead when possible.
+
+        :param <res.partner> partners_to_exclude: used when checking recursively
+          through article parents, we only check for the most specific access
+          for a given partner;
+        :param <knowledge.article.member> members_to_exclude: memberships that
+          should not be considered when checking for a write access, used when
+          unlinking members that should not be taken into account;
+
+        :return boolean: whether a write member has been found;
+        """
+        self.ensure_one()
+        partners_to_exclude = partners_to_exclude if partners_to_exclude else self.env['res.partner']
+
+        article_members = self.article_member_ids
+        if members_to_exclude:
+            article_members -= members_to_exclude
+
+        if any(m.permission == 'write' and m.partner_id not in partners_to_exclude
+               for m in article_members):
+            return True
+        if not self.is_desynchronized and self.parent_id:
+            return self.parent_id._has_write_member(
+                partners_to_exclude=article_members.partner_id | partners_to_exclude
+            )
+        return False
+
     # ------------------------------------------------------------
     # PERMISSIONS BATCH COMPUTATION
     # ------------------------------------------------------------

addons/knowledge/models/knowledge_article_member.py

@@ -61,7 +61,6 @@ class ArticleMember(models.Model):
             for member in self.filtered(lambda member: member.article_id in articles_to_check):
                 deleted_members_by_article[member.article_id.id] |= member
 
-        parents_members_permission = articles_to_check.parent_id._get_article_member_permissions()
         for article in articles_to_check:
             # Check on permission on members
             members_to_check = article.article_member_ids
@@ -70,15 +69,8 @@ class ArticleMember(models.Model):
             if any(m.permission == 'write' for m in members_to_check):
                 continue
 
-            # we need to add the members on parents to check the validity
-            parent_write_members = any(
-                values['permission'] == 'write' for partner_id, values
-                in parents_members_permission[article.parent_id.id].items()
-                if article.parent_id and not article.is_desynchronized
-                and partner_id not in article.article_member_ids.partner_id.ids
-            )
-
-            if not parent_write_members:
+            members_to_exclude = deleted_members_by_article[article.id] if on_unlink else False
+            if not article._has_write_member(members_to_exclude=members_to_exclude):
                 raise ValidationError(
                     _("Article '%s' should always have a writer: inherit write permission, or have a member with write access",
                       article.display_name)

addons/knowledge/tests/test_knowledge_article_constraints.py

@@ -319,6 +319,13 @@ class TestKnowledgeArticleConstraints(KnowledgeCommon):
         with self.assertRaises(exceptions.ValidationError, msg='Cannot remove the last writer on an article'):
             article_private._add_members(membership_sudo.partner_id, 'none')
 
+        # moving the article to private will remove the second member
+        # but should not trigger an error since we also add 'employee' as a write member
+        article_workspace = self.article_workspace.with_env(self.env)
+        article_workspace.move_to(is_private=True)
+        self.assertEqual(article_workspace.category, 'private')
+        self.assertTrue(article_workspace._has_write_member())
+
     @mute_logger('odoo.sql_db')
     @users('employee')
     def test_favourite_uniqueness(self):

addons/knowledge/tests/test_knowledge_performance.py

@@ -81,7 +81,7 @@ class KnowledgePerformanceCase(KnowledgeCommonWData):
     @users('employee')
     @warmup
     def test_article_invite_members(self):
-        with self.assertQueryCount(employee=89):  # knowledge only: 89
+        with self.assertQueryCount(employee=84):  # knowledge only: 84
             shared_article = self.shared_children[0].with_env(self.env)
             partners = (self.customer + self.partner_employee_manager + self.partner_employee2).with_env(self.env)
             shared_article.invite_members(partners, 'write')