From a2888250f4ab3615c1590ab3bbf90302963a7c57 Mon Sep 17 00:00:00 2001 From: Alexander Sulfrian Date: Fri, 22 Apr 2016 00:59:05 +0200 Subject: [PATCH 1/5] Models: Replace before.insert with autoValues The before.insert hooks have the problem, that they are executed in a different order if called from the client or from the server. If called from the client, the before.insert hook is called before validation of the schema, but if called from the server, the validation is called first and fails. --- client/components/cards/labels.js | 4 +- models/boards.js | 109 ++++++++++++++++++++---------- models/cardComments.js | 17 +++-- models/cards.js | 32 +++++---- models/lists.js | 33 +++++---- models/unsavedEdits.js | 9 +-- 6 files changed, 129 insertions(+), 75 deletions(-) diff --git a/client/components/cards/labels.js b/client/components/cards/labels.js index 20d95bc61..cdd5a7008 100644 --- a/client/components/cards/labels.js +++ b/client/components/cards/labels.js @@ -9,9 +9,7 @@ BlazeComponent.extendComponent({ }, labels() { - return labelColors.map((color) => { - return { color, name: '' }; - }); + return labelColors.map((color) => ({ color, name: '' })); }, isSelected(color) { diff --git a/models/boards.js b/models/boards.js index 52272ccea..715bb8388 100644 --- a/models/boards.js +++ b/models/boards.js @@ -6,25 +6,77 @@ Boards.attachSchema(new SimpleSchema({ }, slug: { type: String, + autoValue() { // eslint-disable-line consistent-return + // XXX We need to improve slug management. Only the id should be necessary + // to identify a board in the code. + // XXX If the board title is updated, the slug should also be updated. + // In some cases (Chinese and Japanese for instance) the `getSlug` function + // return an empty string. This is causes bugs in our application so we set + // a default slug in this case. + if (this.isInsert && !this.isSet) { + let slug = 'board'; + const title = this.field('title'); + if (title.isSet) { + slug = getSlug(title.value) || slug; + } + return slug; + } + }, }, archived: { type: Boolean, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return false; + } + }, }, createdAt: { type: Date, - denyUpdate: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert) { + return new Date(); + } else { + this.unset(); + } + }, }, // XXX Inconsistent field naming modifiedAt: { type: Date, - denyInsert: true, optional: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isUpdate) { + return new Date(); + } else { + this.unset(); + } + }, }, // De-normalized number of users that have starred this board stars: { type: Number, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert) { + return 0; + } + }, }, // De-normalized label system + 'labels': { + type: [Object], + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + const colors = Boards.simpleSchema()._schema['labels.$.color'].allowedValues; + const defaultLabelsColors = _.clone(colors).splice(0, 6); + return defaultLabelsColors.map((color) => ({ + color, + _id: Random.id(6), + name: '', + })); + } + }, + }, 'labels.$._id': { // We don't specify that this field must be unique in the board because that // will cause performance penalties and is not necessary since this field is @@ -47,6 +99,19 @@ Boards.attachSchema(new SimpleSchema({ // XXX We might want to maintain more informations under the member sub- // documents like de-normalized meta-data (the date the member joined the // board, the number of contributions, etc.). + 'members': { + type: [Object], + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return [{ + userId: this.userId, + isAdmin: true, + isActive: true, + isInvited: false, + }]; + } + }, + }, 'members.$.userId': { type: String, }, @@ -70,6 +135,11 @@ Boards.attachSchema(new SimpleSchema({ 'wisteria', 'midnight', ], + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return Boards.simpleSchema()._schema.color.allowedValues[0]; + } + }, }, description: { type: String, @@ -338,41 +408,6 @@ if (Meteor.isServer) { }); } -Boards.before.insert((userId, doc) => { - // XXX We need to improve slug management. Only the id should be necessary - // to identify a board in the code. - // XXX If the board title is updated, the slug should also be updated. - // In some cases (Chinese and Japanese for instance) the `getSlug` function - // return an empty string. This is causes bugs in our application so we set - // a default slug in this case. - doc.slug = doc.slug || getSlug(doc.title) || 'board'; - doc.createdAt = new Date(); - doc.archived = false; - doc.members = doc.members || [{ - userId, - isAdmin: true, - isActive: true, - }]; - doc.stars = 0; - doc.color = Boards.simpleSchema()._schema.color.allowedValues[0]; - - // Handle labels - const colors = Boards.simpleSchema()._schema['labels.$.color'].allowedValues; - const defaultLabelsColors = _.clone(colors).splice(0, 6); - doc.labels = defaultLabelsColors.map((color) => { - return { - color, - _id: Random.id(6), - name: '', - }; - }); -}); - -Boards.before.update((userId, doc, fieldNames, modifier) => { - modifier.$set = modifier.$set || {}; - modifier.$set.modifiedAt = new Date(); -}); - if (Meteor.isServer) { // Let MongoDB ensure that a member is not included twice in the same board Meteor.startup(() => { diff --git a/models/cardComments.js b/models/cardComments.js index 224deb037..ce6edf3c0 100644 --- a/models/cardComments.js +++ b/models/cardComments.js @@ -16,10 +16,22 @@ CardComments.attachSchema(new SimpleSchema({ createdAt: { type: Date, denyUpdate: false, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert) { + return new Date(); + } else { + this.unset(); + } + }, }, // XXX Should probably be called `authorId` userId: { type: String, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return this.userId; + } + }, }, })); @@ -44,11 +56,6 @@ CardComments.helpers({ CardComments.hookOptions.after.update = { fetchPrevious: false }; -CardComments.before.insert((userId, doc) => { - doc.createdAt = new Date(); - doc.userId = userId; -}); - if (Meteor.isServer) { CardComments.after.insert((userId, doc) => { Activities.insert({ diff --git a/models/cards.js b/models/cards.js index 09c861917..aa19a64a2 100644 --- a/models/cards.js +++ b/models/cards.js @@ -9,6 +9,11 @@ Cards.attachSchema(new SimpleSchema({ }, archived: { type: Boolean, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return false; + } + }, }, listId: { type: String, @@ -25,10 +30,19 @@ Cards.attachSchema(new SimpleSchema({ }, createdAt: { type: Date, - denyUpdate: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert) { + return new Date(); + } else { + this.unset(); + } + }, }, dateLastActivity: { type: Date, + autoValue() { + return new Date(); + }, }, description: { type: String, @@ -46,6 +60,11 @@ Cards.attachSchema(new SimpleSchema({ // the `members` field? userId: { type: String, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return this.userId; + } + }, }, sort: { type: Number, @@ -190,17 +209,6 @@ Cards.mutations({ }, }); -Cards.before.insert((userId, doc) => { - doc.createdAt = new Date(); - doc.dateLastActivity = new Date(); - if(!doc.hasOwnProperty('archived')){ - doc.archived = false; - } - if (!doc.userId) { - doc.userId = userId; - } -}); - if (Meteor.isServer) { Cards.after.insert((userId, doc) => { Activities.insert({ diff --git a/models/lists.js b/models/lists.js index 4e4a11342..a4938f672 100644 --- a/models/lists.js +++ b/models/lists.js @@ -6,13 +6,24 @@ Lists.attachSchema(new SimpleSchema({ }, archived: { type: Boolean, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return false; + } + }, }, boardId: { type: String, }, createdAt: { type: Date, - denyUpdate: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert) { + return new Date(); + } else { + this.unset(); + } + }, }, sort: { type: Number, @@ -22,8 +33,14 @@ Lists.attachSchema(new SimpleSchema({ }, updatedAt: { type: Date, - denyInsert: true, optional: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isUpdate) { + return new Date(); + } else { + this.unset(); + } + }, }, })); @@ -73,18 +90,6 @@ Lists.mutations({ Lists.hookOptions.after.update = { fetchPrevious: false }; -Lists.before.insert((userId, doc) => { - doc.createdAt = new Date(); - doc.archived = false; - if (!doc.userId) - doc.userId = userId; -}); - -Lists.before.update((userId, doc, fieldNames, modifier) => { - modifier.$set = modifier.$set || {}; - modifier.$set.modifiedAt = new Date(); -}); - if (Meteor.isServer) { Lists.after.insert((userId, doc) => { Activities.insert({ diff --git a/models/unsavedEdits.js b/models/unsavedEdits.js index 87a70e22b..25952fb58 100644 --- a/models/unsavedEdits.js +++ b/models/unsavedEdits.js @@ -14,6 +14,11 @@ UnsavedEditCollection.attachSchema(new SimpleSchema({ }, userId: { type: String, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return this.userId; + } + }, }, })); @@ -28,7 +33,3 @@ if (Meteor.isServer) { fetch: ['userId'], }); } - -UnsavedEditCollection.before.insert((userId, doc) => { - doc.userId = userId; -}); From 18b1573b25b5f387c42071d2aca0ff8b78f54a3e Mon Sep 17 00:00:00 2001 From: Alexander Sulfrian Date: Tue, 31 May 2016 20:03:48 +0200 Subject: [PATCH 2/5] Meteor.users: Add SimpleSchema Replace before.insert hook with SimpleSchema and autoValue. --- models/users.js | 98 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 8 deletions(-) diff --git a/models/users.js b/models/users.js index 4e4a0faca..5641ec9b5 100644 --- a/models/users.js +++ b/models/users.js @@ -1,5 +1,95 @@ Users = Meteor.users; +Users.attachSchema(new SimpleSchema({ + username: { + type: String, + optional: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + const name = this.field('profile.fullname'); + if (name.isSet) { + return name.value.toLowerCase().replace(/\s/g, ''); + } + } + }, + }, + emails: { + type: [Object], + optional: true, + }, + 'emails.$.address': { + type: String, + regEx: SimpleSchema.RegEx.Email, + }, + 'emails.$.verified': { + type: Boolean, + }, + createdAt: { + type: Date, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert) { + return new Date(); + } else { + this.unset(); + } + }, + }, + profile: { + type: Object, + optional: true, + autoValue() { // eslint-disable-line consistent-return + if (this.isInsert && !this.isSet) { + return {}; + } + }, + }, + 'profile.avatarUrl': { + type: String, + optional: true, + }, + 'profile.emailBuffer': { + type: [String], + optional: true, + }, + 'profile.fullname': { + type: String, + optional: true, + }, + 'profile.initials': { + type: String, + optional: true, + }, + 'profile.invitedBoards': { + type: [String], + optional: true, + }, + 'profile.language': { + type: String, + optional: true, + }, + 'profile.notifications': { + type: [String], + optional: true, + }, + 'profile.starredBoards': { + type: [String], + optional: true, + }, + 'profile.tags': { + type: [String], + optional: true, + }, + services: { + type: Object, + optional: true, + blackbox: true, + }, + heartbeat: { + type: Date, + optional: true, + }, +})); + // Search a user in the complete server database by its name or username. This // is used for instance to add a new user to a board. const searchInFields = ['username', 'profile.fullname']; @@ -259,14 +349,6 @@ if (Meteor.isServer) { }); } -Users.before.insert((userId, doc) => { - doc.profile = doc.profile || {}; - - if (!doc.username && doc.profile.name) { - doc.username = doc.profile.name.toLowerCase().replace(/\s/g, ''); - } -}); - if (Meteor.isServer) { // Let mongoDB ensure username unicity Meteor.startup(() => { From b9883a8e244d21f7caf00a794371dbe6b2e4e2ad Mon Sep 17 00:00:00 2001 From: Alexander Sulfrian Date: Tue, 31 May 2016 21:32:08 +0200 Subject: [PATCH 3/5] Remove duplicated logic The duplicated logic was nessessary because the before.insert hook was not called before validation, when inserting was initiated on the server. Using autoValues fixed this problem. --- models/users.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/models/users.js b/models/users.js index 5641ec9b5..b56fd2aba 100644 --- a/models/users.js +++ b/models/users.js @@ -404,13 +404,6 @@ if (Meteor.isServer) { title, boardId, userId: ExampleBoard.userId, - - // XXX Not certain this is a bug, but we except these fields get - // inserted by the Lists.before.insert collection-hook. Since this - // hook is not called in this case, we have to dublicate the logic and - // set them here. - archived: false, - createdAt: new Date(), }; Lists.insert(list); From ae2c1fb77f498e0c92cbcb0a48fe6e7ecc9c14c4 Mon Sep 17 00:00:00 2001 From: Alexander Sulfrian Date: Tue, 29 Mar 2016 18:36:16 +0200 Subject: [PATCH 4/5] Fix initial board creation We cannot rely on the automatic userId setting of the collection hooks. If a user is created during invitation, the userId field will contain the id of the inviting user. This fix this, by mocking the CollectionHooks.getUserId function and returning the userId of the new user for all new documents after creating the user. --- .eslintrc.json | 1 + models/users.js | 32 +++++++++++++++++++------------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 939f7b46d..87c2e2cf1 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -74,6 +74,7 @@ "Avatars": true, "BlazeComponent": false, "BlazeLayout": false, + "CollectionHooks": false, "DocHead": false, "ESSearchResults": false, "FastRender": false, diff --git a/models/users.js b/models/users.js index b56fd2aba..629caa4ab 100644 --- a/models/users.js +++ b/models/users.js @@ -388,25 +388,31 @@ if (Meteor.isServer) { incrementBoards(_.difference(newIds, oldIds), +1); }); + const fakeUserId = new Meteor.EnvironmentVariable(); + const getUserId = CollectionHooks.getUserId; + CollectionHooks.getUserId = () => { + return fakeUserId.get() || getUserId(); + }; + // XXX i18n Users.after.insert((userId, doc) => { - const ExampleBoard = { - title: 'Welcome Board', - userId: doc._id, - permission: 'private', + const fakeUser = { + extendAutoValueContext: { + userId: doc._id, + }, }; - // Insert the Welcome Board - Boards.insert(ExampleBoard, (err, boardId) => { + fakeUserId.withValue(doc._id, () => { + // Insert the Welcome Board + Boards.insert({ + title: 'Welcome Board', + permission: 'private', + }, fakeUser, (err, boardId) => { - ['Basics', 'Advanced'].forEach((title) => { - const list = { - title, - boardId, - userId: ExampleBoard.userId, - }; + ['Basics', 'Advanced'].forEach((title) => { + Lists.insert({ title, boardId }, fakeUser); + }); - Lists.insert(list); }); }); }); From e521fe617ee923f72d5d6ca27cc03147d353eec4 Mon Sep 17 00:00:00 2001 From: Alexander Sulfrian Date: Tue, 31 May 2016 21:20:25 +0200 Subject: [PATCH 5/5] Welcome board: Allow localization --- i18n/en.i18n.json | 3 +++ models/users.js | 7 +++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/i18n/en.i18n.json b/i18n/en.i18n.json index b171bdf93..2047d8e14 100755 --- a/i18n/en.i18n.json +++ b/i18n/en.i18n.json @@ -294,5 +294,8 @@ "watch": "Watch", "watching": "Watching", "watching-info": "You will be notified of any change in this board", + "welcome-board": "Welcome Board", + "welcome-list1": "Basics", + "welcome-list2": "Advanced", "what-to-do": "What do you want to do?" } diff --git a/models/users.js b/models/users.js index 629caa4ab..a3bdb6f8b 100644 --- a/models/users.js +++ b/models/users.js @@ -394,7 +394,6 @@ if (Meteor.isServer) { return fakeUserId.get() || getUserId(); }; - // XXX i18n Users.after.insert((userId, doc) => { const fakeUser = { extendAutoValueContext: { @@ -405,12 +404,12 @@ if (Meteor.isServer) { fakeUserId.withValue(doc._id, () => { // Insert the Welcome Board Boards.insert({ - title: 'Welcome Board', + title: TAPi18n.__('welcome-board'), permission: 'private', }, fakeUser, (err, boardId) => { - ['Basics', 'Advanced'].forEach((title) => { - Lists.insert({ title, boardId }, fakeUser); + ['welcome-list1', 'welcome-list2'].forEach((title) => { + Lists.insert({ title: TAPi18n.__(title), boardId }, fakeUser); }); });