[9.0] [WIP] website_crm_privacy_policy module migrate by cubells · Pull Request #302 · OCA/website

cubells · 2016-12-20T17:26:34Z

Updated README
Updated views
Updated js

WIP

yajo · 2016-12-21T09:36:29Z

-if (typeof jQuery === 'undefined') { throw new Error('Website CRM privacy policy addon requires jQuery') }
+    var core = require('web.core');
+    var ajax = require('web.ajax');
+    var _t = core._t;


Add var animation = require('web_editor.snippets.animation');

yajo · 2016-12-21T09:36:55Z


-+function ($) {
-    'use strict';
+    crm_policy.registry.accept_policy = snippet_animation.Class.extend({


return animation.registry.accept_policy = animation.Class.extend({

yajo · 2016-12-21T09:37:05Z

-    $(document).ready(function() {
-        var _t = openerp._t;
+        start: function() {
+            var self = this;


Delete this line.

yajo · 2016-12-21T09:38:11Z

-        var _t = openerp._t;
+        start: function() {
+            var self = this;
+            this.$target.find('.o_website_form_send').on('click', function (e) {


Better get used to proxies: this.$('.o_website_form_send').on('click', $.proxy(this.accept_policy, this));

yajo · 2016-12-21T09:38:46Z

-        $('form[action="/crm/contactus"]').on('submit', function(e) {
-            // Validate privacy_policy is checked
-            if(!$('input[name="privacy_policy"]').is(':checked')) {
+        accept_policy: function(e) {


Other developers appreciate real names for variables, functions, etc. Use event instead of e for the sake of readability please.

yajo · 2016-12-21T09:39:13Z

+            e.preventDefault();  // Prevent the default submit behavior
+            this.$target.find('.o_website_form_send').off();  // Prevent users from crazy clicking
+
+            var self = this;


Remove this line, you are not using it.

yajo · 2016-12-21T11:55:02Z

-            e.preventDefault();  // Prevent the default submit behavior
+        accept_policy: function(event) {
+            event.preventDefault();  // Prevent the default submit behavior
            this.$target.find('.o_website_form_send').off();  // Prevent users from crazy clicking


The normal thing to do here is to disable the button instead of unbinding its events. Please make sure off() is doing what you expect.

yajo · 2016-12-21T11:56:29Z

-            var self = this;
-            if(!this.target.find('input[name="privacy_policy"]').is(':checked')) {
-                e.preventDefault();  // Prevent form from submitting
+            if(!this.$target.find('input[name="privacy_policy"]').is(':checked')) {


You can use the this.$('input[name="privacy_policy"]') shorthand available in animations.

yajo · 2016-12-21T11:58:07Z

-    crm_policy.registry.accept_policy = snippet_animation.Class.extend({
+
+    return animation.registry.accept_policy = animation.Class.extend({
        selector: '.s_website_form',


You are affecting more forms than just the website_crm one, right? Shouldn't this be reflected in the addon name?

How can affect "only" the website_crm form?

yajo · 2016-12-21T11:58:32Z

@@ -9,28 +9,24 @@ odoo.define('website_crm_privacy_policy.crm_policy', function (require) {
    var core = require('web.core');
    var ajax = require('web.ajax');


I think you are not using this dependency. Please check and drop it if so.

yajo

I have tested in runbot and current behavior is plainly broken. Alert triggers twice and form is never sent.

However, I checked that nowadays every major browser supports HTML5 validation, so how about simply removing all the JS stuff and replacing it by a required="required" attribute in the legal terms checkbox? That will trigger the native, beautiful and automatic browser validation with no extra work.

pedrobaeza · 2017-01-04T11:29:29Z

@yajo, please make the change as you have proposed.

yajo · 2017-01-04T12:02:05Z

I just did a quick update, so you can see the idea, but did not test it. Let's wait for runbot and see.

yajo · 2017-01-05T09:54:42Z

OK @pedrobaeza @cubells, this is interesting. With 4dee297 it works now.

The problem is not about using or not HTML5 validation, the problem is that the new website_form addon disables it.

I decided to write the long explanation of the problem as a bug in odoo/odoo#14942, to keep it as a reference. Please check it out.

pedrobaeza

Well, there's no visual sign that you lack of clicking on the accept terms, only "Invalid form". Can this be due to a missing class? For now, I'm merging, and if it can be improved, we make another PR.

… test fails on this dead link.

New `website_form` addon includes an undocumented validation system. By simply adapting layout to what it wants, we get validation for free.

pedrobaeza · 2017-01-05T21:05:20Z

In website_sale_require_legal we have a balloon:

Why don't we have it here?

pedrobaeza · 2017-01-05T21:07:38Z

I wait for merging until the question is solved.

yajo · 2017-01-09T09:36:49Z

Yep, that's all explained in odoo/odoo#14942.

The contactus form uses the website_form tools, which has these drawbacks. OTOH the checkout form is a pseudo-standard HTML5 form (with the pseudo part fixed in that addon) and thus benefits from having a clear & standard UI feedback on validation error.

Avoiding the website_form dependency would mean to refactor the whole website_crm addon, so this should be fixed upstream in master. For now, this is what we have.

BTW, doesn't the label get red when you don't check it?

pedrobaeza · 2017-01-09T09:51:55Z

No, it doesn't. You don't have any visual sign of what is incorrect, which is very anti-user friendly.

yajo · 2017-01-09T10:00:36Z

Sorry to correct you but it does:

It gets red, and the field has an *. This is the expected behavior for a website_form-based form. I agree it's a 💩, but that must be fixed in odoo/odoo#14942 😞

pedrobaeza · 2017-01-09T10:05:41Z

OK, it's very weird indeed and I didn't even noticed about the text turning into red (I was expecting the checkbox is surrounded by red). Anyway, if there's no other solution, let's go this way.

cubells force-pushed the 9.0-mig-website_crm_privacy_policy branch from 1ecc11c to 0d68c36 Compare December 20, 2016 17:27

yajo added the work in progress label Dec 21, 2016

yajo added this to the 9.0 milestone Dec 21, 2016

yajo requested changes Dec 21, 2016

View reviewed changes

cubells force-pushed the 9.0-mig-website_crm_privacy_policy branch from 4ff63e8 to 0c23225 Compare December 21, 2016 11:51

yajo requested changes Dec 21, 2016

View reviewed changes

yajo requested changes Dec 28, 2016

View reviewed changes

pedrobaeza mentioned this pull request Jan 4, 2017

Migration to version 9.0 #85

Closed

34 tasks

yajo force-pushed the 9.0-mig-website_crm_privacy_policy branch from b732f62 to 4dee297 Compare January 5, 2017 09:05

yajo approved these changes Jan 5, 2017

View reviewed changes

pedrobaeza approved these changes Jan 5, 2017

View reviewed changes

oca-transbot and others added 4 commits January 5, 2017 21:58

OCA Transbot updated translations from Transifex

d002d52

Dependency on module that provides /page/privacy. Otherwise the crawl…

0ba63aa

… test fails on this dead link.

[9.0] website_crm_privacy_policy: Migration to 9.0

3d5be6b

[REF][website_crm_privacy_policy] Remove JS. Use website_form's.

542c37f

New `website_form` addon includes an undocumented validation system. By simply adapting layout to what it wants, we get validation for free.

pedrobaeza force-pushed the 9.0-mig-website_crm_privacy_policy branch from 4dee297 to 542c37f Compare January 5, 2017 20:58

pedrobaeza removed the work in progress label Jan 5, 2017

pedrobaeza merged commit d954c8b into OCA:9.0 Jan 9, 2017

pedrobaeza deleted the 9.0-mig-website_crm_privacy_policy branch January 9, 2017 10:05

		@@ -9,28 +9,24 @@ odoo.define('website_crm_privacy_policy.crm_policy', function (require) {
		var core = require('web.core');
		var ajax = require('web.ajax');

Uh oh!

Conversation

cubells commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yajo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Jan 4, 2017

Uh oh!

yajo commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yajo commented Jan 5, 2017

Uh oh!

pedrobaeza left a comment

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Jan 5, 2017

Uh oh!

pedrobaeza commented Jan 5, 2017

Uh oh!

yajo commented Jan 9, 2017

Uh oh!

pedrobaeza commented Jan 9, 2017

Uh oh!

yajo commented Jan 9, 2017

Uh oh!

pedrobaeza commented Jan 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yajo left a comment •

edited

Loading

yajo commented Jan 4, 2017 •

edited

Loading