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

[IMP] bus: notify user when assets have changed #39875

Open
wants to merge 4 commits into
base: master
from

Conversation

@kebeclibre
Copy link
Contributor

kebeclibre commented Nov 6, 2019

Use case:
When the server is restarted, the python is updated,
but some users may have an ongoing session in a browser tab
This may lead to code being unsynchronized and ultimately to some
odd bugs

Purpose:
When we are in such a case, notify connected users that assets have changed
And propose them to reload the page

Known caveats:
This is not a developer's feature
Since assets computing is ORM cached, they have limited
opportunities to rebuild. Namely, the feature won't trigger
each time the JS has changed, rather, it will
when JS has changed AND the cache has been reset somehow

This not a portal/website feature either
Business clients won't be notified that the JS has changed

Task 2034462

@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 554ea4c to 887c97b Nov 6, 2019
@robodoo robodoo added the seen 🙂 label Nov 6, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch 2 times, most recently from 11fe139 to 58c99ff Nov 6, 2019
@C3POdoo C3POdoo added the RD label Nov 6, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch 4 times, most recently from 4ffa165 to 1e14020 Nov 6, 2019
@robodoo robodoo added the CI 🤖 label Nov 6, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 1e14020 to 8424a8c Nov 7, 2019
@robodoo robodoo removed the CI 🤖 label Nov 7, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch 2 times, most recently from 2067073 to b117401 Nov 7, 2019
@kebeclibre kebeclibre requested a review from KangOl Nov 7, 2019
@kebeclibre kebeclibre changed the title [IMP] web, bus: notify user when assets have changed [IMP] bus: notify user when assets have changed Nov 7, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from b117401 to 3fdbefa Nov 7, 2019
@robodoo robodoo added the CI 🤖 label Nov 7, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 3fdbefa to 83b3ae7 Nov 7, 2019
@robodoo robodoo removed the CI 🤖 label Nov 7, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 83b3ae7 to 330f583 Nov 7, 2019
@robodoo robodoo added the CI 🤖 label Nov 7, 2019
Copy link
Contributor

KangOl left a comment

I don't like your approach to triggering the change on the bus.

Why not directly push the notification on the bus (it only be sent once the cursor is committed)
If more than 1 asset is changed, the client will receive all bus messages in the same poll response.

The js code should then be adapted as a bus message will only contain 1 update (instead of a dict with all changes)

Can you also change the notification shown to the user to not talk about "code"? It's too cryptic for end-users.

@override
"""
saved = super(BusAssetsBundle, self).save_attachment(type, content)

This comment has been minimized.

Copy link
@KangOl

KangOl Nov 7, 2019

Contributor

Embrace python3!

Suggested change
saved = super(BusAssetsBundle, self).save_attachment(type, content)
saved = super().save_attachment(type, content)
"""
saved = super(BusAssetsBundle, self).save_attachment(type, content)

if type == 'js' and self.name in self.TRACKED_BUNDLES:

This comment has been minimized.

Copy link
@KangOl

KangOl Nov 7, 2019

Contributor

Why only js files?
It also works for (compiled) css files.

