[MIG] website_mass_mailing_name: Migration to 12.0 #385

victormmtorres · 2019-05-21T17:49:54Z

No description provided.

victormmtorres · 2019-05-22T08:56:21Z

Once test fixed I'll set to review

victormmtorres · 2019-05-28T15:31:32Z

cc @Tecnativa

website_mass_mailing_name/static/src/css/website_mass_mailing_name.scss

website_mass_mailing_name/static/src/js/editor_and_public_tour.js

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js

victormmtorres · 2019-05-29T08:43:57Z

Changes done @yajo

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js

victormmtorres · 2019-05-29T10:05:01Z

@yajo Changes done

Tardo

LGTM, tested on runbot

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js

pedrobaeza · 2019-06-03T10:26:00Z

When someone approves a PR, but put comments, they are only informative ones, that can be changed if you want, but not blocking. I understand those 2 as this type, for knowing more JS tricks, accelerators, and so.

I'm seeing a warning in runbot, but logs are not accessible. I have launched a rebuild. Please check when finished, and if you want, make the proposed improvements, that are not blocking.

victormmtorres · 2019-06-03T15:24:01Z

Changes done @pedrobaeza @Tardo please review

Tardo · 2019-06-03T15:41:22Z

@victormmtorres thx, good work!

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js

website_mass_mailing_name/templates/snippets.xml

chienandalu · 2019-06-05T09:42:40Z

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js

-        on_click: function() {
-            var email_error = !this.$email.val().match(/.+@.+/),
+        _onClick: function() {
+            function validateEmail(sEmail) {


Sorry @victormmtorres I don't fully understand what you want to achieve here 🙂 You should not need to validate email here don't you? You already have that in the main method: https://github.com/odoo/odoo/blob/12.0/addons/website_mass_mailing/static/src/js/website_mass_mailing.js#L41-L64

I will change this part

Let you have the name field in the mass mailing subscription snippet. Leave most logic to base module. Make name field autofilled if user data is available. Add tour test.

Test does not work, probably due to the framework. Leaving out the last step and hope for the best for v9.

[FIX]website_mass_mailing_name: Show disabled name. When the snippet appears as disabled, do not hide the name field.

- Relicense to LGPL. - Fix all known issues. - Enable tour only in demo instances. - Fix Sass headers. - Remove compiled css and maps. - Update JS modules to new api. - Update tour to new tours api. - Update module structure to match latest template.

Under some integration environment, the public part of the tour fails to load, I don't know exactly why (or better, I don't know why it was working before, because it was deleting the phantomjs session entirely). In any case, with this fix, it works there and is faster to test.

victormmtorres · 2019-06-14T17:14:41Z

@yajo please help me out as none of the suggested changes are working and I spend some time investigating and I believe at least the validation for email should work

victormmtorres · 2019-06-17T08:48:58Z

Changes done @chienandalu @yajo

chienandalu

Thanks @victormmtorres !

chienandalu · 2019-06-17T09:00:18Z

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js

-            if (email_error || name_error) {
-                this.$target.addClass("has-error")
+        _onClick: function() {
+            var email_valid = this.$email[0].reportValidity(),


I still think we're checking this twice. Anyway, not blocking.

Yes, but there's a reason for that.

If we do it like it is now, UX is way better. The user gets a clear bubble indicating where the problem is. Maybe it's a wrong email, or maybe it's an empty field.

If we rely on upstream validation, it would just set all red (in fact, both fields) and say nothing. It's OK if there's only one field, but now that there are 2, if they are validated in different ways, it gets inconsistent.

The way odoo does it supports very old browsers, but has the problems explained in odoo/odoo#14942.

However it's true that you can add a comment explaining why we're duplicating this validation here, @victormmtorres.

Add the comment and I'll merge

comment added

OCA-git-bot · 2019-06-17T09:41:57Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

pedrobaeza added this to the 12.0 milestone May 21, 2019

OCA-git-bot mentioned this pull request May 21, 2019

Migration to version 12.0 #306

Closed

24 tasks

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from 074e240 to e05b836 Compare May 22, 2019 08:51

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from e05b836 to 70e9167 Compare May 28, 2019 15:30

victormmtorres marked this pull request as ready for review May 28, 2019 15:31

pedrobaeza requested a review from Tardo May 28, 2019 15:31

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from 70e9167 to 3dde16b Compare May 28, 2019 15:32

yajo requested changes May 29, 2019

View reviewed changes

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from 3dde16b to b6063d5 Compare May 29, 2019 08:43

yajo requested changes May 29, 2019

View reviewed changes

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js Outdated Show resolved Hide resolved

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from b6063d5 to be463f8 Compare May 29, 2019 10:04

Tardo approved these changes Jun 3, 2019

View reviewed changes

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js Outdated Show resolved Hide resolved

website_mass_mailing_name/static/src/js/website_mass_mailing_name.js Outdated Show resolved Hide resolved

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from be463f8 to 3901215 Compare June 3, 2019 15:23

yajo requested changes Jun 4, 2019

View reviewed changes

chienandalu suggested changes Jun 5, 2019

View reviewed changes

yajo and others added 7 commits June 14, 2019 14:03

[8.0][ADD][website_snippet_mass_mailing_partner] Snippet + name

e777cdb

Let you have the name field in the mass mailing subscription snippet. Leave most logic to base module. Make name field autofilled if user data is available. Add tour test.

Workaround test failure

7e38be4

Test does not work, probably due to the framework. Leaving out the last step and hope for the best for v9.

[FIX][website_mass_mailing_name] Show disabled name. (OCA#83)

6e3df3f

[FIX]website_mass_mailing_name: Show disabled name. When the snippet appears as disabled, do not hide the name field.

OCA Transbot updated translations from Transifex

f183560

[UPD] Update website_mass_mailing_name.pot

57ff362

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from 3901215 to 19a3e30 Compare June 17, 2019 08:11

chienandalu approved these changes Jun 17, 2019

View reviewed changes

yajo approved these changes Jun 17, 2019

View reviewed changes

OCA-git-bot added approved ready to merge labels Jun 17, 2019

[MIG] website_mass_mailing_name: Migration to 12.0

2ff470b

victormmtorres force-pushed the 12.0-mig-website_mass_mailing_name branch from 19a3e30 to 2ff470b Compare June 17, 2019 09:54

yajo merged commit a3706b5 into OCA:12.0 Jun 17, 2019

yajo deleted the 12.0-mig-website_mass_mailing_name branch June 17, 2019 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG] website_mass_mailing_name: Migration to 12.0 #385

[MIG] website_mass_mailing_name: Migration to 12.0 #385

victormmtorres commented May 21, 2019

victormmtorres commented May 22, 2019

victormmtorres commented May 28, 2019

victormmtorres commented May 29, 2019

victormmtorres commented May 29, 2019

Tardo left a comment

pedrobaeza commented Jun 3, 2019

victormmtorres commented Jun 3, 2019

Tardo commented Jun 3, 2019

chienandalu Jun 5, 2019

victormmtorres Jun 14, 2019

victormmtorres commented Jun 14, 2019

victormmtorres commented Jun 17, 2019

chienandalu left a comment

chienandalu Jun 17, 2019

yajo Jun 17, 2019

pedrobaeza Jun 17, 2019

pedrobaeza Jun 17, 2019

victormmtorres Jun 17, 2019

OCA-git-bot commented Jun 17, 2019

[MIG] website_mass_mailing_name: Migration to 12.0 #385

[MIG] website_mass_mailing_name: Migration to 12.0 #385

Conversation

victormmtorres commented May 21, 2019

victormmtorres commented May 22, 2019

victormmtorres commented May 28, 2019

victormmtorres commented May 29, 2019

victormmtorres commented May 29, 2019

Tardo left a comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 3, 2019

victormmtorres commented Jun 3, 2019

Tardo commented Jun 3, 2019

chienandalu Jun 5, 2019

Choose a reason for hiding this comment

victormmtorres Jun 14, 2019

Choose a reason for hiding this comment

victormmtorres commented Jun 14, 2019

victormmtorres commented Jun 17, 2019

chienandalu left a comment

Choose a reason for hiding this comment

chienandalu Jun 17, 2019

Choose a reason for hiding this comment

yajo Jun 17, 2019

Choose a reason for hiding this comment

pedrobaeza Jun 17, 2019

Choose a reason for hiding this comment

pedrobaeza Jun 17, 2019

Choose a reason for hiding this comment

victormmtorres Jun 17, 2019

Choose a reason for hiding this comment

OCA-git-bot commented Jun 17, 2019