Skip to content
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

Master save snippet template fja #40408

Closed

Conversation

@fja-odoo
Copy link
Contributor

fja-odoo commented Nov 18, 2019

[IMP] website, web_editor: allow saving of snippets

Now the user can save snippets to use them later.

task-2120409

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

@robodoo robodoo added the seen 🙂 label Nov 18, 2019
@C3POdoo C3POdoo added the RD label Nov 18, 2019
@fja-odoo fja-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from b0b41b7 to eae47b7 Nov 18, 2019
@fja-odoo fja-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch 3 times, most recently from 3ce52f0 to 21ecf2d Dec 2, 2019
@robodoo robodoo added the CI 🤖 label Dec 2, 2019
Copy link
Contributor

qsm-odoo left a comment

Code in web_editor calling routes of website... start by changing that.

addons/website/views/snippets.xml Outdated Show resolved Hide resolved
addons/website/views/snippets.xml Outdated Show resolved Hide resolved
addons/website/views/snippets.xml Outdated Show resolved Hide resolved
@fja-odoo fja-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from 21ecf2d to a288067 Jan 8, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 8, 2020
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from a288067 to ff781a1 Jan 9, 2020
@robodoo robodoo removed the CI 🤖 label Jan 9, 2020
Copy link
Contributor

qsm-odoo left a comment

I adapted by myself 👍

if not thumbnail_url:
thumbnail_url = self.get_thumbnail(snippet_class)

model = template.split('.')[0]

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

My guess is model = app ?

