Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MERGE] crm, mass_mailing(_sms): improve blacklist management #45315

Closed

Conversation

@ticodoo
Copy link

ticodoo commented Feb 13, 2020

Description of the issue/feature this PR addresses

Update UX for blacklist management of "mass mailing" and "mass mailing sms" by:

  • Making phone and email blacklisting text / UX more consistent
  • Making it easier to remove a phone/email from blacklist
  • Improve GDPR compliance by adding in a "Reason for unblacklisting" via a wizard for traceability

Current behavior before PR:

A blacklisted phone number or email address can only be removed from their respective blacklist by going to the corresponding blacklist view [in mass_mailing_(sms)] and manually archiving the entry. All users could see that an email was blacklisted via an fa-ban icon added next to their corresponding field in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via extension). Some UI inconsistencies between the Phone Blacklist and Email Blacklist implementation exist.

Desired behavior after PR is merged:

Add additional fa-ban icons next to blacklisted phone numbers and make all instances of these icons clickable to remove their corresponding field value from blacklist using a wizard. When submitting a value to be unblacklisted, a "Reason for unblacklisting" is possible to help with GDPR compliance. Additionally use same wizard as a "Unblacklist" button within form view in phone/email blacklists.

Task ID: 2117635
Upgrade PR: odoo/upgrade#939

Copy link
Contributor

tde-banana-odoo left a comment

Technical validation


@api.model
def _default_email(self):
return self.env.context.get('email')

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 5, 2020

Contributor

You could simply set default_email in context and everything will be managed by standard behavior (default_get check for default_field_name in context).

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 5, 2020

Author

Neat, I didn't know that existed. Updated it!

def _default_email(self):
return self.env.context.get('email')

email = fields.Char(name="Email", default=_default_email, readonly=True)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 5, 2020

Contributor

I would make it required

This comment has been minimized.

Copy link
@ticodoo
def _default_phone(self):
return self.env.context.get('phone')

phone = fields.Char(string="Phone Number", default=_default_phone, readonly=True)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 5, 2020

Contributor

Same here with default and required

This comment has been minimized.

Copy link
@ticodoo
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 0f5041c to 5a94cd1 Mar 5, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 5, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 5a94cd1 to b668b91 Mar 5, 2020
@robodoo robodoo removed the CI 🤖 label Mar 5, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from b668b91 to f03d4e0 Mar 6, 2020
@robodoo robodoo added the CI 🤖 label Mar 6, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from f03d4e0 to a697c8a Mar 10, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 10, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from a697c8a to dab8a39 Mar 10, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 10, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from dab8a39 to 230cae7 Mar 11, 2020
@robodoo robodoo removed the CI 🤖 label Mar 11, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch 2 times, most recently from 7507802 to 5f94cbb Mar 11, 2020
@robodoo robodoo added the CI 🤖 label Mar 12, 2020
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 5f94cbb to 583046e Mar 12, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 12, 2020
ticodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 12, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "is_blacklisted" flag (this was already the case with
the icon, but now a user may try to unblacklist a value that is already
unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding
permissions will be able to click on the icon and be informed they do not
have permission to unblacklist. A wizard was determined to be the best way
to implement this unblacklisting for the following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: odoo#45315
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 583046e to 8fe25fd Mar 12, 2020
@robodoo robodoo removed the CI 🤖 label Mar 12, 2020
ticodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 12, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "is_blacklisted" flag (this was already the case with
the icon, but now a user may try to unblacklist a value that is already
unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding
permissions will be able to click on the icon and be informed they do not
have permission to unblacklist. A wizard was determined to be the best way
to implement this unblacklisting for the following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: odoo#45315
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 8fe25fd to f303960 Mar 12, 2020
@robodoo robodoo removed the CI 🤖 label Mar 24, 2020
ticodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 24, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "is_blacklisted" flag (this was already the case with
the icon, but now a user may try to unblacklist a value that is already
unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding unblacklist
permissions will be able to click on the icon. When they click on it, they
will be informed they do not have permission to unblacklist. A wizard was
determined to be the best way to implement this unblacklisting for the
following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: odoo#45315
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 25268f1 to 8f9efb8 Mar 24, 2020
@robodoo robodoo added the CI 🤖 label Mar 24, 2020
Copy link
Contributor

tde-banana-odoo left a comment

Good work, I think this cleaning is very interesting :) . Some naming remarks, globally models are ok ! Ping me if anything is unclear :) .

record = self._remove(email)
if not reason:
reason = _("Not specified")
record.message_post(body=_("Unblacklisting Reason: %s" % (reason)))

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

I am not sure we should log "not specified". Indeed this wastes storage as it adds no real value. If we have a reason we log it, otherwise we just skip the log. What do you think ?

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Removed it as discussed on task chatter.

<field name="email_state" invisible="1"/>
<field name="phone_state" invisible="1"/>
<label for="email_from" class="oe_inline"/>
<div class="o_row o_row_readonly">
<i class="fa fa-ban" style="color: red;" role="img" title="This email is blacklisted for mass mailing"
aria-label="Blacklisted" attrs="{'invisible': ['|', ('is_blacklisted', '=', False), ('partner_address_email', '!=', False)]}" groups="base.group_user"/>
<button name="action_remove_email_from_blacklist" icon="fa-ban" style="color: red;"

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

Couldn't we use standard bootstrap classes with danger / error / .. instead of manual coloring ?

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Done. The text-danger color isn't as bold as just making it color: red, but I think it's still good enough? (Picture below shows contrast). I also realized I could make it better by making the class fa fa-ban as well so the extra rectangle shading is now gone when you hover over the icon.
Screenshot from 2020-03-25 14-22-03

addons/crm/views/crm_lead_views.xml Show resolved Hide resolved
<i class="fa fa-ban" style="color: red;" role="img" title="This email is blacklisted for mass mailing"
aria-label="Blacklisted" attrs="{'invisible': [('is_blacklisted', '=', False)]}" groups="base.group_user"/>
<field name="email_from" widget="email"/>
<button name="action_remove_email_from_blacklist" icon="fa-ban" style="color: red;"

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

Same remarks done above apply here :D

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Bootstrap classes added + same issue with the partner_address_email condition

'view_mode': 'form',
'res_model': 'mail.blacklist.remove',
'target': 'new',
'context': self.env.context,

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

Context being normally propagated, is it necessary to give it ?

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Ah, I didn't know it wasn't necessary. A lot of other code still uses it :) Fixed it here though.

