[10.0] Migrate website legal page to version 10.0 by hurrinico · Pull Request #310 · OCA/website

hurrinico · 2017-01-03T09:34:31Z

No description provided.

OCA Transbot updated translations from Transifex

eLBati

thanks

eugen-don · 2017-03-17T04:16:57Z

@hurrinico
thank you my friend, i was looking for this module... works like expected...

lasley

Oops actually looks like there's a few warnings

https://travis-ci.org/OCA/website/jobs/190224344#L456

lasley · 2017-03-17T18:02:12Z

Hint the warnings are due to us using website. in a few views, which is a namespace we don't own. I'm almost positive it should be as simple as removing that prefix from the ids & updating the link

pedrobaeza · 2017-03-18T12:45:23Z

@hurrinico please fix what @lasley says and I'll merge.

eLBati · 2017-05-05T13:38:28Z

@lasley @pedrobaeza @hurrinico changing

<template id="website.legal" name="Legal advice" page="True">

to

<template id="website_legal_page.legal" name="Legal advice" page="True">

would prevent page /page/legal to be served (404 error)

What do you suggest?

cc @antespi

lasley · 2017-05-05T15:00:09Z

Hmmmm I see the problem. Odoo SA's /page interface is enforcing the bad practice of editing namespaces you don't own.

A controller with an explicit route is my preferred. Interface or not, it is pretty commonly accepted practice to isolate everything by namespacing & breaking the good practice is not worth the 6-7 lines of code we'll save.

That's my opinion. Anyone else have thoughts?

antespi · 2017-05-05T18:32:40Z

I agree with @lasley

JonathanNEMRY · 2017-05-17T14:21:38Z

add setup here hurrinico#1

…up-jne [ADD] setup.py

hurrinico · 2017-05-24T07:46:19Z

@lasley @antespi i'm quite busy at the moment, and that type of mod require to change all the module, but prs are welcom

eLBati · 2017-05-26T14:14:00Z

@lasley I don't know how to implement what you suggest. Hints?

lasley · 2017-05-26T17:28:17Z

@eLBati - I'll actually submit a PR in the next few days. I was just looking, and it seems I had a branch doing exactly this in v9 + updating the privacy policy in accordance with some advice I have received from legal counsel.

yajo

I feel adding custom controllers is an overkill.

This addon just adds 3 pages, and it uses the tool Odoo uses for pages. It lets submodules to know the URL for that page and make sure it exists; these pages are expected to be changed by the user. Seems legit IMHO; custom controllers and so on would be more code to maintain for basically no benefit.

yajo · 2017-05-29T10:59:25Z

@lasley This is working perfectly and is holding other PRs; mind if we merge as it is? You can update the texts later, and we can defer the controller conversation to future v11...

lasley · 2017-05-30T17:31:57Z

@yajo - merging as is will break the 10.0 Runbot build going forward. Is that something we're OK with?

yajo · 2017-05-31T07:35:25Z

Hmm no, definitely. It seems it breaks due to 3 warnings:

2017-05-29 09:33:12,132 149 INFO openerp_test odoo.modules.loading: loading website_legal_page/views/website_legal.xml
2017-05-29 09:33:12,199 149 WARNING openerp_test odoo.addons.base.ir.ir_model: Creating the ir.model.data legal in module website instead of website_legal_page.
2017-05-29 09:33:12,199 149 INFO openerp_test odoo.modules.loading: loading website_legal_page/views/website_privacy.xml
2017-05-29 09:33:12,268 149 WARNING openerp_test odoo.addons.base.ir.ir_model: Creating the ir.model.data privacy in module website instead of website_legal_page.
2017-05-29 09:33:12,268 149 INFO openerp_test odoo.modules.loading: loading website_legal_page/views/website_terms.xml
2017-05-29 09:33:12,341 149 WARNING openerp_test odoo.addons.base.ir.ir_model: Creating the ir.model.data terms in module website instead of website_legal_page.

Hard to fix if we don't do custom controllers... OK, well if you can have it for today it would be great, we are at #ocacodesprint merging like crazy goats! 🐐

yajo

As said, we need to fix bots, which will probably include adding the controller.

pedrobaeza · 2017-06-09T00:22:10Z

What is the state of this?

yajo · 2017-06-09T07:52:05Z

You see, current design raises warnings, which kills runbot. @lasley suggested to add custom controllers and that's what we're waiting for.

pedrobaeza · 2017-06-09T08:02:40Z

OK, thanks for the heads-up. Let's see if @hurrinico can work on this.

hurrinico · 2017-06-09T13:40:25Z

Hi @pedrobaeza as i said few days ago, this mod is quite hard and at the moment i'm quite busy, so can be difficult to me redefine all the module

lasley · 2017-06-09T15:02:26Z

I've actually already done this, just need to add tests. Will submit a PR today

pedrobaeza · 2017-06-12T16:19:28Z

@lasley did you checked your code for making the PR?

lasley · 2017-06-12T22:32:43Z

hurrinico#3 - TBD whether tests pass

* Move legal pages to explicit /legal routes

[10.0] [IMP] website_legal: Move to explicit route

pedrobaeza · 2017-06-13T07:48:34Z

Everything is now green, so please update your reviews, @lasley and @yajo

yajo · 2017-06-13T08:39:01Z

+        type='http',
+        website=True,
+    )
+    def show(self, page='legal'):


SEO-wise, if you have a /legal and a /legal/legal page that are the same, you get penalized.

I think you should better not add a default page and require it (404 error otherwise), or make it redirect to /legal/legal if not page.

