Skip to content

[11.0][MIG] website_google_tag_manager: Migration to 11.0 - #524

Merged
pedrobaeza merged 3 commits into
OCA:11.0from
Tecnativa:11.0-mig-website_google_tag_manager
Oct 25, 2018
Merged

pedrobaeza merged 3 commits into
OCA:11.0from
Tecnativa:11.0-mig-website_google_tag_manager

Conversation

@cristinamartinrod

Copy link
Copy Markdown
Member

This module allows to configure Google Tag Manager on website.

cc @Tecnativa

sebalix and others added 2 commits October 5, 2018 11:37
* [ADD] New module 'website_google_tag_manager'

* [FIX] website_google_tag_manager - Added OCA as author

* [FIX] website_google_tag_manager - Fix RST syntax error (duplicate implicit target name 'google tag manager')

* [IMP] website_google_tag_manager - Replace 'openerp' tags by 'odoo' ones + Remove 'data' tags

@nikul-serpentcs nikul-serpentcs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor Change

Comment thread website_google_tag_manager/__manifest__.py
@pedrobaeza pedrobaeza added this to the 11.0 milestone Oct 11, 2018
@pedrobaeza
pedrobaeza requested a review from rafaelbn October 11, 2018 12:17

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also remove the SVG icon, as it's the standard one.

Comment thread website_google_tag_manager/models/res_config_settings.py Outdated
Comment thread website_google_tag_manager/views/website_templates.xml
_inherit = 'website'

google_tag_manager_key = fields.Char(u"Google Tag Manager Key")
has_google_tag_manager = fields.Boolean('Google Tag Manager')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding this boolean? Isn't better to simply add the key if present? If not, you should program an onchange that unchecking this field, the field is emptied.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be simpler, as in previous versions there was only the fields to include key_analytics, key_google maps, etc ..., but I have included that field to the same structure of website settings. Right now there isn't any group to include that field.

captura de pantalla 2018-10-16 a las 10 12 47

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you don't need an extra field for that. Use other structure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@pedrobaeza

Copy link
Copy Markdown
Member

Check also Runbot and Travis status

@cristinamartinrod
cristinamartinrod force-pushed the 11.0-mig-website_google_tag_manager branch 2 times, most recently from 38ff63c to 32f7e3a Compare October 15, 2018 09:05
@pedrobaeza

Copy link
Copy Markdown
Member

@cristinamartinrod can you attend my comments?

@cristinamartinrod
cristinamartinrod force-pushed the 11.0-mig-website_google_tag_manager branch 2 times, most recently from f103ee8 to 31ec332 Compare October 16, 2018 09:02
<xpath expr="//div[@id='google_maps_setting']" position="after">
<div class="col-xs-12 col-md-6 o_setting_box" id="google_tag_manager_setting">
<!-- <div class="o_setting_left_pane">
</div> -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups. Solve

</xpath>
</template>

<template id="web_layout" inherit_id="web.layout">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inherit from website.layout instead. Backend doesn't need this, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not for the backend

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then change it, website.layout is the frontend-only layout.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the problem is that this view is for meta-tags, and this show it in web.layout view

@@ -0,0 +1,5 @@
To configure this module, you need to:

#. Go to **Website > Configuration >Settings**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing one space after last >? 🧐

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeje Right 🔎

@cristinamartinrod
cristinamartinrod force-pushed the 11.0-mig-website_google_tag_manager branch from 743fa72 to 5e8207e Compare October 16, 2018 13:01
@cristinamartinrod
cristinamartinrod force-pushed the 11.0-mig-website_google_tag_manager branch 2 times, most recently from dec83aa to 7e6bcd2 Compare October 16, 2018 13:05
@cristinamartinrod

Copy link
Copy Markdown
Member Author

@pedrobaeza review please :)

@pedrobaeza

Copy link
Copy Markdown
Member

@cristinamartinrod The SVG file is still there...

@cristinamartinrod
cristinamartinrod force-pushed the 11.0-mig-website_google_tag_manager branch 2 times, most recently from 803a3d0 to ffb884a Compare October 17, 2018 12:15

@tarteo tarteo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread website_google_tag_manager/models/res_config_settings.py Outdated
@pedrobaeza

Copy link
Copy Markdown
Member

Please squash your fixups and we are ready to merge.

@cristinamartinrod
cristinamartinrod force-pushed the 11.0-mig-website_google_tag_manager branch from 2f5475c to 4552b8f Compare October 24, 2018 08:16
@pedrobaeza
pedrobaeza merged commit 648e3a7 into OCA:11.0 Oct 25, 2018
@pedrobaeza
pedrobaeza deleted the 11.0-mig-website_google_tag_manager branch October 25, 2018 11:29
@pedrobaeza pedrobaeza mentioned this pull request Oct 25, 2018
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants