From 8dd045af75fa2370875cb29a73c6b3251ab84761 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Mon, 31 Jan 2022 10:50:48 +0100 Subject: [PATCH] Maintenance: Fix difference function to compare both ways and also make sure that the observer copies hashes to prevent references. --- .../_application_controller/observer.coffee | 42 ++-- .../controllers/organization_profile.coffee | 2 +- .../article_action/internal.coffee | 2 +- .../ticket_zoom/article_view.coffee | 10 +- .../app/controllers/ticket_zoom/title.coffee | 2 +- .../app/controllers/user_profile.coffee | 2 +- app/assets/javascripts/application.js | 10 +- .../assets/tests/qunit/controller_observer.js | 202 ++++++++++++++++++ public/assets/tests/qunit/core.js | 130 ++++++++--- 9 files changed, 342 insertions(+), 60 deletions(-) create mode 100644 public/assets/tests/qunit/controller_observer.js diff --git a/app/assets/javascripts/app/controllers/_application_controller/observer.coffee b/app/assets/javascripts/app/controllers/_application_controller/observer.coffee index 293c02528..cc9a34a64 100644 --- a/app/assets/javascripts/app/controllers/_application_controller/observer.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller/observer.coffee @@ -2,6 +2,7 @@ class App.ControllerObserver extends App.Controller model: 'Ticket' template: 'tba' globalRerender: true + lastAttributes: undefined ### observe: @@ -25,7 +26,7 @@ class App.ControllerObserver extends App.Controller # rerender, e. g. on language change if @globalRerender @controllerBind('ui:rerender', => - @lastAttributres = undefined + @lastAttributes = undefined @maybeRender(App[@model].fullLocal(@object_id)) ) @@ -43,32 +44,39 @@ class App.ControllerObserver extends App.Controller if !@subscribeId @subscribeId = object.subscribe(@subscribe) - # remember current attributes + return if !@hasChanged(object) + + @render(object) + + hasChanged: (object) => currentAttributes = {} + if @observe for key, active of @observe - if active - currentAttributes[key] = object[key] + if active && !_.isFunction(value) + currentAttributes[key] = clone(object[key]) + if @observeNot for key, value of object - if key isnt 'cid' && !@observeNot[key] && !_.isFunction(value) && !_.isObject(value) - currentAttributes[key] = value + if key isnt 'cid' && !@observeNot[key] && !_.isFunction(value) + currentAttributes[key] = clone(value) - if !@lastAttributres - @lastAttributres = {} - else - diff = difference(currentAttributes, @lastAttributres) - if _.isEmpty(diff) - @log 'debug', 'maybeRender no diff, no rerender' - return + if !@lastAttributes + @lastAttributes = currentAttributes + return true + + diff = difference(currentAttributes, @lastAttributes) + if _.isEmpty(diff) + @log 'debug', 'maybeRender no diff, no rerender' + return false @log 'debug', 'maybeRender.diff', diff, @observe, @model - @lastAttributres = currentAttributes + @lastAttributes = currentAttributes - @render(object, diff) + true - render: (object, diff) => - @log 'debug', 'render', @template, object, diff + render: (object) => + @log 'debug', 'render', @template, object @html App.view(@template)( object: object ) diff --git a/app/assets/javascripts/app/controllers/organization_profile.coffee b/app/assets/javascripts/app/controllers/organization_profile.coffee index 90ee9a602..315e7951b 100644 --- a/app/assets/javascripts/app/controllers/organization_profile.coffee +++ b/app/assets/javascripts/app/controllers/organization_profile.coffee @@ -202,7 +202,7 @@ class Object extends App.ControllerObserver value = $(e.target).html() org = App.Organization.find(@object_id) if org[name] isnt value - @lastAttributres[name] = value + @lastAttributes[name] = value data = {} data[name] = value org.updateAttributes(data) diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/article_action/internal.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/article_action/internal.coffee index f88ec8354..c2760699f 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/article_action/internal.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/article_action/internal.coffee @@ -39,7 +39,7 @@ class Internal internal = true if article.internal == true internal = false - ui.lastAttributres.internal = internal + ui.lastAttributes.internal = internal article.updateAttributes(internal: internal) # runtime update diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/article_view.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/article_view.coffee index 67f21134f..cbe38bed3 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/article_view.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/article_view.coffee @@ -218,11 +218,11 @@ class ArticleViewItem extends App.ControllerObserver ) @articleActions = new App.TicketZoomArticleActions( - el: @$('.js-article-actions') - ticket: @ticket - article: article - lastAttributres: @lastAttributres - form_id: @form_id + el: @$('.js-article-actions') + ticket: @ticket + article: article + lastAttributes: @lastAttributes + form_id: @form_id ) # set see more diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/title.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/title.coffee index a9acea2ea..3555967a5 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/title.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/title.coffee @@ -19,7 +19,7 @@ class App.TicketZoomTitle extends App.ControllerObserver title = $(e.target).ceg() || '' # update title - return if title is @lastAttributres.title + return if title is @lastAttributes.title ticket = App.Ticket.find(@object_id) ticket.title = title diff --git a/app/assets/javascripts/app/controllers/user_profile.coffee b/app/assets/javascripts/app/controllers/user_profile.coffee index 38a1503ae..fef15587f 100644 --- a/app/assets/javascripts/app/controllers/user_profile.coffee +++ b/app/assets/javascripts/app/controllers/user_profile.coffee @@ -220,7 +220,7 @@ class Object extends App.ControllerObserver value = $(e.target).html() user = App.User.find(@object_id) if user[name] isnt value - @lastAttributres[name] = value + @lastAttributes[name] = value data = {} data[name] = value user.updateAttributes(data) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index bdb48e09b..58459d186 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -70,9 +70,9 @@ Date.prototype.getWeek = function() { function difference(object1, object2) { var changes = {}; - for (var name in object1) { - if (name in object2) { - if (_.isObject(object2[name]) && !_.isArray(object2[name])) { + _.uniq(Object.keys(object1).concat(Object.keys(object2))).forEach(function(name) { + if (name in object1 && name in object2) { + if (_.isObject(object1[name]) && !_.isArray(object1[name]) && _.isObject(object2[name]) && !_.isArray(object2[name])) { var diff = difference(object1[name], object2[name]); if (!_.isEmpty(diff)) { changes[name] = diff; @@ -80,8 +80,10 @@ function difference(object1, object2) { } else if (!_.isEqual(object1[name], object2[name])) { changes[name] = object2[name]; } + } else { + changes[name] = object2[name] } - } + }) return changes; } diff --git a/public/assets/tests/qunit/controller_observer.js b/public/assets/tests/qunit/controller_observer.js new file mode 100644 index 000000000..1f9b4878a --- /dev/null +++ b/public/assets/tests/qunit/controller_observer.js @@ -0,0 +1,202 @@ +QUnit.test( "controller observer tests - observe", assert => { + + App.Ticket.refresh([{ + id: 1, + title: 'ticket', + state_id: 1, + customer_id: 33, + organization_id: 1, + owner_id: 1, + preferences: { a: 1, b: 2 }, + }]) + + var observer1 = new App.ControllerObserver({ + object_id: 1, + template: 'version', + observe: { + title: true, + preferences: true, + }, + }) + + var ticket = App.Ticket.find(1) + + assert.equal(false, observer1.hasChanged(ticket)) + + // track title changes + ticket.title = 'title 2' + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.title = undefined + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.title = 'title 3' + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track no owner_id changes + ticket.owner_id = 2 + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track preferences changes + ticket.preferences['a'] = 3 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.preferences['c'] = 3 + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track no new_attribute1 changes + ticket.new_attribute1 = 'na 3' + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.new_attribute2 = function() { console.log(1) } + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.new_attribute2 = function() { console.log(2) } + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track title changes + ticket.title = function() { console.log(1) } + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.title = function() { console.log(2) } + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.title = 1 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + +}); + +QUnit.test( "controller observer tests - observeNot", assert => { + + App.Ticket.refresh([{ + id: 2, + title: 'ticket', + state_id: 1, + customer_id: 33, + organization_id: 1, + owner_id: 1, + preferences: { a: 1, b: 2 }, + }]) + + var observer1 = new App.ControllerObserver({ + object_id: 2, + template: 'version', + observeNot: { + title: true, + preferences: true, + }, + }) + + var ticket = App.Ticket.find(2) + + assert.equal(false, observer1.hasChanged(ticket)) + + // track no title changes + ticket.title = 'title 2' + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track owner_id changes + ticket.owner_id = 2 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.owner_id = undefined + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.owner_id = 3 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track no preferences changes + ticket.preferences['a'] = 3 + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.preferences['c'] = 3 + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track preferences2 changes + ticket.preferences2 = {} + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.preferences2['a'] = 3 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.preferences2['a'] = 2 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.preferences2['c'] = 3 + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track new_attribute1 changes + ticket.new_attribute1 = 'na 3' + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track no new_attribute2 changes (because of function content) + ticket.new_attribute2 = function() { console.log(1) } + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.new_attribute2 = function() { console.log(2) } + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + // track owner_id changes (pnly if content has no function content) + ticket.owner_id = function() { console.log(1) } + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.owner_id = function() { console.log(2) } + + assert.equal(false, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + + ticket.owner_id = 1 + + assert.equal(true, observer1.hasChanged(ticket)) + assert.equal(false, observer1.hasChanged(ticket)) + +}); diff --git a/public/assets/tests/qunit/core.js b/public/assets/tests/qunit/core.js index dad19c095..5d05dfc8c 100644 --- a/public/assets/tests/qunit/core.js +++ b/public/assets/tests/qunit/core.js @@ -538,7 +538,9 @@ QUnit.test('difference', assert => { object2 = { key1: 123, } - result = {} + result = { + key2: undefined + } item = difference(object1, object2) assert.deepEqual(item, result) @@ -549,38 +551,106 @@ QUnit.test('difference', assert => { key1: 123, key2: 124 } + result = { + key2: 124 + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + organization_id: 2, + } + object2 = { + customer_id: 1, + organization_id: null, + } + result = { + organization_id: null, + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + organization_id: null, + } + object2 = { + customer_id: 1, + organization_id: 2, + } + result = { + organization_id: 2, + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + preferences: { resolved: true }, + } + object2 = { + customer_id: 1, + preferences: {}, + } + result = { + preferences: { resolved: undefined } + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + } + object2 = { + customer_id: 1, + preferences: { resolved: true }, + } + result = { + preferences: { resolved: true } + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + preferences: {}, + } + object2 = { + customer_id: 1, + preferences: { resolved: true }, + } + result = { + preferences: { resolved: true } + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + preferences: { resolved: false }, + } + object2 = { + customer_id: 1, + preferences: { resolved: true }, + } + result = { + preferences: { resolved: true } + } + item = difference(object1, object2) + assert.deepEqual(item, result) + + object1 = { + customer_id: 1, + preferences: { resolved: true }, + } + object2 = { + customer_id: 1, + preferences: { resolved: true }, + } result = {} item = difference(object1, object2) assert.deepEqual(item, result) - - object1 = { - customer_id: 1, - organization_id: 2, - } - object2 = { - customer_id: 1, - organization_id: null, - } - result = { - organization_id: null, - } - item = difference(object1, object2) - assert.deepEqual(item, result) - - object1 = { - customer_id: 1, - organization_id: null, - } - object2 = { - customer_id: 1, - organization_id: 2, - } - result = { - organization_id: 2, - } - item = difference(object1, object2) - assert.deepEqual(item, result) - }); QUnit.test('auth - not existing user', assert => {