From 74f2755843a39a4ca3912afaf35b0598270f2253 Mon Sep 17 00:00:00 2001 From: Rodrigo Nascimento Date: Wed, 6 May 2020 12:23:35 -0300 Subject: [PATCH 1/2] Regression: RegExp callbacks of settings were not being called --- app/settings/lib/settings.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/settings/lib/settings.ts b/app/settings/lib/settings.ts index f3aa1f2d0bf5..9415d7a55a0a 100644 --- a/app/settings/lib/settings.ts +++ b/app/settings/lib/settings.ts @@ -79,8 +79,7 @@ export class SettingsBase { callbacks.forEach((callback) => callback(key, value, initialLoad)); } }); - Object.keys(this.regexCallbacks).forEach((cbKey) => { - const cbValue = this.regexCallbacks.get(cbKey); + this.regexCallbacks.forEach((cbValue) => { if (!cbValue?.regex.test(key)) { return; } From 35e7f4554195570dcce880a51a7bdfb922f4ac42 Mon Sep 17 00:00:00 2001 From: Rodrigo Nascimento Date: Thu, 7 May 2020 17:17:18 -0300 Subject: [PATCH 2/2] Add tests --- app/settings/lib/settings.ts | 6 +- .../server/functions/settings.mocks.ts | 18 +++++ .../server/functions/settings.tests.ts | 71 +++++++++++++++---- app/settings/server/functions/settings.ts | 8 +-- package-lock.json | 16 ++++- package.json | 2 + 6 files changed, 96 insertions(+), 25 deletions(-) diff --git a/app/settings/lib/settings.ts b/app/settings/lib/settings.ts index 9415d7a55a0a..4bc180c99415 100644 --- a/app/settings/lib/settings.ts +++ b/app/settings/lib/settings.ts @@ -19,7 +19,7 @@ export class SettingsBase { // private ts = new Date() - public get(_id: string, callback?: SettingCallback): SettingValue | SettingComposedValue[] | void { + public get(_id: string | RegExp, callback?: SettingCallback): SettingValue | SettingComposedValue[] | void { if (callback != null) { this.onload(_id, callback); if (!Meteor.settings) { @@ -41,7 +41,9 @@ export class SettingsBase { }); } - return Meteor.settings[_id] != null && callback(_id, Meteor.settings[_id]); + if (typeof _id === 'string') { + return Meteor.settings[_id] != null && callback(_id, Meteor.settings[_id]); + } } if (!Meteor.settings) { diff --git a/app/settings/server/functions/settings.mocks.ts b/app/settings/server/functions/settings.mocks.ts index 16bad40274cb..d28383b14252 100644 --- a/app/settings/server/functions/settings.mocks.ts +++ b/app/settings/server/functions/settings.mocks.ts @@ -1,3 +1,4 @@ +import { Meteor } from 'meteor/meteor'; import mock from 'mock-require'; type Dictionary = { @@ -24,8 +25,25 @@ class SettingsClass { // console.log(query, data); this.data.set(query._id, data); + Meteor.settings[query._id] = data.value; + + // Can't import before the mock command on end of this file! + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { settings } = require('./settings'); + settings.load(query._id, data.value, !existent); + this.upsertCalls++; } + + updateValueById(id: string, value: any): void { + this.data.set(id, { ...this.data.get(id), value }); + + // Can't import before the mock command on end of this file! + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { settings } = require('./settings'); + Meteor.settings[id] = value; + settings.load(id, value, false); + } } export const Settings = new SettingsClass(); diff --git a/app/settings/server/functions/settings.tests.ts b/app/settings/server/functions/settings.tests.ts index 2cc47778628a..243fbc74a58c 100644 --- a/app/settings/server/functions/settings.tests.ts +++ b/app/settings/server/functions/settings.tests.ts @@ -1,11 +1,14 @@ /* eslint-disable @typescript-eslint/camelcase */ /* eslint-env mocha */ import { Meteor } from 'meteor/meteor'; -import { expect } from 'chai'; +import chai, { expect } from 'chai'; +import spies from 'chai-spies'; import { Settings } from './settings.mocks'; import { settings } from './settings'; +chai.use(spies); + describe('Settings', () => { beforeEach(() => { Settings.upsertCalls = 0; @@ -254,20 +257,21 @@ describe('Settings', () => { expect(Settings.upsertCalls).to.be.equal(2); expect(Settings.findOne({ _id: 'my_setting' })).to.include(expectedSetting); - settings.addGroup('group', function() { - this.section('section', function() { - this.add('my_setting', 1, { - type: 'int', - sorter: 0, - }); - }); - }); - - expectedSetting.packageValue = 1; - - expect(Settings.data.size).to.be.equal(2); - expect(Settings.upsertCalls).to.be.equal(3); - expect(Settings.findOne({ _id: 'my_setting' })).to.include(expectedSetting); + // Can't reset setting because the Meteor.setting will have the first value and will act to enforce his value + // settings.addGroup('group', function() { + // this.section('section', function() { + // this.add('my_setting', 1, { + // type: 'int', + // sorter: 0, + // }); + // }); + // }); + + // expectedSetting.packageValue = 1; + + // expect(Settings.data.size).to.be.equal(2); + // expect(Settings.upsertCalls).to.be.equal(3); + // expect(Settings.findOne({ _id: 'my_setting' })).to.include(expectedSetting); }); it('should change group and section', () => { @@ -316,4 +320,41 @@ describe('Settings', () => { expect(Settings.upsertCalls).to.be.equal(4); expect(Settings.findOne({ _id: 'my_setting' })).to.include(expectedSetting); }); + + it('should call `settings.get` callback on setting added', () => { + settings.addGroup('group', function() { + this.section('section', function() { + this.add('setting_callback', 'value1', { + type: 'string', + }); + }); + }); + + const spy = chai.spy(); + settings.get('setting_callback', spy); + settings.get(/setting_callback/, spy); + + expect(spy).to.have.been.called.exactly(2); + expect(spy).to.have.been.called.always.with('setting_callback', 'value1'); + }); + + it('should call `settings.get` callback on setting changed', () => { + const spy = chai.spy(); + settings.get('setting_callback', spy); + settings.get(/setting_callback/, spy); + + settings.addGroup('group', function() { + this.section('section', function() { + this.add('setting_callback', 'value2', { + type: 'string', + }); + }); + }); + + settings.updateById('setting_callback', 'value3'); + + expect(spy).to.have.been.called.exactly(4); + expect(spy).to.have.been.called.with('setting_callback', 'value2'); + expect(spy).to.have.been.called.with('setting_callback', 'value3'); + }); }); diff --git a/app/settings/server/functions/settings.ts b/app/settings/server/functions/settings.ts index 27a9881b8c36..4efb204c6442 100644 --- a/app/settings/server/functions/settings.ts +++ b/app/settings/server/functions/settings.ts @@ -27,11 +27,7 @@ const overrideSetting = (_id: string, value: SettingValue, options: ISettingAddO } options.processEnvValue = value; options.valueSource = 'processEnvValue'; - } else if (typeof Meteor.settings[_id] !== 'undefined') { - if (Meteor.settings[_id] == null) { - return false; - } - + } else if (Meteor.settings[_id] != null && Meteor.settings[_id] !== value) { value = Meteor.settings[_id]; options.meteorSettingsValue = value; options.valueSource = 'meteorSettingsValue'; @@ -291,7 +287,7 @@ class Settings extends SettingsBase { /* * Update a setting by id */ - updateById(_id: string, value: SettingValue, editor: string): boolean { + updateById(_id: string, value: SettingValue, editor?: string): boolean { if (!_id || value == null) { return false; } diff --git a/package-lock.json b/package-lock.json index e5dccfabcb46..b10ed4b00fcc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6053,8 +6053,15 @@ "@types/chai": { "version": "4.2.11", "resolved": "https://registry.npmjs.org/@types/chai/-/chai-4.2.11.tgz", - "integrity": "sha512-t7uW6eFafjO+qJ3BIV2gGUyZs27egcNRkUdalkud+Qa3+kg//f129iuOFivHDXQ+vnU3fDXuwgv0cqMCbcE8sw==", - "dev": true + "integrity": "sha512-t7uW6eFafjO+qJ3BIV2gGUyZs27egcNRkUdalkud+Qa3+kg//f129iuOFivHDXQ+vnU3fDXuwgv0cqMCbcE8sw==" + }, + "@types/chai-spies": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/@types/chai-spies/-/chai-spies-1.0.1.tgz", + "integrity": "sha512-vvlTzisMpzmPZaKDd0pFybCJB5bzx398JEdPsVD9Rwq4a7dcGSUDlNG2PAVhsanRBPl4hezqnlaFtL1/YwBGgw==", + "requires": { + "@types/chai": "*" + } }, "@types/color-name": { "version": "1.1.1", @@ -10451,6 +10458,11 @@ "chai": ">1.9.0" } }, + "chai-spies": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/chai-spies/-/chai-spies-1.0.0.tgz", + "integrity": "sha512-elF2ZUczBsFoP07qCfMO/zeggs8pqCf3fZGyK5+2X4AndS8jycZYID91ztD9oQ7d/0tnS963dPkd0frQEThDsg==" + }, "chalk": { "version": "1.1.3", "resolved": "https://registry.npmjs.org/chalk/-/chalk-1.1.3.tgz", diff --git a/package.json b/package.json index fc15d157f51b..160879b9dc5f 100644 --- a/package.json +++ b/package.json @@ -63,6 +63,7 @@ "@storybook/react": "^5.3.18", "@types/bcrypt": "^3.0.0", "@types/chai": "^4.2.11", + "@types/chai-spies": "^1.0.1", "@types/meteor": "^1.4.37", "@types/mocha": "^7.0.2", "@types/mock-require": "^2.0.0", @@ -78,6 +79,7 @@ "babel-polyfill": "^6.26.0", "chai": "^4.2.0", "chai-datetime": "^1.5.0", + "chai-spies": "^1.0.0", "cypress": "^4.0.2", "emojione-assets": "^4.5.0", "eslint": "^6.7.2",