Remember to add **kwargs so that we can add ?debug=assets and so on.

Also, this controller's name should be legal instead of show IMHO.

/legal is the default route here. I guess technically /legal/legal would work too, but I'm pretty sure I rigged everything up as /legal

Oh I see, you're recommending explicitly denying (still early here, need coffee).

What about /legal/overview or something? I don't like the one duplicated

yajo · 2017-06-13T08:40:34Z

+        response = self.url_open('/legal')
+        self.assertEqual(
+            response.getcode(),
+            200,


This will change with my above comment.

yajo · 2017-06-13T08:41:21Z

          name="Legal advice Link">
    <xpath expr="//footer//div/span[@t-field='res_company.name']" position="after">
-        <span>- <a href="/page/legal">Legal advice</a></span>
+        <span>- <a href="/legal">Legal Advice</a></span>


/legal/advice maybe? Better than /legal/legal, right?

👍 will solve above

yajo · 2017-06-13T08:42:29Z

          name="Privacy policy Link">
    <xpath expr="//footer//div/span[@t-field='res_company.name']" position="after">
-        <span>- <a href="/page/privacy">Privacy policy</a></span>
+        <span>- <a href="/legal/privacy">Privacy Policy</a></span>


It turns out that it is better for SEO having a explicit name than a short one (unless the explicit one is too long). What if we take the chance to rename this one to /legal/privacy-policy?

yajo · 2017-06-13T08:42:51Z

          name="Terms of use Link">
    <xpath expr="//footer//div/span[@t-field='res_company.name']" position="after">
-        <span>- <a href="/page/terms">Terms of use</a></span>
+        <span>- <a href="/legal/terms">Terms of use</a></span>


This one could be /legal/usage-terms too.

I went with terms-of-use so that it more closely matches the link text

rafaelbn · 2017-06-13T11:49:14Z

@yajo as @hurrinico said #310 (comment) he cannot finish this PR so please make a PR to his branch to go ahead

lasley · 2017-06-13T16:37:12Z

Superseding with #353 in order to save a bunch of side merges/PRs

hurrinico mentioned this pull request Jan 3, 2017

[10.0] Migrate website cookie notice #309

Merged

1 task

pedrobaeza mentioned this pull request Jan 3, 2017

Migration to version 10.0 #262

Closed

35 tasks

hurrinico force-pushed the 10.0-mig-website_legal_page branch from 761bc8a to f4be405 Compare January 9, 2017 10:13

oca-transbot and others added 2 commits January 9, 2017 12:53

OCA Transbot updated translations from Transifex

a2dc5df

OCA Transbot updated translations from Transifex

[MIG] website_legal_page: Migrated to 10.0

a46c1a0

hurrinico force-pushed the 10.0-mig-website_legal_page branch from f4be405 to a46c1a0 Compare January 9, 2017 11:54

eLBati approved these changes Jan 9, 2017

View reviewed changes

lasley approved these changes Mar 17, 2017

View reviewed changes

lasley added this to the 10.0 milestone Mar 17, 2017

lasley suggested changes Mar 17, 2017

View reviewed changes

lasley added the needs fixing label Mar 17, 2017

[ADD] setup.py

9082adb

Merge pull request #1 from acsone/10.0-mig-website_legal_page-add-set…

8928f1c

…up-jne [ADD] setup.py

yajo approved these changes May 29, 2017

View reviewed changes

OsoTranquilo approved these changes May 29, 2017

View reviewed changes

yajo requested changes May 31, 2017

View reviewed changes

lasley and others added 2 commits June 12, 2017 16:19

[IMP] website_legal: Move to explicit route

311c6ca

* Move legal pages to explicit /legal routes

Merge pull request #3 from LasLabs/hurrinico-10.0-mig-website_legal_page

7baea79

[10.0] [IMP] website_legal: Move to explicit route

yajo requested changes Jun 13, 2017

View reviewed changes

lasley mentioned this pull request Jun 13, 2017

[10.0] [MIG] website_legal_page #353

Merged

lasley closed this Jun 13, 2017

Uh oh!

Conversation

hurrinico commented Jan 3, 2017

Uh oh!

eLBati left a comment

Choose a reason for hiding this comment

Uh oh!

eugen-don commented Mar 17, 2017

Uh oh!

lasley left a comment

Choose a reason for hiding this comment

Uh oh!

lasley commented Mar 17, 2017

Uh oh!

pedrobaeza commented Mar 18, 2017

Uh oh!

eLBati commented May 5, 2017

Uh oh!

lasley commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antespi commented May 5, 2017

Uh oh!

JonathanNEMRY commented May 17, 2017

Uh oh!

hurrinico commented May 24, 2017

Uh oh!

eLBati commented May 26, 2017

Uh oh!

lasley commented May 26, 2017

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

yajo commented May 29, 2017

Uh oh!

lasley commented May 30, 2017

Uh oh!

yajo commented May 31, 2017

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Jun 9, 2017

Uh oh!

yajo commented Jun 9, 2017

Uh oh!

pedrobaeza commented Jun 9, 2017

Uh oh!

hurrinico commented Jun 9, 2017

Uh oh!

lasley commented Jun 9, 2017

Uh oh!

pedrobaeza commented Jun 12, 2017

Uh oh!

lasley commented Jun 12, 2017

Uh oh!

pedrobaeza commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley Jun 13, 2017 • 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!

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!

rafaelbn commented Jun 13, 2017

lasley commented May 5, 2017 •

edited

Loading

lasley Jun 13, 2017 •

edited

Loading