[FIX] Various web_advanced_filter cleanup

web_advanced_filter/README.rst

@@ -76,6 +76,7 @@ Contributors
 ~~~~~~~~~~~~
 
 * Holger Brunn <hbrunn@therp.nl>
+* Jan Verbeek <jverbeek@therp.nl>
 
 Other credits
 ~~~~~~~~~~~~~

web_advanced_filter/models/ir_filters.py

@@ -17,45 +17,51 @@ class IrFilters(models.Model):
     union_filter_ids = fields.Many2many(
         'ir.filters', 'ir_filters_union_rel', 'left_filter_id',
         'right_filter_id', 'Add result of filters',
-        domain=['|', ('active', '=', False), ('active', '=', True)],
+        context={"active_test": False},
     )
     complement_filter_ids = fields.Many2many(
         'ir.filters', 'ir_filters_complement_rel', 'left_filter_id',
         'right_filter_id', 'Remove result of filters',
-        domain=['|', ('active', '=', False), ('active', '=', True)],
+        context={"active_test": False},
     )
     domain = fields.Text(compute='_compute_domain', inverse='_inverse_domain')
-    domain_this = fields.Text('This filter\'s own domain', oldname='domain')
+    domain_this = fields.Text("Domain without helpers", oldname='domain')
     evaluate_before_negate = fields.Boolean(
         'Evaluate this filter before negating',
-        compute='_compute_flags',
+        compute='_compute_pre_evaluate',
         help='This is necessary if this filter contains positive operators'
         'on x2many fields',
     )
