Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cannot recognize room event #1116

Merged
merged 10 commits into from
Mar 11, 2018
Merged

cannot recognize room event #1116

merged 10 commits into from
Mar 11, 2018

Conversation

lijiarui
Copy link
Member

@lijiarui lijiarui commented Mar 6, 2018

Sometimes, when a person joins in the room by the qrcode shared by bot's, the sentence as follows:

image

So, I change this to make this more compatibility.

@huan
Copy link
Member

huan commented Mar 6, 2018

Please add the related unit test for before I could review/merge your PR.

The following steps are my suggestion:

  1. provide an example which current version of Wechaty is not supported(as your snapshot, but should in text description format);
  2. add a new unit test from your example, which should fail under the current version of Wechaty;
  3. add your pr and make your unit test passed.

@lijiarui
Copy link
Member Author

lijiarui commented Mar 7, 2018

WECHATY VERSION

[email protected] doctor /Users/jiaruili/git/rui/wechaty
npm run check-node-version && ts-node bin/doctor

[email protected] check-node-version /Users/jiaruili/git/rui/wechaty
check-node-version --node ">= 8.5"

Wechaty Doctor

  1. Wechaty version: #git[b5c0c14]
  2. Darwin x64 version 16.7.0 memory 456/16384 MB
  3. Docker: false
  4. Node version: v9.3.0
  5. Tcp IPC TEST: PASS

REPRODUCE

run WECHATY_LOG=silly ts-node example/room-bot.ts
code in example/room-bot

1. Bot invite someone into the group.

Log as follows:

12:35:05 VERB PuppetWebFirer checkRoomJoin(你邀请"桔小秘"加入了群聊  )
12:35:05 SILL PuppetWebFirer fireRoomJoin() "你邀请"桔小秘"加入了群聊  " is not a join message
12:35:05 VERB PuppetWebFirer fireRoomLeave(你邀请"桔小秘"加入了群聊  )
12:35:05 SILL Message ready()
12:35:05 SILL Room ready()
12:35:05 SILL Room ready()
12:35:05 VERB Room topic()
[测试群]<测试群>:{SYS}你邀请"桔小秘"加入了群聊

Actually the room join system log is 你邀请"桔小秘"加入了群聊 撤销, but 撤销 is an HTML link, so it cannot list on the silly log, I don't suggest to add too many strict limits on the room join fire condition, because it always changes a lot, so I think just keep it fuzzy matching is enough.

2. People join the group by the qrcode shared by the bot.

Log as follows:

12:43:46 VERB PuppetWebFirer checkRoomJoin("李佳芮"通过扫描你分享的二维码加入群聊  )
12:43:46 SILL PuppetWebFirer fireRoomJoin() ""李佳芮"通过扫描你分享的二维码加入群聊  " is not a join message
12:43:46 VERB PuppetWebFirer fireRoomLeave("李佳芮"通过扫描你分享的二维码加入群聊  )
12:43:46 SILL Message ready()
12:43:46 SILL Room ready()
12:43:46 SILL Room ready()
12:43:46 VERB Room topic()
[测试aaa]<测试aaa>:{SYS}"李佳芮"通过扫描你分享的二维码加入群聊

Actually the room join system log is "李佳芮"通过扫描你分享的二维码加入群聊 撤销, but 撤销 is an HTML link, so it cannot list on the silly log, I don't suggest to add too many strict limits on the room join fire condition, because it always changes a lot, so I think just keep it fuzzy matching is enough.

CONCLUSION

Base on above information, I will pr then, and I think it happens not only on room join event, so I delete $ on each regex expression.

@lijiarui
Copy link
Member Author

lijiarui commented Mar 7, 2018

UNIT TEST PART

I add a unit test, and the current version cannot pass the unit test, log as follows:

