Merge pull request #3456 from weseek/fix/5239-5240-save-slack-channel-when-trigger-user-notification

Fix/5239 5240 save slack channel when trigger user notification

src/server/models/page.js

@@ -46,21 +46,7 @@ const pageSchema = new mongoose.Schema({
   liker: [{ type: ObjectId, ref: 'User' }],
   seenUsers: [{ type: ObjectId, ref: 'User' }],
   commentCount: { type: Number, default: 0 },
-  extended: {
-    type: String,
-    default: '{}',
-    get(data) {
-      try {
-        return JSON.parse(data);
-      }
-      catch (e) {
-        return data;
-      }
-    },
-    set(data) {
-      return JSON.stringify(data);
-    },
-  },
+  slackChannels: { type: String },
   pageIdOnHackmd: String,
   revisionHackmdSynced: { type: ObjectId, ref: 'Revision' }, // the revision that is synced to HackMD
   hasDraftOnHackmd: { type: Boolean }, // set true if revision and revisionHackmdSynced are same but HackMD document has modified
@@ -426,33 +412,10 @@ module.exports = function(crowi) {
     return saved;
   };
 
-  pageSchema.methods.getSlackChannel = function() {
-    const extended = this.get('extended');
-    if (!extended) {
-      return '';
-    }
-
-    return extended.slack || '';
-  };
-
-  pageSchema.methods.updateSlackChannel = function(slackChannel) {
-    const extended = this.extended;
-    extended.slack = slackChannel;
-
-    return this.updateExtended(extended);
-  };
+  pageSchema.methods.updateSlackChannels = function(slackChannels) {
+    this.slackChannels = slackChannels;
 
-  pageSchema.methods.updateExtended = function(extended) {
-    const page = this;
-    page.extended = extended;
-    return new Promise(((resolve, reject) => {
-      return page.save((err, doc) => {
-        if (err) {
-          return reject(err);
-        }
-        return resolve(doc);
-      });
-    }));
+    return this.save();
   };
 
   pageSchema.methods.initLatestRevisionField = async function(revisionId) {

src/server/routes/comment.js

@@ -271,7 +271,7 @@ module.exports = function(crowi, app) {
       const user = await User.findUserByUsername(req.user.username);
       const channelsStr = slackNotificationForm.slackChannels || null;
 
-      page.updateSlackChannel(channelsStr).catch((err) => {
+      page.updateSlackChannels(channelsStr).catch((err) => {
         logger.error('Error occured in updating slack channels: ', err);
       });
 

src/server/routes/page.js

@@ -136,16 +136,16 @@ module.exports = function(crowi, app) {
   const User = crowi.model('User');
   const Bookmark = crowi.model('Bookmark');
   const PageTagRelation = crowi.model('PageTagRelation');
-  const UpdatePost = crowi.model('UpdatePost');
   const GlobalNotificationSetting = crowi.model('GlobalNotificationSetting');
   const ShareLink = crowi.model('ShareLink');
 
   const ApiResponse = require('../util/apiResponse');
   const getToday = require('../util/getToday');
 
-  const { slackNotificationService, configManager, xssService } = crowi;
+  const { configManager, xssService } = crowi;
   const interceptorManager = crowi.getInterceptorManager();
   const globalNotificationService = crowi.getGlobalNotificationService();
+  const userNotificationService = crowi.getUserNotificationService();
 
   const XssOption = require('../../lib/service/xss/xssOption');
   const Xss = require('../../lib/service/xss/index');
@@ -194,37 +194,6 @@ module.exports = function(crowi, app) {
     };
   }
 
-  // user notification
-  // TODO create '/service/user-notification' module
-  /**
-   *
-   * @param {Page} page
-   * @param {User} user
-   * @param {string} slackChannelsStr comma separated string. e.g. 'general,channel1,channel2'
-   * @param {boolean} updateOrCreate
-   * @param {string} previousRevision
-   */
-  async function notifyToSlackByUser(page, user, slackChannelsStr, updateOrCreate, previousRevision) {
-    await page.updateSlackChannel(slackChannelsStr)
-      .catch((err) => {
-        logger.error('Error occured in updating slack channels: ', err);
-      });
-
-
-    if (slackNotificationService.hasSlackConfig()) {
-      const slackChannels = slackChannelsStr != null ? slackChannelsStr.split(',') : [null];
-
-      const promises = slackChannels.map((chan) => {
-        return crowi.slack.postPage(page, user, chan, updateOrCreate, previousRevision);
-      });
-
-      Promise.all(promises)
-        .catch((err) => {
-          logger.error('Error occured in sending slack notification: ', err);
-        });
-    }
-  }
-
   function addRenderVarsForPage(renderVars, page) {
     renderVars.page = page;
     renderVars.revision = page.revision;
@@ -268,10 +237,6 @@ module.exports = function(crowi, app) {
     renderVars.grantedGroupName = page.grantedGroup ? page.grantedGroup.name : null;
   }
 
-  async function addRenderVarsForSlack(renderVars, page) {
-    renderVars.slack = await getSlackChannels(page);
-  }
-
   async function addRenderVarsForDescendants(renderVars, path, requestUser, offset, limit, isRegExpEscapedFromPath) {
     const SEENER_THRESHOLD = 10;
 
@@ -349,7 +314,6 @@ module.exports = function(crowi, app) {
     portalPage = await portalPage.populateDataToShowRevision();
 
     addRenderVarsForPage(renderVars, portalPage);
-    await addRenderVarsForSlack(renderVars, portalPage);
 
     const sharelinksNumber = await ShareLink.countDocuments({ relatedPage: portalPage._id });
     renderVars.sharelinksNumber = sharelinksNumber;
@@ -399,7 +363,6 @@ module.exports = function(crowi, app) {
     addRenderVarsForPage(renderVars, page);
     addRenderVarsForScope(renderVars, page);
 
-    await addRenderVarsForSlack(renderVars, page);
     await addRenderVarsForDescendants(renderVars, path, req.user, offset, limit, true);
 
     const sharelinksNumber = await ShareLink.countDocuments({ relatedPage: page._id });
@@ -415,16 +378,6 @@ module.exports = function(crowi, app) {
     return res.render(view, renderVars);
   }
 
-  const getSlackChannels = async(page) => {
-    if (page.extended.slack) {
-      return page.extended.slack;
-    }
-
-    const data = await UpdatePost.findSettingsByPath(page.path);
-    const channels = data.map((e) => { return e.channel }).join(', ');
-    return channels;
-  };
-
   actions.showTopPage = function(req, res) {
     return showTopPage(req, res);
   };
@@ -769,7 +722,17 @@ module.exports = function(crowi, app) {
 
     // user notification
     if (isSlackEnabled) {
-      await notifyToSlackByUser(createdPage, req.user, slackChannels, 'create', false);
+      try {
+        const results = await userNotificationService.fire(createdPage, req.user, slackChannels, 'create');
+        results.forEach((result) => {
+          if (result.status === 'rejected') {
+            logger.error('Create user notification failed', result.reason);
+          }
+        });
+      }
+      catch (err) {
+        logger.error('Create user notification failed', err);
+      }
     }
   };
 
@@ -904,7 +867,17 @@ module.exports = function(crowi, app) {
 
     // user notification
     if (isSlackEnabled) {
-      await notifyToSlackByUser(page, req.user, slackChannels, 'update', previousRevision);
+      try {
+        const results = await userNotificationService.fire(page, req.user, slackChannels, 'update', previousRevision);
+        results.forEach((result) => {
+          if (result.status === 'rejected') {
+            logger.error('Create user notification failed', result.reason);
+          }
+        });
+      }
+      catch (err) {
+        logger.error('Create user notification failed', err);
+      }
     }
   };
 

src/server/service/user-notification/index.js

@@ -19,13 +19,13 @@ class UserNotificationService {
    * @param {Page} page
    * @param {User} user
    * @param {string} slackChannelsStr comma separated string. e.g. 'general,channel1,channel2'
-   * @param {boolean} updateOrCreate
+   * @param {string} updateOrCreate 'create' or 'update'
    * @param {string} previousRevision
    */
-  async fire(page, user, slackChannelsStr, updateOrCreate, previousRevision) {
+  async fire(page, user, slackChannelsStr, updateOrCreate, previousRevision = '') {
     const { slackNotificationService, slack } = this.crowi;
 
-    await page.updateSlackChannel(slackChannelsStr);
+    await page.updateSlackChannels(slackChannelsStr);
 
     if (!slackNotificationService.hasSlackConfig()) {
       throw new Error('slackNotificationService has not been set up');

src/server/views/widget/page_content.html

@@ -18,7 +18,7 @@
   data-page-is-deletable="{% if isDeletablePage() %}true{% else %}false{% endif %}"
   data-page-is-not-creatable="false"
   data-page-is-able-to-delete-completely="{% if user.canDeleteCompletely(page.creator._id) %}true{% else %}false{% endif %}"
-  data-slack-channels="{{ slack|default('') }}"
+  data-slack-channels="{% if page %}{{ page.slackChannels }}{% endif %}"
   data-page-created-at="{% if page %}{{ page.createdAt|datetz('Y/m/d H:i:s') }}{% endif %}"
   data-page-creator="{% if page && page.creator %}{{ page.creator|json }}{% endif %}"
   data-page-last-update-username="{% if page && page.lastUpdateUser %}{{ page.lastUpdateUser.name }}{% endif %}"
@@ -36,7 +36,7 @@
 <div id="content-main" class="content-main d-flex"
   data-path="{{ encodeURI(path) }}"
   data-current-user="{% if user %}{{ user._id.toString() }}{% endif %}"
-  data-slack-channels="{{ slack|default('') }}"
+  data-slack-channels="{% if page %}{{ page.slackChannels }}{% endif %}"
   data-page-is-deleted="{% if page.isDeleted() %}true{% else %}false{% endif %}"
   data-page-has-children="{% if pages.length > 0 %}true{% else %}false{% endif %}"
   >

src/test/models/page.test.js

@@ -93,10 +93,9 @@ describe('Page', () => {
         creator: testUser0,
       },
       {
-        path: '/page/for/extended',
+        path: '/page/child/without/parents',
         grant: Page.GRANT_PUBLIC,
         creator: testUser0,
-        extended: { hoge: 1 },
       },
       {
         path: '/grant/groupacl',
@@ -266,24 +265,6 @@ describe('Page', () => {
     });
   });
 
-  describe('Extended field', () => {
-    describe('Slack Channel.', () => {
-      test('should be empty', async() => {
-        const page = await Page.findOne({ path: '/page/for/extended' });
-        expect(page.extended.hoge).toEqual(1);
-        expect(page.getSlackChannel()).toEqual('');
-      });
-
-      test('set slack channel and should get it and should keep hoge ', async() => {
-        let page = await Page.findOne({ path: '/page/for/extended' });
-        await page.updateSlackChannel('slack-channel1');
-        page = await Page.findOne({ path: '/page/for/extended' });
-        expect(page.extended.hoge).toEqual(1);
-        expect(page.getSlackChannel()).toEqual('slack-channel1');
-      });
-    });
-  });
-
   describe('.findPage', () => {
     describe('findByIdAndViewer', () => {
       test('should find page (public)', async() => {
@@ -341,7 +322,7 @@ describe('Page', () => {
       expect(result.length).toEqual(1);
       // assert paths
       const pagePaths = result.map((page) => { return page.path });
-      expect(pagePaths).toContainEqual('/page/for/extended');
+      expect(pagePaths).toContainEqual('/page/child/without/parents');
     });
 
     test('can retrieve descendants of /page1', async() => {
@@ -370,7 +351,7 @@ describe('Page', () => {
       expect(result.length).toEqual(1);
       // assert paths
       const pagePaths = result.map((page) => { return page.path });
-      expect(pagePaths).toContainEqual('/page/for/extended');
+      expect(pagePaths).toContainEqual('/page/child/without/parents');
     });
 
     test('can retrieve only descendants of /page1', async() => {
@@ -398,7 +379,7 @@ describe('Page', () => {
       expect(result.length).toEqual(4);
       // assert paths
       const pagePaths = result.map((page) => { return page.path });
-      expect(pagePaths).toContainEqual('/page/for/extended');
+      expect(pagePaths).toContainEqual('/page/child/without/parents');
       expect(pagePaths).toContainEqual('/page1');
       expect(pagePaths).toContainEqual('/page1/child1');
       expect(pagePaths).toContainEqual('/page2');