-    evaluate_always = fields.Boolean(
-        'Always evaluate this filter before using it',
-        compute='_compute_flags',
+    evaluate_before_join = fields.Boolean(
+        'Evaluate this filter before joining',
+        compute='_compute_pre_evaluate',
         help='This is necessary if this filter contains x2many fields with'
         '_auto_join activated',
     )
     save_as_public = fields.Boolean(
-        'Share with all users', default=False,
-        compute=lambda self: None, inverse=lambda self: None,
+        'Share with all users', default=False, store=False
     )
 
+    @api.model
+    def _eval_domain(self, domain):
+        """Parse a domain and normalize it, with a default if it's invalid."""
+        try:
+            domain = safe_eval(domain) or [expression.FALSE_LEAF]
+        except (SyntaxError, TypeError, ValueError, NameError):
+            domain = [expression.FALSE_LEAF]
+        return expression.normalize_domain(domain)
+
     @api.multi
     @api.depends('domain')
     def _compute_is_frozen(self):
         '''determine if this is fixed list of ids'''
         for this in self:
-            try:
-                domain = safe_eval(this.domain)
-            except (SyntaxError, TypeError, ValueError, NameError):
-                domain = [expression.FALSE_LEAF]
+            domain = self._eval_domain(this.domain)
             this.is_frozen = (
-                len(domain) == 1 and
-                expression.is_leaf(domain[0]) and
-                domain[0][0] == 'id'
+                len(domain) == 1
+                and expression.is_leaf(domain[0])
+                and domain[0][0] == "id"
+                and domain[0][1] == "in"
             )
 
     @api.multi
@@ -66,37 +72,53 @@ class IrFilters(models.Model):
     def _compute_domain(self):
         '''combine our domain with all domains to union/complement,
         this works recursively'''
-        def eval_n(domain):
-            '''parse a domain and normalize it'''
-            try:
-                domain = safe_eval(domain)
-            except (SyntaxError, TypeError, ValueError, NameError):
-                domain = [expression.FALSE_LEAF]
-            return expression.normalize_domain(
-                domain or [expression.FALSE_LEAF])
 
         for this in self:
             if this.model_id not in self.env:
                 this.domain = '[]'
-                _logger.error('Unknown model %s used in filter', this.model_id)
+                _logger.error(
+                    "Unknown model %s used in filter %d",
+                    this.model_id,
+                    this.id,
+                )
                 continue
-            domain = eval_n(this.domain_this)
+            domain = self._eval_domain(this.domain_this)
             for u in this.union_filter_ids:
-                if u.evaluate_always:
-                    matching_ids = self.env[u.model_id].search(
-                        eval_n(u.domain),
-                    ).ids
+                if u.model_id != this.model_id:
+                    _logger.warning(
+                        "Model mismatch in helper %d on filter %d!",
+                        u.model_id,
+                        this.model_id,
+                    )
+                    continue
+                if u.evaluate_before_join:
+                    matching_ids = (
+                        self.env[this.model_id]
+                        .search(self._eval_domain(u.domain))
+                        .ids
+                    )
                     domain = expression.OR([
                         domain,
                         [('id', 'in', matching_ids)],
                     ])
                 else:
-                    domain = expression.OR([domain, eval_n(u.domain)])
+                    domain = expression.OR(
+                        [domain, self._eval_domain(u.domain)]
+                    )
             for c in this.complement_filter_ids:
+                if c.model_id != this.model_id:
+                    _logger.warning(
+                        "Model mismatch in helper %d on filter %d!",
+                        c.model_id,
+                        this.model_id,
+                    )
+                    continue
                 if c.evaluate_before_negate:
-                    matching_ids = self.env[c.model_id].search(
-                        eval_n(c.domain),
-                    ).ids
+                    matching_ids = (
+                        self.env[this.model_id]
+                        .search(self._eval_domain(c.domain))
+                        .ids
+                    )
                     domain = expression.AND([
                         domain,
                         [('id', 'not in', matching_ids)],
@@ -104,9 +126,9 @@ class IrFilters(models.Model):
                 else:
                     domain = expression.AND([
                         domain,
-                        ['!'] + eval_n(c['domain']),
+                        ["!"] + self._eval_domain(c["domain"])
                     ])
-            this.domain = str(expression.normalize_domain(domain))
+            this.domain = repr(expression.normalize_domain(domain))
 
     @api.multi
     def _inverse_domain(self):
@@ -114,43 +136,69 @@ class IrFilters(models.Model):
             this.domain_this = this.domain
 
     @api.multi
-    @api.depends('domain')
-    def _compute_flags(self):
-        """check if this filter contains references to x2many fields. If so,
-        then negation goes wrong in nearly all cases, so we evaluate the
-        filter and remove its resulting ids"""
+    @api.depends('domain', 'model_id')
+    def _compute_pre_evaluate(self):
+        """Check if this filter contains problematic fields.
+
+        Domains with certain fields can't be negated or joined properly.
+        They have to be evaluated in advance. In particular:
+
+        - Negating a query on a 2many field doesn't invert the matched records.
+          ``foo.bar != 3`` will yield all records with a ``foo.bar`` that
+          isn't ``3``, not all records without a ``foo.bar`` that is ``3``.
+          So just putting a ``!`` in front of the domain doesn't do what we
+          want.
+
+        - Querying an autojoined field constrains the entire search, even if
+          it's joined with ``|``. If ``('user_ids.login', '=', 'admin')`` is
+          used anywhere in the domain then only records that satisfy that leaf
+          are found.
+
+        - Fields with custom search logic (``search=...``) might do one of the
+          above, or some other strange thing.
+
+        Examples can be found in the tests for this module.
+        """
         for this in self:
-            this.evaluate_before_negate = False
-            this.evaluate_always = False
-            domain = expression.normalize_domain(
-                safe_eval(this.domain or 'False') or [expression.FALSE_LEAF]
-            )
+            evaluate_before_negate = False
+            evaluate_before_join = False
+            domain = self._eval_domain(this.domain)
             for arg in domain:
+                if not expression.is_leaf(arg) or not isinstance(arg[0], str):
+                    continue
                 current_model = self.env.get(this.model_id)
                 if current_model is None:
                     _logger.error(
-                        'Unknown model %s used in filter', this.model_id)
+                        "Unknown model %s used in filter %d",
+                        this.model_id,
+                        this.id,
+                    )
                     continue
-                if not expression.is_leaf(arg) or not isinstance(arg[0], str):
-                    continue
-                has_x2many = False
-                has_auto_join = False
                 for field_name in arg[0].split('.'):
                     if field_name in models.MAGIC_COLUMNS:
                         continue
                     field = current_model._fields[field_name]
-                    has_x2many |= field.type in self._evaluate_before_negate
-                    has_x2many |= bool(field.compute)
-                    has_auto_join |= getattr(field, 'auto_join', False)
-                    has_auto_join |= bool(field.compute)
+                    evaluate_before_negate = (
+                        evaluate_before_negate
+                        or field.search
+                        or field.type in self._evaluate_before_negate
+                        or getattr(field, "auto_join", False)
+                    )
+                    evaluate_before_join = (
+                        evaluate_before_join
+                        or field.search
+                        or getattr(field, "auto_join", False)
+                    )
                     if field.comodel_name:
                         current_model = self.env.get(field.comodel_name)
-                    if current_model is None or has_x2many and has_auto_join:
+                    if current_model is None or (
+                        evaluate_before_negate and evaluate_before_join
+                    ):
                         break
-                this.evaluate_before_negate |= has_x2many
-                this.evaluate_always |= has_auto_join
-                if this.evaluate_before_negate and this.evaluate_always:
+                if evaluate_before_negate and evaluate_before_join:
                     break
+            this.evaluate_before_negate = evaluate_before_negate
+            this.evaluate_before_join = evaluate_before_join
 
     @api.model_create_multi
     def create(self, vals_list):
@@ -164,9 +212,11 @@ class IrFilters(models.Model):
     @api.multi
     def _evaluate(self):
         self.ensure_one()
-        return self.env[self.model_id].with_context(
-            safe_eval(self.context)
-        ).search(safe_eval(self.domain))
+        return (
+            self.env[self.model_id]
+            .with_context(**safe_eval(self.context))
+            .search(safe_eval(self.domain))
+        )
 
     @api.multi
     def button_save(self):
@@ -177,27 +227,59 @@ class IrFilters(models.Model):
         '''evaluate the filter and write a fixed [('id', 'in', [])] domain'''
         for this in self:
             ids = this._evaluate().ids
-            removed_filters = \
-                this.union_filter_ids + this.complement_filter_ids
             this.write({
-                'domain': str([('id', 'in', ids)]),
+                'domain': repr([('id', 'in', ids)]),
                 'union_filter_ids': [(6, 0, [])],
                 'complement_filter_ids': [(6, 0, [])],
             })
-            # if we removed inactive filters which are orphaned now, delete
-            # them
-            if removed_filters:
-                self.env.cr.execute(
-                    '''delete from ir_filters
-                    where
-                    not active and id in %s
-                    and not exists (select right_filter_id
-                    from ir_filters_union_rel where left_filter_id=id)
-                    and not exists (select right_filter_id
-                    from ir_filters_complement_rel where
-                    left_filter_id=id)''',
-                    (tuple(removed_filters.ids),)
-                )
+
+    @api.multi
+    def write(self, vals):
+        if not ("union_filter_ids" in vals or "complement_filter_ids" in vals):
+            return super().write(vals)
+        old = self.mapped("union_filter_ids") | self.mapped(
+            "complement_filter_ids"
+        )
+        res = super().write(vals)
+        new = self.mapped("union_filter_ids") | self.mapped(
+            "complement_filter_ids"
+        )
+        (old - new)._garbage_collect_helpers()
+        return res
+
+    @api.multi
+    def unlink(self):
+        helpers = self.mapped("union_filter_ids") | self.mapped(
+            "complement_filter_ids"
+        )
+        res = super().unlink()
+        helpers._garbage_collect_helpers()
+        return res
+
+    @api.multi
+    def _garbage_collect_helpers(self):
+        """Remove filters that have been made obsolete.
+
+        This method should only be called on filters that are/were in some
+        other filter's union_filter_ids or complement_filter_ids.
+
+        Filters are removed if they're not active and no longer used.
+        """
+        if not self:
+            return
+        self.env.cr.execute(
+            """DELETE FROM ir_filters
+            WHERE NOT active AND id IN %s
+            AND NOT EXISTS (
+                SELECT left_filter_id FROM ir_filters_union_rel
+                WHERE right_filter_id = id
+            )
+            AND NOT EXISTS (
+                SELECT left_filter_id FROM ir_filters_complement_rel
+                WHERE right_filter_id = id
+            )""",
+            (tuple(self.ids),),
+        )
 
     @api.multi
     def button_test(self):

web_advanced_filter/readme/CONTRIBUTORS.rst

@@ -1 +1,2 @@
 * Holger Brunn <hbrunn@therp.nl>
+* Jan Verbeek <jverbeek@therp.nl>

web_advanced_filter/static/description/index.html

@@ -3,7 +3,7 @@
 <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
 <head>
 <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
-<meta name="generator" content="Docutils 0.14: http://docutils.sourceforge.net/" />
+<meta name="generator" content="Docutils 0.15.1: http://docutils.sourceforge.net/" />
 <title>Advanced filters</title>
 <style type="text/css">
 
@@ -423,6 +423,7 @@ If you spotted it first, help us smashing it by providing a detailed and welcome
 <h2><a class="toc-backref" href="#id6">Contributors</a></h2>
 <ul class="simple">
 <li>Holger Brunn &lt;<a class="reference external" href="mailto:hbrunn&#64;therp.nl">hbrunn&#64;therp.nl</a>&gt;</li>
+<li>Jan Verbeek &lt;<a class="reference external" href="mailto:jverbeek&#64;therp.nl">jverbeek&#64;therp.nl</a>&gt;</li>
 </ul>
 </div>
 <div class="section" id="other-credits">

web_advanced_filter/static/src/js/web_advanced_filter.js

@@ -52,7 +52,7 @@ AdvancedFiltersMenu = DropdownMenu.extend({
         return [
             {
                 itemId: 'domain-header',
-                description: _t('Whole selection (criteria)'),
+                description: _t('Search query'),
             },
             {
                 itemId: 'domain-new',
@@ -61,14 +61,14 @@ AdvancedFiltersMenu = DropdownMenu.extend({
             },
             {
                 itemId: 'domain-union',
-                description: _t('To existing filter'),
+                description: _t('Add to existing filter'),
                 callback: this._advanced_filters_combine_with_existing.bind(
                     this, 'union', 'domain'
                 ),
             },
             {
                 itemId: 'domain-complement',
-                description: _t('Remove from existing filter'),
+                description: _t('Subtract from existing filter'),
                 callback: this._advanced_filters_combine_with_existing.bind(
                     this, 'complement', 'domain'
                 ),
@@ -84,14 +84,14 @@ AdvancedFiltersMenu = DropdownMenu.extend({
             },
             {
                 itemId: 'ids-union',
-                description: _t('To existing filter'),
+                description: _t('Add to existing filter'),
                 callback: this._advanced_filters_combine_with_existing.bind(
                     this, 'union', 'ids'
                 ),
             },
             {
                 itemId: 'ids-complement',
-                description: _t('Remove from existing filter'),
+                description: _t('Subtract from existing filter'),
                 callback: this._advanced_filters_combine_with_existing.bind(
                     this, 'complement', 'ids'
                 ),

web_advanced_filter/static/src/xml/web_advanced_filter.xml

@@ -2,17 +2,17 @@
 <templates>
     <t t-name="AdvancedFilter">
         <div role="separator" class="dropdown-divider"/>
-        <button type="button" class="dropdown-item o_advanced_filter o_closed_menu" aria-expanded="false">Whole selection (criteria)</button>
+        <button type="button" class="dropdown-item o_advanced_filter o_closed_menu" aria-expanded="false">Search query</button>
         <div class="dropdown-item-text o_advanced_filter-domain">
             <button type="button" class="dropdown-item o_advanced_filter-add-new">To new filter</button>
-            <button type="button" class="dropdown-item o_advanced_filter-add-existing">To existing filter</button>
-            <button type="button" class="dropdown-item o_advanced_filter-remove">Remove from existing filter</button>
+            <button type="button" class="dropdown-item o_advanced_filter-add-existing">Add to existing filter</button>
+            <button type="button" class="dropdown-item o_advanced_filter-remove">Subtract from existing filter</button>
         </div>
         <button type="button" class="dropdown-item o_advanced_filter o_closed_menu" aria-expanded="false">Marked records</button>
         <div class="dropdown-item-text o_advanced_filter-ids">
             <button type="button" class="dropdown-item o_advanced_filter-add-new">To new filter</button>
-            <button type="button" class="dropdown-item o_advanced_filter-add-existing">To existing filter</button>
-            <button type="button" class="dropdown-item o_advanced_filter-remove">Remove from existing filter</button>
+            <button type="button" class="dropdown-item o_advanced_filter-add-existing">Add to existing filter</button>
+            <button type="button" class="dropdown-item o_advanced_filter-remove">Subtract from existing filter</button>
         </div>
     </t>
 </templates>

web_advanced_filter/tests/test_web_advanced_filter.py

@@ -1,6 +1,10 @@
 # Copyright 2020 Therp BV <https://therp.nl>
 # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl.html).
+
 import json
+import uuid
+
+from odoo.osv import expression
 from odoo.tests.common import TransactionCase
 
 
@@ -93,24 +97,100 @@ class TestWebAdvancedFilter(TransactionCase):
             self.test_filter.complement_filter_ids.evaluate_before_negate
         )
         self.assertFalse(
-            self.test_filter.complement_filter_ids.evaluate_always
+            self.test_filter.complement_filter_ids.evaluate_before_join
         )
 
     def test_computed_field(self):
         """computed fields always need evaluation"""
-        self._combine('complement', [('display_name', '=', 'test')])
         self.assertTrue(
-            self.test_filter.complement_filter_ids.evaluate_before_negate
+            self.env["res.groups"]._fields["full_name"].search,
+            "res.groups.full_name doesn't have custom search, outdated test",
         )
+        filt = self._create_filter(
+            model_id="res.groups", domain=[("full_name", "=", "test")]
+        )
+        self.assertTrue(filt.evaluate_before_negate)
+        self.assertTrue(filt.evaluate_before_join)
+
+    def test_garbage_collection(self):
+        helper1 = self._create_filter(active=False)
+        helper2 = self._create_filter(active=False)
+        helper3 = self._create_filter()
+        filter1 = self._create_filter(
+            unions=helper1, complements=helper2 | helper3
+        )
+        filter2 = self._create_filter(unions=helper1)
+        self.assertTrue(helper1.exists())
+        self.assertTrue(helper2.exists())
+
+        filter1.unlink()
+        # Still used by others → kept
+        self.assertTrue(helper1.exists())
+        # Not used by others → deleted
+        self.assertFalse(helper2.exists())
+        # Not a "hidden" filter → kept
+        self.assertTrue(helper3.exists())
+
+        filter2.write({"union_filter_ids": [(5, 0, 0)]})
+        self.assertFalse(helper1.exists())
+        self.assertFalse(helper2.exists())
+        self.assertTrue(helper3.exists())
+
+    def test_autojoin_eval_necessity(self):
         self.assertTrue(
-            self.test_filter.complement_filter_ids.evaluate_always
+            self.env["res.partner"]._fields["user_ids"].auto_join,
+            "res.partner.user_ids is no longer autojoin, change the test",
         )
 
-    def _combine(self, action, domain):
+        normal_domain = [("id", "=", self.env.ref("base.main_partner").id)]
+        autojoin_domain = [("user_ids.login", "=", "admin")]
+        merged_domain = expression.OR([normal_domain, autojoin_domain])
+
+        search = self.env["res.partner"].search
+        self.assertNotEqual(
+            search(normal_domain) | search(autojoin_domain),
+            search(merged_domain),
+            "Searching autojoined fields is not buggy, is the workaround "
+            "still needed?",
+        )
+
+        helper = self._create_filter(domain=autojoin_domain)
+        main = self._create_filter(domain=normal_domain, unions=helper)
+        self.assertEqual(
+            main._evaluate(), search(normal_domain) | search(autojoin_domain)
+        )
+
+    def test_2many_eval_necessity(self):
+        category = self.env.ref("base.res_partner_category_14")
+        domain = [("category_id.name", "=", category.name)]
+        search = self.env["res.partner"].search
+        self.assertTrue(
+            search(domain) & search(["!"] + domain),
+            "Negating across 2many relations is not buggy, is the workaround "
+            "still needed?",
+        )
+
+        helper = self._create_filter(domain=domain)
+        main = self._create_filter(domain=domain, complements=helper)
+        self.assertFalse(main._evaluate())
+
+    def _create_filter(self, unions=(), complements=(), **vals):
+        vals.setdefault("model_id", "res.partner")
+        vals.setdefault("name", str(uuid.uuid4()))
+        vals.setdefault("domain", "[(1, '=', 1)]")
+        vals.setdefault(
+            "union_filter_ids", [(6, 0, [rec.id for rec in unions])]
+        )
+        vals.setdefault(
+            "complement_filter_ids", [(6, 0, [rec.id for rec in complements])]
+        )
+        return self.env["ir.filters"].create(vals)
+
+    def _combine(self, action, domain, *, model="res.partner"):
         """run the combination wizard with some default values"""
         self.env['ir.filters.combine.with.existing'].create({
             'action': action,
             'domain': json.dumps(domain),
-            'model': 'res.partner',
+            'model': model,
             'filter_id': self.test_filter.id
         }).button_save()

web_advanced_filter/views/ir_filters.xml

@@ -7,7 +7,7 @@
             <sheet position="before">
                 <header>
                     <button type="object" string="Test filter" name="button_test" class="btn-primary" />
-                    <button type="object" string="Freeze filter" name="button_freeze" attrs="{'invisible': [('is_frozen', '=', True)]}" help="Have this filter contain extly the records it currently contains, with no changes in the future. Be careful, you can&apos;t undo this operation!" confirm="Are you sure? You can&apos;t undo this operation!" />
+                    <button type="object" string="Freeze filter" name="button_freeze" attrs="{'invisible': [('is_frozen', '=', True)]}" help="Have this filter contain exactly the records it currently contains, with no changes in the future. Be careful, you can&apos;t undo this operation!" confirm="Are you sure? You can&apos;t undo this operation!" />
                 </header>
             </sheet>
             <group position="after">
@@ -21,10 +21,15 @@
                 </group>
             </group>
             <field name="domain" position="attributes">
-                <attribute name="attrs">{'readonly': ['|', ('union_filter_ids', '!=', [[6, False, []]]), ('complement_filter_ids', '!=', [[6, False, []]])]}</attribute>
+                <attribute name="attrs">
+                    {'readonly': ['|', ('union_filter_ids', '!=', []), ('complement_filter_ids', '!=', [])]}
+                </attribute>
             </field>
             <field name="domain" position="after">
-                <field name="domain_this" attrs="{'invisible': [('union_filter_ids', '=', [[6, False, []]]), ('complement_filter_ids', '=', [[6, False, []]])]}" />
+                <field name="domain_this"
+                       widget="domain"
+                       options="{'model': 'model_id'}"
+                       attrs="{'invisible': [('union_filter_ids', '=', []), ('complement_filter_ids', '=', [])]}" />
             </field>
         </field>
     </record>

web_advanced_filter/wizards/ir_filters_combine_with_existing.py

@@ -23,45 +23,47 @@ class IrFiltersCombineWithExisting(models.TransientModel):
     @api.multi
     def button_save(self):
         self.ensure_one()
-        this = self
-        domain = json.loads(this.domain)
-        is_frozen = (len(domain) == 1 and
-                     expression.is_leaf(domain[0]) and
-                     domain[0][0] == 'id')
+        domain = json.loads(self.domain)
+        is_frozen = (
+            len(domain) == 1
+            and expression.is_leaf(domain[0])
+            and domain[0][0] == "id"
+            and domain[0][1] == "in"
+        )
 
-        if this.action == 'union':
-            if is_frozen and this.filter_id.is_frozen:
+        if self.action == 'union':
+            if is_frozen and self.filter_id.is_frozen:
                 domain[0][2] = list(set(domain[0][2]).union(
-                    set(safe_eval(this.filter_id.domain)[0][2])))
-                this.filter_id.write({'domain': str(domain)})
+                    set(safe_eval(self.filter_id.domain)[0][2])))
+                self.filter_id.write({'domain': repr(domain)})
             else:
-                this.filter_id.write({
+                self.filter_id.write({
                     'union_filter_ids': [(0, 0, {
                         'name': '%s_%s_%d' % (
-                            this.filter_id.name, 'add', time.time()),
+                            self.filter_id.name, 'add', time.time()),
                         'active': False,
-                        'domain': str(domain),
-                        'context': this.context,
-                        'model_id': this.model,
-                        'user_id': this.filter_id.user_id.id or False,
+                        'domain': repr(domain),
+                        'context': repr(json.loads(self.context)),
+                        'model_id': self.model,
+                        'user_id': self.filter_id.user_id.id or False,
                     })],
                 })
-        elif this.action == 'complement':
-            if is_frozen and this.filter_id.is_frozen:
-                complement_set = set(safe_eval(this.filter_id.domain)[0][2])
+        elif self.action == 'complement':
+            if is_frozen and self.filter_id.is_frozen:
+                complement_set = set(safe_eval(self.filter_id.domain)[0][2])
                 domain[0][2] = list(
                     complement_set.difference(set(domain[0][2])))
-                this.filter_id.write({'domain': str(domain)})
+                self.filter_id.write({'domain': repr(domain)})
             else:
-                this.filter_id.write({
+                self.filter_id.write({
                     'complement_filter_ids': [(0, 0, {
                         'name': '%s_%s_%d' % (
-                            this.filter_id.name, 'remove', time.time()),
+                            self.filter_id.name, 'remove', time.time()),
                         'active': False,
-                        'domain': str(domain),
-                        'context': this.context,
-                        'model_id': this.model,
-                        'user_id': this.filter_id.user_id.id or False,
+                        'domain': repr(domain),
+                        'context': repr(json.loads(self.context)),
+                        'model_id': self.model,
+                        'user_id': self.filter_id.user_id.id or False,
                     })],
                 })