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

[IMP] mass_mailing: improve onboarding #28497

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gla-odoo

gla-odoo commented Nov 8, 2018

Purpose : this commit improve various elements of the mass mailing interface :

  • make recipients field in mass mailing form required and add "mass mailing contact" in selection
  • new "Archive" button and color picker in dropdown menus of mailing lists and mailings
    kanban views.
  • icon change in list view in merge mailing lists form view
  • various little improvements in labels

task : https://www.odoo.com/web#id=1906531&action=333&active_id=965&model=project.task&view_type=form&menu_id=4720

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added seen 🙂 CI 🤖 and removed CI 🤖 labels Nov 8, 2018

@C3POdoo C3POdoo added the RD label Nov 9, 2018

@dbeguin

Here is my modest participation to this code review :)

@@ -444,7 +444,8 @@ def default_get(self, fields):
default=lambda self: self.env['mail.message']._get_default_from())
# recipients
mailing_model_real = fields.Char(compute='_compute_model', string='Recipients Real Model', default='mail.mass_mailing.contact', required=True)
mailing_model_id = fields.Many2one('ir.model', string='Recipients Model', domain=[('model', 'in', MASS_MAILING_BUSINESS_MODELS)],
mailing_model_id = fields.Many2one('ir.model', string='Recipients Model',
domain=['|', ('model', 'in', MASS_MAILING_BUSINESS_MODELS), ('model', '=', 'mail.mass_mailing.contact')],

This comment has been minimized.

@dbeguin

dbeguin Nov 12, 2018

Contributor

As MASS_MAILING_BUSINESS_MODELS is used only here, you can add directly 'mail.mass_mailing.contact into MASS_MAILING_BUSINESS_MODELS.

@@ -381,6 +382,11 @@
<div class="dropdown-menu" role="menu">
<a t-if="widget.editable" role="menuitem" type="edit" class="dropdown-item">Edit</a>
<a t-if="widget.deletable" role="menuitem" type="delete" class="dropdown-item">Delete</a>
<a role="menuitem" class="dropdown-item o_kanban_mailing_active" name="toggle_active" type="object">
<t t-if="record.active.raw_value">Archive</t>
<t t-if="!record.active.raw_value">Unarchive</t>

This comment has been minimized.

@dbeguin

dbeguin Nov 12, 2018

Contributor

use 'Restore' instead of 'Unarchive'

@@ -667,6 +674,10 @@
<t t-if="widget.deletable">
<a role="menuitem" type="delete" class="dropdown-item">Delete</a>
</t>
<a role="menuitem" class="dropdown-item o_kanban_mailing_active" name="toggle_active" type="object">
<t t-if="record.active.raw_value">Archive</t>
<t t-if="!record.active.raw_value">Unarchive</t>

This comment has been minimized.

@dbeguin

dbeguin Nov 12, 2018

Contributor

same remark as above

@@ -12,7 +12,7 @@
<field name="dest_list_id" attrs="{'invisible': [('merge_options', '=', 'new')], 'required': [('merge_options', '=', 'existing')]}"/>
<field name="archive_src_lists"/>
</group>
<field name="src_list_ids">
<field name="src_list_ids" class="o_merge_lists_tree">

This comment has been minimized.

@dbeguin

dbeguin Nov 12, 2018

Contributor

Remove this, not used.

@robodoo robodoo removed the CI 🤖 label Nov 13, 2018

@gla-odoo

This comment has been minimized.

gla-odoo commented Nov 13, 2018

Thanks for the review @dbeguin, changes applied!

@robodoo robodoo added the CI 🤖 label Nov 13, 2018

@tde-banana-odoo

Tech review: reads ok except JS code

@@ -47,4 +48,15 @@ FieldTextHtml.include({
},
});
ListRenderer.include({

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Nov 14, 2018

Contributor

I suppose the purpose of this code is to add a trash button instead of a cross to unlink items. Some questions ..
1/ Why is it specific to mass mailing ?
2/ If we really want something specific here couldn't we support a trashIcon parameter to propagate to the list view instead of doing hardcoded JS change ? (cc @ged-odoo )

Bisous.

This comment has been minimized.

@ged-odoo

ged-odoo Nov 14, 2018

Contributor

@tde-banana-odoo YES YES YES!

Please, do not merge this code. We are in 2018, we are civilized, and we do not hack stuff the wrong way. The proper way is definitely what tde said: add an attribute to the list view arch (trash_icon), make sure it is used by the list renderer, write a test, and update the documentation

This comment has been minimized.

@ged-odoo

ged-odoo Nov 14, 2018

Contributor

Note that the code above (on kanban view, and on fieldtexthmtl) is totally wrong as well... This is definitely not the way it should be done

This comment has been minimized.

@gla-odoo

gla-odoo Nov 14, 2018

I confess I am a bit glad of your remark, I didn't like that way but didn't dare to change the widget itself for such a customization. I'll rewrite it the clean way as soon as I can!

This comment has been minimized.

@ged-odoo

ged-odoo Nov 14, 2018

Contributor

don't be afraid to come visit us in our small house, if you need help.

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Nov 14, 2018

Contributor

Note that the code above (on kanban view, and on fieldtexthmtl) is totally wrong as well... This is definitely not the way it should be done

Probably, but just for this small onboarding I propose to not rewrite the mass mailing JS code ;) . I can do another tasks for that specific point.

This comment has been minimized.

@gla-odoo
[IMP] mass_mailing: improve onboarding
Purpose : this commit improve various elements of the mass mailing interface :
- make recipients field in mass mailing form required and add "mass mailing contact" in selection
- new "Archive" button and color picker in dropdown menus of mailing lists and mailings
kanban views.
- various little improvements in labels

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment