odoo [FIX] phone_validation: blacklisting unblacklisted phones should re-blacklist them

It may happen that user will try to blacklist a number that he already unblacklisted (archived blacklist record). Before that attempt was ignored. With this commit we simply blacklist the number again (so we unarchive existing record)

[Reproduce]

Install mass_mailing_sms
Open Blacklisted Phone Numbers (Email Marketing / Configuration / Blacklisted Email Addresses)
Blacklist a number N
Unblacklist number N
Go back to Blacklisted Phone Numbers
Blacklist a number N (again) -> BUG: no effect

opw-3700268

Mar 22 '24 17:03 nd-dew

.

Mar 22 '24 17:03 robodoo

Existing Behavior

https://github.com/odoo/odoo/assets/33809926/09eefeb6-ca3a-4c69-a87d-ed514d20becd

Solution 1: Raise an error

https://github.com/odoo/odoo/assets/33809926/4e331b80-af45-45f8-af9c-f5fb08666ee9

Solution 2: Blacklist unblacklisted

https://github.com/odoo/odoo/assets/33809926/88d1d8ce-344b-4d77-8ae3-d5a676605dad

Last Solution (unarchiving on the spot) got accepted by Product Owners

Applying last solution
Cleaning PR
Rebasing

Apr 05 '24 11:04 nd-dew

Fix moved:

from the new onchange method to the create method (which make more sense cause you don't want to unarchive records every-time you change number field on the form)

I had to accommodate the create method quite a lot tho :shrug: (but it's quite shiny now :+1: )

Apr 09 '24 11:04 nd-dew

Thanks for review @nle-odoo

Refatoring add/remove

In https://github.com/odoo/odoo/pull/158923#pullrequestreview-1991620737 you suggested simplifying the logic of add/remove methods. I agree that there is some redundancy.

I managed to refactor add and _add methods, however remove method handles the logic of deactivating or creating deactivated records. This is something my modification to the create method aren't including. To be precise the create method can create deactivated records just fine (created a test to make sure, see 2nd commit). However it can't deactivate them. I wonder if I should move that to create method, I don't know if that would make sense to move the following logic here:

    def _remove(self, numbers):
        """ Add de-activated or de-activate a phone blacklist entry.

        :param numbers: list of sanitized numbers """
        records = self.env["phone.blacklist"].with_context(active_test=False).search([('number', 'in', numbers)])
        todo = [n for n in numbers if n not in records.mapped('number')]
        if records:
            records.action_archive()  # That is sth current create is not doing
        if todo:
            records += self.create([{'number': n, 'active': False} for n in todo])
        return records

PS. nit remove is a interesting name for something that creates records or renders then non-active, but i guess there is some explanation for that

See 2nd commit below :arrow_down:

Apr 10 '24 17:04 nd-dew

applied RETH suggestions, rebased

May 02 '24 11:05 nd-dew

Added preserving the original order of numbers requested to create

May 02 '24 13:05 nd-dew

@tde-banana-odoo can we merge it ?

May 08 '24 06:05 nd-dew

@tde-banana-odoo :eyes:

May 15 '24 11:05 nd-dew

Noice, thanks for the extra fix :)

Note that runbot was broken this morning, could you rebase on last branches ? An exception was added to repair templates. After that we can send it :)

Cheers !

May 16 '24 07:05 tde-banana-odoo

@robodoo r+

May 21 '24 08:05 nd-dew

I'm sorry, @nd-dew: you can't review+.

May 21 '24 08:05 robodoo

@robodoo r+

May 21 '24 09:05 tde-banana-odoo