/**
*
*/
saveTemplate() {
Comment on lines 2960 to 2963

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

@fja-odoo Missing comment and missing : function (), you don't need me to see that.

Also: the method is async it should wait for dialog choices before returning -> EDIT: doing that seems to break everything so I had to adapt :/

return '%s.%s' % (template, key)

@http.route(['/snippet/save'], type='json', auth="public", website=True)
def snippet_save(self, name, arch, template, snippet_class=None, thumbnail_url=None):

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

Missing documentation

'arch': xml_arch,
})
custom_section = View.search([('key', '=', template)])
View.create({

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

I am not sure if we spoke about this or not anymore but I checked with the team and it should be website-specific in the end.

@@ -490,3 +492,14 @@ def content_image(self, id=None, max_width=0, max_height=0, **kw):
def favicon(self, **kw):
# when opening a pdf in chrome, chrome tries to open the default favicon url
return self.content_image(model='website', id=str(request.website.id), field='favicon', **kw)

class WebsiteEditor(Web_Editor):

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

Why putting this in retrocompatibility routes ?

const self = this;
// parent is null to prevent the dialog from being destroyed when reloading the snippets.
new Dialog(null, {
title: _t("Snippet Template"),

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

We don't show the "snippet" word to users...

if ($selectedSnippet) {
await this._activateSnippet($selectedSnippet);
}
Comment on lines 1297 to 1299

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

In fact seems better if no snippet is selected again afterwards ?

this.customizePanel = document.createElement('div');
this.customizePanel.classList.add('o_we_customize_panel', 'd-none');
this.$el.append(this.customizePanel);

this.invisibleDOMPanelEl = document.createElement('div');
this.invisibleDOMPanelEl.classList.add('o_we_invisible_el_panel');
this.invisibleDOMPanelEl.appendChild(
$('<div/>', {
text: _t('Invisible Elements'),
class: 'o_panel_header',
}).prepend(
$('<i/>', {class: 'fa fa-eye-slash'})
)[0]
);
this.$el.append(this.invisibleDOMPanelEl);
return this._updateInvisibleDOM();
Comment on lines 865 to 880

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

Why was this moved ? EDIT: ok... but _updateInvisibleDOM here is bad :/ I had to adapt.


<!-- Save Snippet -->
<div data-js="saveTemplate"
data-selector="section">

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

Not working with s_popup... which is one of the most interesting... also cannot work since in website :/

new Dialog(this, {
size: 'medium',
title: _t('Confirmation'),
$content: $('<p>' + _t(`Are you sure you want to delete the snippet : ${$snippet.attr('name')} ?` + '</p>')),

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

@fja-odoo You translate the ending </p> here...

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 9, 2020

Contributor

and the layout is broken since you cannot use a p as modal body...

@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch 4 times, most recently from 19d4888 to 9465c32 Jan 9, 2020
@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Jan 13, 2020

I am not sure if we spoke about this or not anymore but I checked with the team and it should be website-specific in the end.

@fja-odoo ... in the end website 2 crashed and was not possible to edit anymore if a snippet was saved in website 1 with your implementation 😥 And snippet saving in mass mailing was never gonna be able to work. Tricky to implement correctly with controllers, I used model methods instead to solve the bug.

@robodoo robodoo added the CI 🤖 label Jan 13, 2020
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from 9465c32 to 0f493d3 Jan 13, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 13, 2020
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from 0f493d3 to 90463b8 Jan 13, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 13, 2020
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from 90463b8 to d523dc3 Jan 14, 2020
@robodoo robodoo removed the CI 🤖 label Jan 14, 2020
@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Jan 14, 2020

Saving reference of commit which allow to save a snippet for all websites: d523dc3 (finally reverted)

@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch 3 times, most recently from 5ddb102 to d9c789a Jan 14, 2020
@qsm-odoo qsm-odoo requested a review from rdeodoo Jan 14, 2020
@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Jan 14, 2020

@rdeodoo a last check from you then we merge ? :) Thanks !

@robodoo robodoo added the CI 🤖 label Jan 14, 2020
Copy link
Contributor

rdeodoo left a comment

I'll finish tomorrow morning :)

addons/web_editor/models/ir_ui_view.py Outdated Show resolved Hide resolved
addons/web_editor/models/ir_ui_view.py Outdated Show resolved Hide resolved
snippet = self.browse(view_id)
key = snippet.key.split('.')[1]
custom_key = self._get_snippet_addition_view_key(template_key, key)
self.search([('key', '=', custom_key)]).unlink()

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo Jan 14, 2020

Contributor

can unlink both in one sql request

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 14, 2020

Contributor

Would you do it though? ORM instead of SQL seems ok for such a small use case

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo Jan 16, 2020

Contributor

yes I always concat records then unlink in those case, when unlink are done that close, no point to fire 2 requests

custom_key_view = self.search([('key', '=', custom_key)]).unlink()
(snippet | custom_key_view).unlink()

nothing critical tho could keep it that way

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Jan 16, 2020

Contributor

Ok, did not think about doing it this way, it is indeed better. I'll change.

addons/web_editor/models/ir_ui_view.py Show resolved Hide resolved
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from d9c789a to c85a601 Jan 14, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 14, 2020
Copy link
Contributor

rdeodoo left a comment

LGTM, just a few feedback (except for the _get_snippet_addition_view_key that I'd remove but keeping it is ok if you prefer).

Also, don't change it but just something I found weird, sometimes ir.ui.view is addresses as template, sometimes as snippet, sometimes as view, would be better to always be named view, easier when reading.
eg:

def save_snippet(self, ..., template_key, ...):
    """
    :param template_key: the key of the view

Why not view_key (as the docstring suggest it).

div = u'<div name="%s" ... data-oe-snippet-id="%s">' % (
     escape(pycompat.to_text(view_id)),
)

Same, why not data-oe-view-id (as the escape is suggesting).

snippet = self.browse(view_id)
key = snippet.key.split('.')[1]
custom_key = self._get_snippet_addition_view_key(template_key, key)
self.search([('key', '=', custom_key)]).unlink()

This comment has been minimized.

Copy link
@rdeodoo

rdeodoo Jan 16, 2020

Contributor

yes I always concat records then unlink in those case, when unlink are done that close, no point to fire 2 requests

custom_key_view = self.search([('key', '=', custom_key)]).unlink()
(snippet | custom_key_view).unlink()

nothing critical tho could keep it that way

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Jan 16, 2020

Also, don't change it but just something I found weird, sometimes ir.ui.view is addresses as template, sometimes as snippet, sometimes as view, would be better to always be named view

Yeah, I just kept felix's names. Although I added "_key" at the end to make it more obvious. I think it is now "template" for the view which regroups snippets and "snippet" for snippet views. I won't change since not critical and won't risk to break something for this 👍

@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from c85a601 to 714fc3b Jan 16, 2020
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 16, 2020
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from 714fc3b to f874e3d Jan 16, 2020
@robodoo robodoo removed the CI 🤖 label Jan 16, 2020
* website, mass_mailing, website_event, website_form,
  website_mass_mailing, website_sale

Now the user can save snippets to use them on other pages.

task-2120409

Co-authored-by: qsm-odoo <qsm@odoo.com>
@qsm-odoo qsm-odoo force-pushed the odoo-dev:master-save-snippet-template-fja branch from f874e3d to 0ef845c Jan 16, 2020
@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Jan 16, 2020

robodoo pushed a commit that referenced this pull request Jan 16, 2020
* website, mass_mailing, website_event, website_form,
  website_mass_mailing, website_sale

Now the user can save snippets to use them on other pages.

task-2120409

closes #40408

Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
Co-authored-by: qsm-odoo <qsm@odoo.com>
@robodoo robodoo closed this Jan 16, 2020
@robodoo robodoo deployed to merge Jan 16, 2020 Active
@qsm-odoo qsm-odoo deleted the odoo-dev:master-save-snippet-template-fja branch Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.