@@ -192,3 +211,19 @@ def _message_reset_bounce(self, email):
bounce counter of the record. """
super(MailBlackListMixin, self)._message_reset_bounce(email)
self.write({'message_bounce': 0})

def action_remove_email_from_blacklist(self):

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

It is a bit confusing to have actions on mail.blacklist model (technical) and on the mixin (more easily available) that are called a bit the same way ...

Purpose is to be able to identify mixin methods at a glance and avoid clash with existing code. That's why we have message_post, sms_post, rating_applyt for example. Maybe we could inaugurate a new namespace ? mail_action_blacklist_remove (mail -> generic mail.thread, action -> called by views, then shortening what it does) ? and phone_action_blacklist_remove ? What do you think ? If you have better proposal feel free :) .

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

I agree, I thought it was confusing when I was adding it in but didn't see another way to do it at the time. So to make sure I understood, I'm just renaming the methods to be clearer, but still leaving them duplicated between the mixin and the technical models, right? If so, done with your recommended phone_action_blacklist_remove

def action_remove_email_from_blacklist(self):
# wizard access rights currently not working as expected and allows users without access to
# open this wizard, therefore we check to make sure they have access before the wizard opens.
can_access = self.env['mail.blacklist.remove'].check_access_rights('create', raise_exception=False)

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

What about checking directly on mail.blacklist model instead ? It means synchronizing blacklist.remove and blacklist models but it seems better from model point of view (wizard is just a bridge between base record and its blacklist records).

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

OK done. I only check if they have write access now since I expect they will only ever be modifying an existing record via this wizard.

reason = fields.Char(name="Reason")

def action_unblacklist_apply(self):
blacklist = self.env['mail.blacklist']

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

Linting: unnecessary variable, can be inlined :)

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Oh punaise! Sorry I missed that.

@@ -121,3 +139,19 @@ def _phone_set_blacklisted(self):

def _phone_reset_blacklisted(self):
return self.env['phone.blacklist'].sudo()._remove([r.phone_sanitized for r in self])

def action_remove_phone_from_blacklist(self):

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

Same remarks for phone than on mail, if you change something simply ensure we propagate here for symmetry. Like having pens parallel to the table edge.

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Done as suggested: phone_action_blacklist_remove

reason = fields.Char(name="Reason")

def action_unblacklist_apply(self):
blacklist = self.env['phone.blacklist']

This comment has been minimized.

Copy link
@tde-banana-odoo

tde-banana-odoo Mar 25, 2020

Contributor

Same, variable can be inlined :)

This comment has been minimized.

Copy link
@ticodoo

ticodoo Mar 25, 2020

Author

Oh punaise!

ticodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 25, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "phone_sanitized_blacklisted" flag (this was already the
case with the icon, but now a user may try to unblacklist a value that is
already unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding unblacklist
permissions will be able to click on the icon. When they click on it, they
will be informed they do not have permission to unblacklist. A wizard was
determined to be the best way to implement this unblacklisting for the
following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: odoo#45315
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from 8f9efb8 to ee66880 Mar 25, 2020
@robodoo robodoo removed the CI 🤖 label Mar 25, 2020
Minor UX tweaks for more consistent and correct wording, visualization,
and editing ability for mass_mailing and mass_mailing_sms. Specifically:

- Fix all "This email is blacklisted for mass mailing" => ".. mass
  mailings" typos
- Make phone blacklist not editable (to align w/ email blacklist)
- Add "Archived" ribbon to phone and email blacklist
- Update menu items and actions
    - "Phone Blacklist" => "Blacklisted Phone Numbers"
    - "Blacklist" => "BLacklisted Email Addresses"

Supports task: 2117635
ticodoo added a commit to odoo-dev/odoo that referenced this pull request Mar 25, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "phone_sanitized_blacklisted" flag (this was already the
case with the icon, but now a user may try to unblacklist a value that is
already unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding unblacklist
permissions will be able to click on the icon. When they click on it, they
will be informed they do not have permission to unblacklist. A wizard was
determined to be the best way to implement this unblacklisting for the
following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: odoo#45315
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from ee66880 to b5eecba Mar 25, 2020
@robodoo robodoo added the CI 🤖 label Mar 25, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "phone_sanitized_blacklisted" flag (this was already the
case with the icon, but now a user may try to unblacklist a value that is
already unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding unblacklist
permissions will be able to click on the icon. When they click on it, they
will be informed they do not have permission to unblacklist. A wizard was
determined to be the best way to implement this unblacklisting for the
following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: #45315
@ticodoo ticodoo force-pushed the odoo-dev:master-mass_mailing_sms-blacklist-mgmt-tic branch from b5eecba to 87378a5 Mar 26, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2020
@tde-banana-odoo tde-banana-odoo changed the title [IMP] mass_mailing(_sms): blacklist management [MERGE] crm, mass_mailing(_sms): improve blacklist management Mar 26, 2020
@tde-banana-odoo

This comment has been minimized.

Copy link
Contributor

tde-banana-odoo commented Mar 26, 2020

@robodoo r+ rebase-merge

@robodoo robodoo added the r+ 👌 label Mar 26, 2020
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 26, 2020

Merge method set to rebase and merge, using the PR as merge commit message

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Mar 26, 2020

Linked pull request(s) odoo/upgrade#939 not ready. Linked PRs are not staged until all of them are ready.

robodoo pushed a commit that referenced this pull request Mar 26, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "phone_sanitized_blacklisted" flag (this was already the
case with the icon, but now a user may try to unblacklist a value that is
already unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding unblacklist
permissions will be able to click on the icon. When they click on it, they
will be informed they do not have permission to unblacklist. A wizard was
determined to be the best way to implement this unblacklisting for the
following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: #45315
robodoo added a commit that referenced this pull request Mar 26, 2020
<h2>Description of the issue/feature this PR addresses</h2>

Update UX for blacklist management of "mass mailing" and "mass mailing sms" by:
- Making phone and email blacklisting text / UX more consistent
- Making it easier to remove a phone/email from blacklist
- Improve GDPR compliance by adding in a "Reason for unblacklisting" via a wizard for traceability

<h2>Current behavior before PR:</h2>

A blacklisted phone number or email address can only be removed from their respective blacklist by going to the corresponding blacklist view [in mass_mailing_(sms)] and manually archiving the entry. All users could see that an email was blacklisted via an fa-ban icon added next to their corresponding field in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via extension). Some UI inconsistencies between the Phone Blacklist and Email Blacklist implementation exist.

<h2>Desired behavior after PR is merged:</h2>

Add additional fa-ban icons next to blacklisted phone numbers and make all instances of these icons clickable to remove their corresponding field value from blacklist using a wizard. When submitting a value to be unblacklisted, a "Reason for unblacklisting" is possible to help with GDPR compliance. Additionally use same wizard as a "Unblacklist" button within form view in phone/email blacklists.

Task ID: 2117635
Upgrade PR: odoo/upgrade#939

closes #45315

Related: odoo/upgrade#939
Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
robodoo pushed a commit that referenced this pull request Mar 26, 2020
Previously a blacklisted phone number or email address could only be
removed from their respective blacklist by going to the corresponding
blacklist view [in mass_mailing_(sms)] and manually
archiving/deactiviting the entry. All users could see that an email was
blacklisted via an fa-ban icon added next to their corresponding field
in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via
extension).

This commit adds additional fa-ban icons next to blacklisted phone
numbers and makes all instances of these icons clickable to remove them
from their corresponding blacklist using a wizard. There are several
limitations to this implementation including:

1. If both a mobile and phone number field appear within a crm lead
instance, then only the mobile field will indicate if it is blacklisted
(due to current implementation of PhoneMixin which only checks 1 phone
value against the blacklist and "sms" which returns mobile numbers first).
I.e. if a phone field value is blacklisted, but a value is typed into the
mobile field, then the phone field will never indicate that it is blacklisted.

2. If someone clicks on a unblacklisting icon after changing the
corresponding field value without clicking off the field input, then the
latest typed in value will be the one submitted for unblacklisting,
which may not actually be blacklisted (due to "is_blacklisted" flag
being a computed field and it not having a chance to re-compute to hide
the button).

3. If someone changes values in the blacklist, then someone who already
has a form open will not see icon disappear until something triggers a
re-compute of the "phone_sanitized_blacklisted" flag (this was already the
case with the icon, but now a user may try to unblacklist a value that is
already unblacklisted.)

4. Since the icon needs to be visible by everyone to indicate whether or
not a phone/email is blacklisted, users without corresponding unblacklist
permissions will be able to click on the icon. When they click on it, they
will be informed they do not have permission to unblacklist. A wizard was
determined to be the best way to implement this unblacklisting for the
following reasons:
  - A "Unblacklisting Reason" is needed and will not be stored as a field.
  - A field widget is unable to do this due to security settings +
    inability to refresh view after unblacklisting with a
    "Unblacklisting Reason" without wiping unsaved changes.

Additionally, to better align with GDPR, the corresponding form view for
the phone/email blacklist has been updated to use the same wizard to
track "Reason for unblacklisting". Unfortunately there is no
straightforward way to prevent direct "Archive" action, so users are
still able to bypass unblacklisting without being asked for a "reason
for unblacklisting".

Supports task: 2117635
Upgrade PR: odoo/upgrade#939
COM PR: #45315
robodoo added a commit that referenced this pull request Mar 26, 2020
<h2>Description of the issue/feature this PR addresses</h2>

Update UX for blacklist management of "mass mailing" and "mass mailing sms" by:
- Making phone and email blacklisting text / UX more consistent
- Making it easier to remove a phone/email from blacklist
- Improve GDPR compliance by adding in a "Reason for unblacklisting" via a wizard for traceability

<h2>Current behavior before PR:</h2>

A blacklisted phone number or email address can only be removed from their respective blacklist by going to the corresponding blacklist view [in mass_mailing_(sms)] and manually archiving the entry. All users could see that an email was blacklisted via an fa-ban icon added next to their corresponding field in the modules: crm, mass_mailing, mass_mailing_sms, and contacts (via extension). Some UI inconsistencies between the Phone Blacklist and Email Blacklist implementation exist.

<h2>Desired behavior after PR is merged:</h2>

Add additional fa-ban icons next to blacklisted phone numbers and make all instances of these icons clickable to remove their corresponding field value from blacklist using a wizard. When submitting a value to be unblacklisted, a "Reason for unblacklisting" is possible to help with GDPR compliance. Additionally use same wizard as a "Unblacklist" button within form view in phone/email blacklists.

Task ID: 2117635
Upgrade PR: odoo/upgrade#939

closes #45315

Related: odoo/upgrade#939
Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>
@robodoo robodoo added merged 🎉 and removed merging 👷 labels Mar 26, 2020
@robodoo robodoo closed this Mar 26, 2020
@robodoo robodoo deployed to merge Mar 26, 2020 Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.