not ok 67 Error: checkRoomJoin() not found matched re of You've invited "李卓桓" to the group chat
  ---
    operator: error
    expected: |-
      undefined
    actual: |-
      [Error: checkRoomJoin() not found matched re of You've invited "李卓桓" to the group chat ]
    at: runMicrotasksCallback (internal/process/next_tick.js:115:5)
    stack: |-
      Error: checkRoomJoin() not found matched re of You've invited "李卓桓" to the group chat
          at Object.parseRoomJoin (/Users/jiaruili/git/rui/wechaty/src/puppet-web/firer.ts:147:15)
          at contentList.forEach (/Users/jiaruili/git/rui/wechaty/src/puppet-web/firer.spec.ts:126:32)
          at Array.forEach (<anonymous>)
          at Object.<anonymous> (/Users/jiaruili/git/rui/wechaty/src/puppet-web/firer.spec.ts:125:17)
          at Generator.next (<anonymous>)
          at /Users/jiaruili/git/rui/wechaty/src/puppet-web/firer.spec.ts:8:71
          at new Promise (<anonymous>)
          at __awaiter (/Users/jiaruili/git/rui/wechaty/src/puppet-web/firer.spec.ts:4:12)
          at Test.test (/Users/jiaruili/git/rui/wechaty/src/puppet-web/firer.spec.ts:46:32)
          at Test.bound [as _cb] (/Users/jiaruili/git/rui/wechaty/node_modules/tape/lib/test.js:64:32)
          at Test.run (/Users/jiaruili/git/rui/wechaty/node_modules/blue-tape/blue-tape.js:13:30)
          at Test.bound [as run] (/Users/jiaruili/git/rui/wechaty/node_modules/tape/lib/test.js:64:32)
          at Immediate.next (/Users/jiaruili/git/rui/wechaty/node_modules/tape/lib/results.js:71:15)
          at runCallback (timers.js:773:18)
          at tryOnImmediate (timers.js:734:5)
          at processImmediate [as _immediateCallback] (timers.js:711:5)
  ...
# parseRoomLeave()

@huan
Copy link
Member

huan commented Mar 7, 2018

Thanks for adding the test cases, but please do not just copy/paste all the code into the discussion because it's not easy to read and more like spam.

Before this PR could be merged, there's one more rule you need follow: we should always use strict RegExp condition instead of a loose one because we'd better make sure what's happening inside our system.

So please: keep the $ for the RegExp rules.

@lijiarui
Copy link
Member Author

lijiarui commented Mar 7, 2018

Update as follows:

What I said about 你邀请"桔小秘"加入了群聊 撤销 above is wrong, this only shows on mobile, while web shows 你邀请"桔小秘"加入了群聊

Reproduce Code

In order to provide a strict RegExp condition, I test all conditions and found all room-join event doesn't work because wechat changes its room join in the system message and all of the system message are different, and I put all related logs here:

I add the following console.log in [room-bot.ts]

.on('message', async function(this, message) {
  const room    = message.room()
  const sender  = message.from()
  const content = message.content()

  console.log((room ? '[' + room.topic() + ']' : '')
              + '<' + sender.name() + '>'
              + ':' + message.toStringDigest(),
  )

  console.log('======console.log the last 4 charCode======')
  console.log(content.charCodeAt(content.length - 4)) 
  console.log(content.charCodeAt(content.length - 3)) 
  console.log(content.charCodeAt(content.length - 2)) 
  console.log(content.charCodeAt(content.length - 1)) 
  console.log('===================================')

All Related Log

1. English version: Bot invite others to the group.

eg: You invited 管理员 to the group chat.

There are 3 blank(charCode is 32) here. Log as follows:

19:11:58 VERB PuppetWebFirer checkRoomJoin(You invited 管理员 to the group chat.   )
19:11:58 SILL PuppetWebFirer fireRoomJoin() "You invited 管理员 to the group chat.   " is not a join message
19:11:58 VERB PuppetWebFirer fireRoomLeave(You invited 管理员 to the group chat.   )
19:11:58 SILL Message ready()
19:11:58 SILL Room ready()
19:11:58 SILL Room ready()
19:11:58 VERB Room topic()
[桔小秘,桔小秘,李佳芮]<桔小秘,桔小秘,李佳芮>:{SYS}You invited 管理员 to the group chat.
======console.log the last 4 charCode======
<.>charCode is<46>
< >charCode is<32>
< >charCode is<32>
< >charCode is<32>
===================================

2. English version: Not bot invite others to the group.

eg: 管理员 invited 小桔建群助手 to the group chat

There no no blank or punctuation here. Log as follows:

19:41:46 VERB PuppetWebFirer checkRoomJoin(管理员 invited 小桔建群助手 to the group chat)
19:41:46 SILL PuppetWebFirer fireRoomJoin() "管理员 invited 小桔建群助手 to the group chat" is not a join message
19:41:46 VERB PuppetWebFirer fireRoomLeave(管理员 invited 小桔建群助手 to the group chat)
19:41:46 SILL Message ready()
19:41:46 SILL Room ready()
19:41:46 SILL Room ready()
19:41:46 VERB Room topic()
[桔小秘,桔小秘,李佳芮]<桔小秘,桔小秘,李佳芮>:{SYS}管理员 invited 小桔建群助手 to the group chat
======console.log the last 4 charCode======
<c>charCode is<99>
<h>charCode is<104>
<a>charCode is<97>
<t>charCode is<116>
===================================

3. English version: Others join the group by qrcode shared by the bot.

eg: "管理员" joined group chat via the QR code you shared.

There are 2 blank(charCode is 32) here. Log as follows:

19:15:14 VERB PuppetWebFirer checkRoomJoin("管理员" joined group chat via the QR code you shared.  )
19:15:14 SILL PuppetWebFirer fireRoomJoin() ""管理员" joined group chat via the QR code you shared.  " is not a join message
19:15:14 VERB PuppetWebFirer fireRoomLeave("管理员" joined group chat via the QR code you shared.  )
19:15:14 SILL Message ready()
19:15:14 SILL Room ready()
19:15:14 SILL Room ready()
19:15:14 VERB Room topic()
[桔小秘,桔小秘,李佳芮]<桔小秘,桔小秘,李佳芮>:{SYS}"管理员" joined group chat via the QR code you shared.
======console.log the last 4 charCode======
<d>charCode is<100>
<.>charCode is<46>
< >charCode is<32>
< >charCode is<32>
===================================

4. English version: Others join the group by qrcode not shared by bot.

eg: "宁锐锋" joined the group chat via the QR Code shared by "管理员".

There are no blank(charCode is 32) here. Log as follows:

19:18:37 SILL Room ready()
19:18:37 VERB Room topic()
[桔小秘,桔小秘,李佳芮]<桔小秘,桔小秘,李佳芮>:{SYS}"宁锐锋" joined the group chat via the QR Code shared by "管理员".
======console.log the last 4 charCode======
<>charCode is<29702>
<>charCode is<21592>
<">charCode is<34>
<.>charCode is<46>
===================================

5. Chinese version: Bot invite others to the group.

eg: 你邀请"管理员"加入了群聊

There are 2 blank(charCode is 32) here. Log as follows:

19:47:43 VERB PuppetWebFirer checkRoomJoin(你邀请"管理员"加入了群聊  )
19:47:43 SILL PuppetWebFirer fireRoomJoin() "你邀请"管理员"加入了群聊  " is not a join message
19:47:43 VERB PuppetWebFirer fireRoomLeave(你邀请"管理员"加入了群聊  )
19:47:43 SILL Message ready()
19:47:43 SILL Room ready()
19:47:43 SILL Room ready()
19:47:43 VERB Room topic()
[神奇群]<神奇群>:{SYS}你邀请"管理员"加入了群聊
======console.log the last 4 charCode======
<群>charCode is<32676>
<聊>charCode is<32842>
< >charCode is<32>
< >charCode is<42>
===================================

6. Chinese version: Not bot invite others to the group.

eg: "管理员"邀请"宁锐锋"加入了群聊

There no no blank or punctuation here. Log as follows:

19:50:47 VERB PuppetWebFirer checkRoomJoin("管理员"邀请"宁锐锋"加入了群聊)
19:50:47 SILL PuppetWebFirer fireRoomJoin() ""管理员"邀请"宁锐锋"加入了群聊" is not a join message
19:50:47 VERB PuppetWebFirer fireRoomLeave("管理员"邀请"宁锐锋"加入了群聊)
19:50:47 SILL Message ready()
19:50:47 SILL Room ready()
19:50:47 SILL Room ready()
19:50:47 VERB Room topic()
[神奇群]<神奇群>:{SYS}"管理员"邀请"宁锐锋"加入了群聊
======console.log the last 4 charCode======
<>charCode is<20837>
<>charCode is<20102>
<>charCode is<32676>
<>charCode is<32842>
===================================

7. Chinese version: Others join the group by qrcode not shared by bot.

eg: "管理员"通过扫描你分享的二维码加入群聊

There are 2 blank(charCode is 32) here. Log as follows:

19:57:40 VERB PuppetWebFirer checkRoomJoin("管理员"通过扫描你分享的二维码加入群聊  )
19:57:40 SILL PuppetWebFirer fireRoomJoin() ""管理员"通过扫描你分享的二维码加入群聊  " is not a join message
19:57:40 VERB PuppetWebFirer fireRoomLeave("管理员"通过扫描你分享的二维码加入群聊  )
19:57:40 SILL Message ready()
19:57:40 SILL Room ready()
19:57:40 SILL Room ready()
19:57:40 VERB Room topic()
[神奇群]<神奇群>:{SYS}"管理员"通过扫描你分享的二维码加入群聊
======console.log the last 4 charCode======
<群>charCode is<32676>
<聊>charCode is<32842>
< >charCode is<32676>
< >charCode is<32842>
===================================

8. Chinese version: Others join the group by qrcode not shared by bot.

eg: " 苏轼"通过扫描"管理员"分享的二维码加入群聊

There are 1 blank(charCode is 32) here, in a very strange position. Log as follows:

20:00:32 VERB PuppetWebFirer checkRoomJoin(" 苏轼"通过扫描"管理员"分享的二维码加入群聊)
20:00:32 SILL PuppetWebFirer fireRoomJoin() "" 苏轼"通过扫描"管理员"分享的二维码加入群聊" is not a join message
20:00:32 VERB PuppetWebFirer fireRoomLeave(" 苏轼"通过扫描"管理员"分享的二维码加入群聊)
20:00:32 SILL Message ready()
20:00:32 SILL Room ready()
20:00:32 SILL Room ready()
20:00:32 VERB Room topic()
[神奇群]<神奇群>:{SYS}" 苏轼"通过扫描"管理员"分享的二维码加入群聊
======console.log the last 4 charCode======
<加>charCode is<21152>
<入>charCode is<20837>
<群>charCode is<32676>
<聊>charCode is<32842>
===================================

@lijiarui
Copy link
Member Author

lijiarui commented Mar 7, 2018

And, we should think more about the frequency system log changing.... log all of this is really a lot of dirty hard work but is important.....

@lijiarui
Copy link
Member Author

lijiarui commented Mar 7, 2018

It seems strange for me, I run the unit test at my mac(npm run test:unit), and all unit test passed... log as follows:

1..191
# tests 191
# pass  191

# ok

@huan huan requested a review from a team March 7, 2018 14:26
@huan
Copy link
Member

huan commented Mar 7, 2018

Good job!

Could you please replace all the spaces inside the RegExp by \s+ for a better match?

@lijiarui
Copy link
Member Author

lijiarui commented Mar 7, 2018

I didn't replace all spaces, I think we should just replace strange spaces to \s+ to recognize better.

@@ -54,13 +54,13 @@ const regexConfig = {

roomJoinInvite: [
// There are 3 blank(charCode is 32) here. eg: You invited 管理员 to the group chat.
/^(.+?) invited (.+) to the group chat. $/,
/^(.+?) invited (.+) to the group chat.\s+\s+\s+$/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure you understand what you are doing before you change anything... :(

Copy link
Member

@hczhcz hczhcz Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{3}(a space at the beginning) or \s{3} should work if you want to match exactly three spaces :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hczhcz thanks!

Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One \s+ is enough.

@@ -54,13 +54,13 @@ const regexConfig = {

roomJoinInvite: [
// There are 3 blank(charCode is 32) here. eg: You invited 管理员 to the group chat.
/^(.+?) invited (.+) to the group chat.\s+\s+\s+$/,
/^(.+?) invited (.+) to the group chat.\s{3}$/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-\s{3}
+\s+

@lijiarui
Copy link
Member Author

lijiarui commented Mar 8, 2018

Consider more, if we need strict regExp, we should also collect all room system message which cannot be recognized. So I change the following function return type:

async function checkRoomJoin(m: Message): Promise<void> 
async function checkRoomLeave(m: Message): Promise<void> 
async function checkRoomTopic(m: Message): Promise<void> 

to

async function checkRoomJoin(m: Message): Promise<boolean> 
async function checkRoomLeave(m: Message): Promise<boolean> 
async function checkRoomTopic(m: Message): Promise<boolean> 

And throw an error when the system message cannot be recognized. Then we can monitor these exceptions more.

const joinResult  = await Firer.checkRoomJoin.call(this  , m)
const leaveResult = await Firer.checkRoomLeave.call(this , m)
const topicRestul = await Firer.checkRoomTopic.call(this , m)

if (!joinResult && !leaveResult && !topicRestul) {
     throw new Error(`PuppetWebEvent, checkRoomSystem message: <${m.content()}> not found`)
}

@lijiarui
Copy link
Member Author

lijiarui commented Mar 8, 2018

But I'm not that sure, whether throw error is better or log.error() is better...

For me, it is really important to monitor room status, but for others, maybe they don't want to see throw error...

@huan
Copy link
Member

huan commented Mar 8, 2018

Yes, I believe silence the Error is better because not all users care about there is an unrecognizable message in a room.

log.error() will be better. However, please do not introduce unrelated changes in this PR, when the title is "modify scan qrcode sentence"

@lijiarui lijiarui changed the title modify scan qrcode sentence cannot recognize room event Mar 8, 2018
@lijiarui
Copy link
Member Author

lijiarui commented Mar 8, 2018

So I change the pr topic... :)

Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The better practice is to create another PR to do another job, instead of adding more and more unrelated modifications into one topic.

const topicRestul = await Firer.checkRoomTopic.call(this , m)

if (!joinResult && !leaveResult && !topicRestul) {
log.error('PuppetWebEvent', `checkRoomSystem message: <${m.content()}> not found`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not an error but a warning.

Copy link
Member Author

@lijiarui lijiarui Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please forgive for my lazy and using one pr.... and I will do a better practice next time. thanks...

But I think here using a silence error is better than a warning, because this is a system error, for the error regExp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be an error but a warning at here.

@huan
Copy link
Member

huan commented Mar 9, 2018

Please get one more approvement from any of the contributors before I could be able to merge this PR.

Thanks.

Copy link
Member

@hczhcz hczhcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good!

For the log.error/log.warning, this message indicates that WeChat's behavior changes and the user have to update the library for new regex rules. It would be good to make similar error messages consist.

@lijiarui
Copy link
Member Author

@hczhcz thanks :)

@huan huan merged commit a746f8e into wechaty:master Mar 11, 2018
@lijiarui lijiarui deleted the qrcode branch June 14, 2018 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants