Repository: Yoctol/bottender
Pull request #377 (2018-11-21 07:00:26)

implement skipProfile for line
If the user profile is not necessary in the bot responding logic, we can skip the profile updating and have a better responding experience.

With this flag setted true, skiping all the getUserProfile api calls when session updating, providing better performance when dealing heavy traffic.

codecov-io: # [Codecov](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=h1) Report
> Merging [#377](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=desc) into [master](https://codecov.io/gh/Yoctol/bottender/commit/3cd76869629580a0ac89512926e6796096cb85ca?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/Yoctol/bottender/pull/377/graphs/tree.svg?width=650&token=JX3EGtxYH3&height=150&src=pr)](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
+ Coverage   89.89%   89.91%   +0.01%     
==========================================
  Files         116      116              
  Lines        3465     3469       +4     
  Branches      783      786       +3     
==========================================
+ Hits         3115     3119       +4     
  Misses        310      310              
  Partials       40       40
```


| [Impacted Files](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/bot/LineBot.js](https://codecov.io/gh/Yoctol/bottender/pull/377/diff?src=pr&el=tree#diff-c3JjL2JvdC9MaW5lQm90Lmpz) | `100% <ø> (ø)` | :arrow_up: |
| [src/bot/LineConnector.js](https://codecov.io/gh/Yoctol/bottender/pull/377/diff?src=pr&el=tree#diff-c3JjL2JvdC9MaW5lQ29ubmVjdG9yLmpz) | `94.73% <100%> (+0.29%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=footer). Last update [3cd7686...6b794b2](https://codecov.io/gh/Yoctol/bottender/pull/377?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

tw0517tw: Also need to skip profile when `source.type === 'group'` and `source.type === 'room'`.

Patch:
diff --git a/src/bot/LineBot.js b/src/bot/LineBot.js
index b54c5374..f4120905 100644
--- a/src/bot/LineBot.js
+++ b/src/bot/LineBot.js
@@ -14,6 +14,7 @@ export default class LineBot extends Bot {
     sync,
     shouldBatch,
     sendMethod,
+    skipProfile,
   }: {
     accessToken: string,
     channelSecret: string,
@@ -22,6 +23,7 @@ export default class LineBot extends Bot {
     shouldBatch: ?boolean,
     sendMethod: ?string,
     origin?: string,
+    skipProfile?: ?boolean,
   }) {
     const connector = new LineConnector({
       accessToken,
@@ -29,6 +31,7 @@ export default class LineBot extends Bot {
       shouldBatch,
       sendMethod,
       origin,
+      skipProfile,
     });
     super({ connector, sessionStore, sync });
   }
diff --git a/src/bot/LineConnector.js b/src/bot/LineConnector.js
index 50570b2b..c1eb52bc 100644
--- a/src/bot/LineConnector.js
+++ b/src/bot/LineConnector.js
@@ -22,6 +22,7 @@ type ConstructorOptions = {|
   shouldBatch: ?boolean,
   sendMethod: ?string,
   origin?: string,
+  skipProfile?: ?boolean,
 |};
 
 export default class LineConnector implements Connector<LineRequestBody> {
@@ -29,6 +30,8 @@ export default class LineConnector implements Connector<LineRequestBody> {
 
   _channelSecret: string;
 
+  _skipProfile: boolean;
+
   _shouldBatch: ?boolean;
 
   _sendMethod: ?string;
@@ -40,6 +43,7 @@ export default class LineConnector implements Connector<LineRequestBody> {
     shouldBatch,
     sendMethod,
     origin,
+    skipProfile,
   }: ConstructorOptions) {
     this._client =
       client ||
@@ -54,6 +58,9 @@ export default class LineConnector implements Connector<LineRequestBody> {
       'sendMethod should be one of `reply` or `push`'
     );
     this._sendMethod = sendMethod || 'push';
+
+    // FIXME: maybe set this default value as true
+    this._skipProfile = typeof skipProfile === 'boolean' ? skipProfile : false;
   }
 
   _isWebhookVerifyEvent(event: LineRawEvent): boolean {
@@ -108,14 +115,19 @@ export default class LineConnector implements Connector<LineRequestBody> {
       let user = null;
 
       if (source.userId) {
-        user = {
-          id: source.userId,
-          _updatedAt: new Date().toISOString(),
-          ...(await this._client.getGroupMemberProfile(
-            source.groupId,
-            source.userId
-          )),
-        };
+        user = this._skipProfile
+          ? {
+              id: source.userId,
+              _updatedAt: new Date().toISOString(),
+            }
+          : {
+              id: source.userId,
+              _updatedAt: new Date().toISOString(),
+              ...(await this._client.getGroupMemberProfile(
+                source.groupId,
+                source.userId
+              )),
+            };
       }
 
       session.user = user;
@@ -139,14 +151,19 @@ export default class LineConnector implements Connector<LineRequestBody> {
       let user = null;
 
       if (source.userId) {
-        user = {
-          id: source.userId,
-          _updatedAt: new Date().toISOString(),
-          ...(await this._client.getRoomMemberProfile(
-            source.roomId,
-            source.userId
-          )),
-        };
+        user = this._skipProfile
+          ? {
+              id: source.userId,
+              _updatedAt: new Date().toISOString(),
+            }
+          : {
+              id: source.userId,
+              _updatedAt: new Date().toISOString(),
+              ...(await this._client.getRoomMemberProfile(
+                source.roomId,
+                source.userId
+              )),
+            };
       }
 
       session.user = user;
@@ -168,7 +185,10 @@ export default class LineConnector implements Connector<LineRequestBody> {
       };
     } else if (source.type === 'user') {
       if (!session.user) {
-        const user = await this._client.getUserProfile(source.userId);
+        const user = this._skipProfile
+          ? {}
+          : await this._client.getUserProfile(source.userId);
+
         session.user = {
           id: source.userId,
           _updatedAt: new Date().toISOString(),


Test patch:
diff --git a/src/bot/__tests__/LineConnector.spec.js b/src/bot/__tests__/LineConnector.spec.js
index 797a65c7..04873ab4 100644
--- a/src/bot/__tests__/LineConnector.spec.js
+++ b/src/bot/__tests__/LineConnector.spec.js
@@ -81,7 +81,7 @@ const webhookVerifyRequest = {
   },
 };
 
-function setup({ sendMethod } = {}) {
+function setup({ sendMethod, skipProfile } = {}) {
   const mockLineAPIClient = {
     getUserProfile: jest.fn(),
     isValidSignature: jest.fn(),
@@ -98,6 +98,7 @@ function setup({ sendMethod } = {}) {
       accessToken: ACCESS_TOKEN,
       channelSecret: CHANNEL_SECRET,
       sendMethod,
+      skipProfile,
     }),
   };
 }
@@ -563,6 +564,30 @@ describe('#updateSession', () => {
     });
   });
 
+  it('update userId without calling any api while skipProfile setted true', async () => {
+    const { connector, mockLineAPIClient } = setup({ skipProfile: true });
+    const session = {};
+
+    await connector.updateSession(session, request.body);
+
+    expect(mockLineAPIClient.getUserProfile).not.toBeCalled();
+
+    expect(session).toEqual({
+      type: 'user',
+      user: {
+        id: 'U206d25c2ea6bd87c17655609a1c37cb8',
+        _updatedAt: expect.any(String),
+      },
+    });
+    expect(Object.isFrozen(session.user)).toBe(true);
+    expect(Object.getOwnPropertyDescriptor(session, 'user')).toEqual({
+      configurable: false,
+      enumerable: true,
+      writable: false,
+      value: session.user,
+    });
+  });
+
   it('do nothing with webhook verify request', async () => {
     const { connector } = setup();