addons/website/models/ir_qweb.py Show resolved Hide resolved
@robodoo robodoo removed the CI 🤖 label Nov 7, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 99919c1 to 44aea83 Nov 7, 2019
"""
saved = super().save_attachment(type, content)

if self.name in self.TRACKED_BUNDLES:

This comment has been minimized.

Copy link
@KangOl

KangOl Nov 7, 2019

Contributor

Just to be sure.

Suggested change
if self.name in self.TRACKED_BUNDLES:
if request and self.name in self.TRACKED_BUNDLES:

This comment has been minimized.

Copy link
@kebeclibre

kebeclibre Nov 7, 2019

Author Contributor

I don't need the request anymore, since I don't need to enforce that the same request that computed the bundles flushes the messages.
I do need an env though

@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 44aea83 to 7e8fe39 Nov 7, 2019
@KangOl
KangOl approved these changes Nov 7, 2019
Copy link
Contributor

KangOl left a comment

It theorically works.

Still need to to test locally.

this._assets = {};

return this._super.apply(this, arguments).then(function () {
var $assets = $('head script[data-asset-xmlid], link[data-asset-xmlid]');

This comment has been minimized.

Copy link
@KangOl

KangOl Nov 7, 2019

Contributor

assets may be anywhere (not necessary in <head>)

Suggested change
var $assets = $('head script[data-asset-xmlid], link[data-asset-xmlid]');
var $assets = $('script[data-asset-xmlid],link[data-asset-xmlid]');

Maybe get any tag with this attribute

Suggested change
var $assets = $('head script[data-asset-xmlid], link[data-asset-xmlid]');
var $assets = $('*[data-asset-xmlid]');
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 7e8fe39 to ccd54ec Nov 7, 2019
@robodoo robodoo added the CI 🤖 label Nov 7, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from d6cc60a to ac8c911 Nov 8, 2019
@robodoo robodoo added the CI 🤖 label Nov 8, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from ac8c911 to cd94095 Nov 9, 2019
@robodoo robodoo removed the CI 🤖 label Nov 9, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch 3 times, most recently from b57f9eb to db30adc Nov 9, 2019
@robodoo robodoo added the CI 🤖 label Nov 9, 2019
* @override
*/
start: function () {
var self = this;

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

in master (and 13 actually), we can use ES6 + async/await. So here, I'd use async/await, an arrow function, and a const (or no variable):

start: async function () {
    await this._super.apply(this, arguments);
    this._assetsChangedNotificationId = null;
    this._assets = {};
    document.querySelectorAll('*[data-asset-xmlid]').forEach(el => {
        this._assets[el.getAttribute('data-asset-xmlid')] = el.getAttribute('data-asset-version');
    });
});
* @private
*/
_displayBundleChangedNotification: function () {
var self = this;

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

same remark about es6

if (!this.assetsChangedNotificationId) {
this.assetsChangedNotificationId = this.call('notification', 'notify', {
title: _t('Update available'),
message: _t('Refresh your browser to take advantage of the latest update of Odoo in your browser'),

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

not sure about the sentence: people should not believe they will be upgraded by pressing f5, obviously, but I don't like the 'in your browser' occurring twice)

This comment has been minimized.

Copy link
@kebeclibre

kebeclibre Nov 12, 2019

Author Contributor

I'm not sure either of what the message should be to avoid mistaking this refresh with an upgrade of Odoo version
a consult of @odoongr is welcome

*
* @private
*/
_displayBundleChangedNotification: function () {

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

should be in a private section

* @param {Array} notifications: list of received notifications
*/
_onNotification: function (notifications) {
var seenXmlIds = [];

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

es6 again (use const everywhere you can, and let when you can't)

var notif = notifications[i];
if (notif[0][1] === 'bundle_changed') {
var bundleXmlId = notif[1][0];
var bundleVersion = notif[1][1];

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

I'd define bundleVersion in the if

if (notif[0][1] === 'bundle_changed') {
var bundleXmlId = notif[1][0];
var bundleVersion = notif[1][1];
if (!seenXmlIds.includes(bundleXmlId)) {

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

what's the purpose of seenXmlIds?

This comment has been minimized.

Copy link
@kebeclibre

kebeclibre Nov 12, 2019

Author Contributor

notifications contains only one of either JS or CSS asset changed but with the same xml id
So potentially, when both CSS and JS of asset_backend have changed, you'd have two notifs for one XML ID, containing with empirically the same hash
To avoid going many times in the loop, seen xml id is necessary, although it might be kinda overkill since the notification that is displayed is a singleton

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

you break as soon as you have a notification to display, so it's indeed overkill I think

{
trigger: '.o_notification_title:contains(Update available)'
},
tour.STEPS.TOGGLE_HOME_MENU, tour.STEPS.SHOW_APPS_MENU_ITEM,

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

indentation (x2)

}, {
trigger: '.o_menu_brand:contains(Settings)',
}]);
});

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Nov 12, 2019

Contributor

this file could be prettified :p

@robodoo robodoo removed the CI 🤖 label Nov 12, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 914d559 to eff5c2a Nov 12, 2019
@robodoo robodoo added the CI 🤖 label Nov 12, 2019
kebeclibre added 3 commits Nov 5, 2019
Use case:
When the server is restarted, the python is updated,
but some users may have an ongoing session in a browser tab
This may lead to code being unsynchronized and ultimately to some
odd bugs

Purpose:
When we are in such a case, notify connected users that assets have changed
And propose them to reload the page

Known caveats:
- This is not a developer's feature
Since assets computing is ORM cached, they have limited
opportunities to rebuild. Namely, the feature won't trigger
each time the JS has changed, rather, it will
when JS has changed AND the cache has been reset somehow

- This not a portal/website feature either
Business clients won't be notified that the JS has changed

- While requests debug=assets do trigger a recomputing
of the *components* of bundles, they do not save a bundle
This means that the requests that sends the notification
cannot be debug=assets

Task 2034462
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from eff5c2a to 7ff8d93 Nov 12, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 12, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 7ff8d93 to 7f74a35 Nov 12, 2019
@robodoo robodoo removed the CI 🤖 label Nov 12, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 7f74a35 to 56c73b7 Nov 12, 2019
@robodoo robodoo added the CI 🤖 label Nov 12, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from 56c73b7 to c54451b Nov 12, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 12, 2019
@kebeclibre kebeclibre force-pushed the odoo-dev:master-asset-update-lpe branch from c54451b to 2637a14 Nov 13, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.