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

add: Message.forward() forward message #727

Merged
merged 17 commits into from
Sep 1, 2017
Merged

Conversation

binsee
Copy link

@binsee binsee commented Aug 13, 2017

You can directly forward files and multimedia, but limit the file size to 25Mb or less, otherwise it will fail because the server policy.

Issues: #726

You can directly forward files and multimedia, but limit the file size to 25Mb or less, otherwise it will fail because the server policy.
@binsee
Copy link
Author

binsee commented Aug 13, 2017

screenshot:

image
image
image

@binsee binsee mentioned this pull request Aug 13, 2017
@binsee
Copy link
Author

binsee commented Aug 13, 2017

demo code:

const QrcodeTerminal  = require('qrcode-terminal')
const finis           = require('finis')

/**
 * Change `import { ... } from '../'`
 * to     `import { ... } from 'wechaty'`
 * when you are runing with Docker or NPM instead of Git Source.
 */
import {
  config,
  Wechaty,
  log,
  MediaMessage,
  MsgType,
}               from './'

//const roomArr = { }

const welcome = `
| __        __        _           _
| \\ \\      / /__  ___| |__   __ _| |_ _   _
|  \\ \\ /\\ / / _ \\/ __| '_ \\ / _\` | __| | | |
|   \\ V  V /  __/ (__| | | | (_| | |_| |_| |
|    \\_/\\_/ \\___|\\___|_| |_|\\__,_|\\__|\\__, |
|                                     |___/

=============== Powered by Wechaty ===============
`

console.log(welcome)
const bot = Wechaty.instance({ profile: config.DEFAULT_PROFILE })

const forwards = <string[]>[]

bot
.on('logout'	, user => log.info('Bot', `${user.name()} logouted`))
.on('login'	  , user => {
  log.info('Bot', `${user.name()} logined`)
  bot.say('Wechaty login')
})
.on('error'   , e => {
  log.info('Bot', 'error: %s', e)
  bot.say('Wechaty error: ' + e.message)
})
.on('scan', (url, code) => {
  if (!/201|200/.test(String(code))) {
    const loginUrl = url.replace(/\/qrcode\//, '/l/')
    QrcodeTerminal.generate(loginUrl)
  }
  console.log(`${url}\n[${code}] Scan QR Code in above url to login: `)
})
.on('message', async m => {
  try {
    const room = m.room()
    const forwardId = room ? m.obj.room as string : m.obj.from as string
    const name = room ? room.topic() as string : m.from().name() as string
    let doForward = false
    console.log((room ? '[' + room.topic() + ']' : '')
                + '<' + m.from().name() + '>'
                + ':' + m.toStringDigest(),
    )
    if (!m.self()) {
      if (/^(ding|ping|bing|code)$/i.test(m.toString())) {
        doForward = true
        m.say('dong')
        log.info('Bot', 'REPLY: dong')
        await m.say(new MediaMessage(__dirname + '/../image/BotQrcode.png'))
        await m.say('Scan now, because other Wechaty developers want to talk with you too!\n\n(secret code: wechaty)')
        log.info('Bot', 'REPLY: Image')
      } else if (/^(forward|f)$/i.test(m.toString())) {
        if (forwards.indexOf(forwardId) < 0) {
          forwards.push(forwardId)
          await m.say(`你为${room ? '群[' + name + ']' : '自己'}订阅了信息转发,稍后接收到的信息将转发给${room ? '群' : '你'}`)
        } else {
          await m.say(`${room ? '群[' + name + ']' : '自己'} 已经订阅了信息转发`)
        }
      } else if (/^(unforward|unf)$/i.test(m.toString())) {
        if (forwards.indexOf(forwardId) >= 0) {
          forwards.splice(forwards.indexOf(forwardId), 1)
          await m.say(`你为${room ? '群[' + name + ']' : '自己'}取消了信息转发,稍后接收到的信息将转发给${room ? '群' : '你'}`)
        } else {
          await m.say(`${room ? '群[' + name + ']' : '自己'} 已经取消了信息转发`)
        }
      } else if (/^(list)$/i.test(m.toString())) {
        await m.say(`当前订阅列表数量为${forwards.length}\n列表如下:\n${forwards.join(',\n')}`)
      } else if (/^(帮助|help|h|使用)$/i.test(m.toString())) {
          await m.say(`你可以在任意聊天界面发送指令订阅/取消订阅信息同步\n'forward' 或 'f' 指令订阅,\n'unforward' 或 'unf' 指令取消订阅\n'list'查看订阅列表\n'help'查看帮助`)
      } else {
        doForward = true
        // 有效消息,且当前聊天未参与同步转发
        if (m.toString().length > 0 && forwards.indexOf(forwardId) < 0) {
          await m.say(`你发的消息内容为: '${m.toString()}'`)
          await m.say(`你可以在任意聊天界面发送指令订阅/取消订阅信息同步\n'forward' 或 'f' 指令订阅,\n'unforward' 或 'unf' 指令取消订阅\n'list'查看订阅列表\n'help'查看帮助`)
        }
      }

      //过滤系统信息
      if (m.type() === MsgType.SYSNOTICE || m.type() === MsgType.SYS) {
        log.info('Bot', '系统信息,不转发')
        doForward = false
      }
      // 过滤 桔小子 bot的 转发信息
      if (/(消息来自)/i.test(m.toString())) {
        doForward = false
        log.info('Bot', '检测到疑似其他Bot信息,不转发')
      }
    }
    if (doForward) {
      forwards.forEach(id => {
        if (id !== forwardId) { // 避免转发到当前会话
          m.forward(id)
        } else {
          log.info('forwards.forEach()', `id=${id},forwardId=${forwardId}`)
        }
      });
    } else {
      log.info('Bot', 'doForward=false')
    }
    } catch (e) {
      log.error('Bot', 'on(message) exception: %s', e)
    }
  })

bot.init()
.catch(e => {
  log.error('Bot', 'init() fail: %s', e)
  bot.quit()
  process.exit(-1)
})

finis((code, signal) => {
  const exitMsg = `Wechaty exit ${code} because of ${signal} `
  console.log(exitMsg)
  bot.say(exitMsg)
})

@binsee
Copy link
Author

binsee commented Aug 13, 2017

Thanks for @lijiarui !

parameter support Room/Contact/roomId/ContactId

  public forward(room: Room): Promise<any>
  public forward(contact: Contact): Promise<any>
  public forward(id: string): Promise<any>
  public forward(sendTo: string|Room|Contact): Promise<any> {}

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.

Hi @binsee, your forward implementation is awesome!

This will help the wechaty user very much when we are dealing with many attachments.

I have some questions and suggestions for your PR, please read my review, and let me know if you have any question/suggestion.

Looking forward to merge your new PR, thanks!

@@ -42,7 +42,9 @@ export interface MsgRawObj {
MMActualSender: string, // getUserContact(message.MMActualSender,message.MMPeerUserName).isContact()
MMPeerUserName: string, // message.MsgType == CONF.MSGTYPE_TEXT && message.MMPeerUserName == 'newsapp'
ToUserName: string,
FromUserName: string,
Copy link
Member

Choose a reason for hiding this comment

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

MsgRawObj is the object from the WebWxApp.

Does FromUserName and Content exist in the structure?

Just want to confirm that.

Copy link
Author

Choose a reason for hiding this comment

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

When MsgType in [1,3,43,47,49],FromUserName and Content exist..
Other unconfirmed.

Copy link
Member

Choose a reason for hiding this comment

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

Then I think it's enough for us to believe that they always exist.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -132,6 +134,16 @@ export interface MsgRawObj {
* MsgType == CONF.MSGTYPE_VERIFYMSG
*/
RecommendInfo?: RecommendInfo,

/**
* Transpond Message
Copy link
Member

Choose a reason for hiding this comment

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

Do those attributes all exist in the WebWxApp data structure?

Need to confirm them too.

Copy link
Author

Choose a reason for hiding this comment

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

Not exist.
I will fix it.

Copy link
Member

@huan huan Aug 13, 2017

Choose a reason for hiding this comment

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

Thanks. Let's make the MsgRawObj reflect and only reflect the WebWxApp data struct.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

src/message.ts Outdated
*/
public forward(room: Room): Promise<any>
public forward(contact: Contact): Promise<any>
public forward(id: string): Promise<any>
Copy link
Member

Choose a reason for hiding this comment

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

Send to UserId should not be supported.

The API of Message class should be abstracted with the Room and Contact, but not the UserName or other id.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

src/message.ts Outdated
public forward(room: Room): Promise<any>
public forward(contact: Contact): Promise<any>
public forward(id: string): Promise<any>
public forward(sendTo: string|Room|Contact): Promise<any> {
Copy link
Member

Choose a reason for hiding this comment

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

Should return Promise<boolean>, according to the forward function in wechaty-bro.js

Copy link
Author

Choose a reason for hiding this comment

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

I will fix it.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

src/message.ts Outdated
public forward(contact: Contact): Promise<any>
public forward(id: string): Promise<any>
public forward(sendTo: string|Room|Contact): Promise<any> {
const m = <MsgRawObj>this.rawObj
Copy link
Member

Choose a reason for hiding this comment

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

Why <MsgRawObj> is needed here?

Does not the this.rawObj already that type?

Copy link
Author

@binsee binsee Aug 13, 2017

Choose a reason for hiding this comment

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

When I remove it, the typescript check will be error.

src/message.ts(645,35): error TS2532: Object is possibly 'undefined'.
src/message.ts(645,61): error TS2532: Object is possibly 'undefined'.
src/message.ts(646,28): error TS2532: Object is possibly 'undefined'.
src/message.ts(648,43): error TS2532: Object is possibly 'undefined'.
src/message.ts(650,9): error TS2532: Object is possibly 'undefined'.
src/message.ts(651,30): error TS2532: Object is possibly 'undefined'.
src/message.ts(652,9): error TS2532: Object is possibly 'undefined'.
src/message.ts(653,25): error TS2532: Object is possibly 'undefined'.
src/message.ts(654,9): error TS2532: Object is possibly 'undefined'.
src/message.ts(655,50): error TS2532: Object is possibly 'undefined'.
src/message.ts(658,16): error TS2345: Argument of type 'MsgRawObj | undefined' is not assignable to parameter of type 'MsgRawObj'.
  Type 'undefined' is not assignable to type 'MsgRawObj'

Copy link
Member

Choose a reason for hiding this comment

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

That is because this.rawObj might be undefined.

So you need to check undefined instead of force it to be MsgRawObj.

Add the follow code at the begining of the function, then you will be able to use this.rawObj because that will make sure it is not undefined.

if (!this.rawObj) {
  throw new Error('no rawObj!')
}

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -422,6 +423,20 @@ export class Bridge {
})
}

public forward(baseData: MsgRawObj, patchData: MsgRawObj): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

As I asked before: could we just use forward(rawObj: MsgRawObj) at here?

Copy link
Author

Choose a reason for hiding this comment

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

After thinking, I can only do this design.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I understood after reading the code you provided. Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

if (!patchData.MMActualContent && !patchData.MMSendContent) {
throw new Error('cannot say nothing')
}
return this.proxyWechaty('forward', baseData, patchData)
Copy link
Member

Choose a reason for hiding this comment

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

Also need to merge two args to one at here.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -490,6 +491,26 @@ export class PuppetWeb extends Puppet {
return ret
}

public async forward(baseData: MsgRawObj, patchData: MsgRawObj): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

forwrd(rawObj: MsgRawObj)

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -505,6 +505,32 @@
return true
}

function forward(baseData, patchData) {
Copy link
Member

Choose a reason for hiding this comment

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

One parameter please

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@@ -54,6 +55,7 @@ export abstract class Puppet extends EventEmitter implements Sayable {
public abstract self(): Contact

public abstract send(message: Message | MediaMessage): Promise<boolean>
public abstract forward(baseData: MsgRawObj, patchData: MsgRawObj): Promise<boolean>
Copy link
Member

Choose a reason for hiding this comment

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

One parameter.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@binsee
Copy link
Author

binsee commented Aug 13, 2017

About baseData and patchData: This design is needed

transpondDialogController in WxApp:

  angular.module("Controllers").controller("transpondDialogController", ["$rootScope", "$scope", "$timeout", "$state", "$log", "$document", "chatFactory", "contactFactory", "appFactory", "chatroomFactory", "confFactory", "mmpop", "ngDialog", "utilFactory", "stateManageService", "accountFactory",
    function(e, t, a, n, i, o, chatFactory, c, s, l, confFactory, f, u, m, g, accountFactory) {
      function h(msg, userName) {
        if (e.MsgType != confFactory.MSGTYPE_SYS) {
          var newMsg = angular.copy(msg);
          newMsg.ToUserName = userName;
          newMsg.FromUserName = accountFactory.getUserName();
          newMsg.isTranspond = !0;
          newMsg.MsgIdBeforeTranspond = msg.MsgIdBeforeTranspond || msg.MsgId;
          newMsg._h = void 0;
          newMsg._offsetTop = void 0;
          newMsg.MMSourceMsgId = msg.MsgId;
          newMsg.Scene = 2;
          newMsg = chatFactory.createMessage(newMsg); // call createMessage()

          // The following parameters need to be overridden after calling createMessage()
          newMsg.sendByLocal = !1;
          newMsg.Content = m.htmlDecode(newMsg.Content.replace(/^@\w+:<br\/>/, ""));
          newMsg.MMActualSender = accountFactory.getUserName();
          newMsg.MMSendContent && (newMsg.MMSendContent = newMsg.MMSendContent.replace(/^@\w+:\s/, ""));
          newMsg.MMDigest && (newMsg.MMDigest = newMsg.MMDigest.replace(/^@\w+:/, ""));
          newMsg.MMActualContent && (newMsg.MMActualContent = m.clearHtmlStr(newMsg.MMActualContent.replace(/^@\w+:<br\/>/, "")));

          // append and send message
          chatFactory.appendMessage(newMsg);
          chatFactory.sendMessage(newMsg)
        }
      }

chatFactory.createMessage() in WxApp:

createMessage: function(e) { // 创建消息体
  switch (e.FromUserName || (e.FromUserName = accountFactory.getUserName()),
    e.ToUserName || (e.ToUserName = this.getCurrentUserName()), //
    e.ClientMsgId = e.LocalID = e.MsgId = (utilFactory.now() + Math.random().toFixed(3)).replace(".", ""), //生成本地消息id
    e.CreateTime = Math.round(utilFactory.now() / 1e3),
    e.MMStatus = confFactory.MSG_SEND_STATUS_READY, //标记消息的状态为准备状态
    e.sendByLocal = !0, // 是否是本地信息,如果需要转发文件,必须为 false
    e.MsgType) { //判断消息类型

    case confFactory.MSGTYPE_TEXT:
      var t = [];
      e.Content = e.Content.replace(/<input.*?un="(.*?)".*?value="(.*?)".*?>/g,
          function(e, a, n) {
            return t.push(a), n
          }),
        e.MMAtContacts = t.join(","),
        e.MMSendContent = utilFactory.htmlDecode(utilFactory.clearHtmlStr(e.Content.replace(/<(?:img|IMG).*?text="(.*?)".*?>/g,
          function(e, t) {
            return t.replace(confFactory.MM_EMOTICON_WEB, "")
          }).replace(/<(?:br|BR)\/?>/g, "\n"))).replace(/<(.*?)>/g, function(e) {
          return emojiFactory.EmojiCodeMap[emojiFactory.QQFaceMap[e]] || e
        }),
        e.Content = e.Content.replace(/<(?!(img|IMG|br|BR))[^>]*>/g, "").replace(/\n/g, "<br>");
      break;

    case confFactory.MSGTYPE_APP:
      if (e.AppMsgType == confFactory.APPMSGTYPE_URL) break;
      e.AppMsgType = confFactory.APPMSGTYPE_ATTACH,
        e.Content = "<msg><appmsg appid='wxeb7ec651dd0aefa9' sdkver=''><title>" + e.FileName + "</title><des></des><action></action><type>" + confFactory.APPMSGTYPE_ATTACH + "</type><content></content><url></url><lowurl></lowurl><appattach><totallen>" + e.FileSize + "</totallen><attachid>#MediaId#</attachid><fileext>" + (e.MMFileExt || e.MMAppMsgFileExt) + "</fileext></appattach><extinfo></extinfo></appmsg></msg>"
  }
  return e
},

foraward() in wechaty-bro

  function forward(baseData, patchData) {
    var chatFactory = WechatyBro.glue.chatFactory
    var confFactory = WechatyBro.glue.confFactory

    if (!chatFactory || !confFactory) {
      log('forward() chatFactory or confFactory not exist.')
      return false
    }

    try {
      var m = chatFactory.createMessage(baseData)

      // Need to override the parametes after called createMessage()
      m = Object.assign(m, patchData)

      chatFactory.appendMessage(m)
      chatFactory.sendMessage(m)
    } catch (e) {
      log('forward() exception: ' + e.message)
      return false
    }
    return true
  }

In addition, some notes are added.
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.

Thank you for update the PR and let me know the pattern of WebWxApp of forwarding messages.

This PR will LGTM after you follow my reviews and mark all of them to be resolved.

src/message.ts Outdated
*/
public forward(room: Room): Promise<boolean>
public forward(contact: Contact): Promise<boolean>
public forward(id: string): Promise<boolean>
Copy link
Member

Choose a reason for hiding this comment

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

Send to UserId should not be supported.

The API of Message class should be abstracted with the Room and Contact, but not the UserName or other id.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

// The following parameters need to be overridden after calling createMessage()

// If you want to forward the file, would like to skip the duplicate upload, sendByLocal must be false.
// But need to pay attention to file.size> 25Mb, due to the server policy restrictions, need to re-upload
Copy link
Member

Choose a reason for hiding this comment

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

Could we reuse the MediaId instead of re-upload it?

Because some projects use the MediaId directly, which means it might be valid across sessions and accounts.

See: #422 (comment)

I think the solution should be not to upload the same media file twice by caching the MediaId of the Uploaded File and reuse it in the future.

There also has a very interesting project that we could have a look: 微信表情轰炸器

It can send emoticon by MediaId directly, no need to upload anything.

Copy link
Member

Choose a reason for hiding this comment

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

The project 微信表情轰炸器 is worth having a look! :)

It only stores the MediaId, and reuse it for sending the emoticons.

Copy link
Author

Choose a reason for hiding this comment

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

By using fiddler to intercept data, testing found files larger than 25mb in webwx can not use mediaId for multiplexing, the server will return processing failure.
I think this problem needs to be shelved and later resolved. Need to refactor the upload file flow.

"BaseResponse": {
"Ret": 1,
"ErrMsg": ""
}

Copy link
Author

Choose a reason for hiding this comment

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

resolved

src/message.ts Outdated
newMsg.MMActualContent = UtilLib.stripHtml(m.MMActualContent.replace(/^@\w+:<br\/>/, ''))

return config.puppetInstance()
.forward(m, newMsg)
Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's keep the patch data design because it's the pattern from WebWxApp.

@@ -422,6 +423,20 @@ export class Bridge {
})
}

public forward(baseData: MsgRawObj, patchData: MsgRawObj): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I understood after reading the code you provided. Thanks.

@huan huan requested a review from lijiarui August 16, 2017 15:21
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.

Thanks for the update.

I'll approve it after you follow my latest reviews.

Nice job!

src/message.ts Outdated
public forward(contact: Contact): Promise<any>
public forward(id: string): Promise<any>
public forward(sendTo: string|Room|Contact): Promise<any> {
const m = <MsgRawObj>this.rawObj
Copy link
Member

Choose a reason for hiding this comment

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

That is because this.rawObj might be undefined.

So you need to check undefined instead of force it to be MsgRawObj.

Add the follow code at the begining of the function, then you will be able to use this.rawObj because that will make sure it is not undefined.

if (!this.rawObj) {
  throw new Error('no rawObj!')
}

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.

Update the outdated reviews for the MsgRawObj undefined problem.

src/message.ts Outdated
public forward(room: Room): Promise<boolean>
public forward(contact: Contact): Promise<boolean>
public forward(sendTo: Room|Contact): Promise<boolean> {
const m = <MsgRawObj>this.rawObj
Copy link
Member

Choose a reason for hiding this comment

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

That is because this.rawObj might be undefined.

So you need to check undefined instead of force it to be MsgRawObj.

Add the follow code at the begining of the function, then you will be able to use this.rawObj because that will make sure it is not undefined.

if (!this.rawObj) {
  throw new Error('no rawObj!')
}

Copy link
Author

Choose a reason for hiding this comment

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

resolved

binsee added 2 commits August 17, 2017 00:16
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.

Thanks for your great job!

LGTM now.

I'll merge this PR after it getting total three approvements, and I'd like to tag a new version of v0.9 after we confirmed the code can work without any problem.

Looking forward to the new version, Cheers!

// The following parameters need to be overridden after calling createMessage()

// If you want to forward the file, would like to skip the duplicate upload, sendByLocal must be false.
// But need to pay attention to file.size> 25Mb, due to the server policy restrictions, need to re-upload
Copy link
Member

Choose a reason for hiding this comment

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

The project 微信表情轰炸器 is worth having a look! :)

It only stores the MediaId, and reuse it for sending the emoticons.

@huan huan removed the request for review from imWildCat August 16, 2017 18:26
binsee added 3 commits August 17, 2017 02:47
```
m.forward(Room.load(id), { user: '$USER', room: 'Room[$ROOM]', custom: 'forward from $ROOM$USER' })

// source message: 'test msg'
// from room: 'testRoom'
// from user: 'Bot'
// forward message: `forward from Room[testRoom]Bot:\ntest msg`
```
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.

I think the forward methods should be moved from the class Message to MediaMessage.

src/message.ts Outdated
*/
public forward(room: Room, option?: ForwardOption): Promise<boolean>
public forward(contact: Contact, option?: ForwardOption): Promise<boolean>
public forward(sendTo: Room|Contact, option?: ForwardOption): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

I think the forward methods should be moved from the class Message to MediaMessage.

Copy link
Author

Choose a reason for hiding this comment

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

Your opinion I agree, i will move it.

Copy link
Author

@binsee binsee Aug 16, 2017

Choose a reason for hiding this comment

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

I think again.
Initially I put the message in the message, hoping to achieve through a single function of multi-group live.
In the multi-group live operation, text, voice, pictures are more types of messages.
And only use forward () this method, you can easily achieve a variety of message types of forwarding.
So, will forward () be retained in Message or maybe?

Copy link
Member

@huan huan Aug 17, 2017

Choose a reason for hiding this comment

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

Please see my comment about this at: #726 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Moved it.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

binsee added 2 commits August 17, 2017 14:32
In room msg, the content prefix sender:, need to be removed, otherwise the forwarded sender will display the source message sender, causing self () to determine the error
src/message.ts Outdated
newMsg.sendByLocal = false
newMsg.MMActualSender = config.puppetInstance().userId || ''
if (m.MMSendContent)
newMsg.MMSendContent = m.MMSendContent.replace(/^@\w+:\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 always use { and } around the code block inside if.

Copy link
Author

Choose a reason for hiding this comment

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

resolved

@huan
Copy link
Member

huan commented Aug 18, 2017

@binsee Could you please mark all the reviews from me as resolved?

Because I'm ok with all of them. After marked as resolved, the outdated reviews will be hidden in the "Files Change" UI.

Thanks!

@huan
Copy link
Member

huan commented Aug 18, 2017

Thanks for the reply the "resolved" to my reviews.

But I remember that there has a [resolved] button under each review, which you could click that button to make the review as resolved state. Could you find it?

@binsee
Copy link
Author

binsee commented Aug 18, 2017

@zixia Sorry,I can't find it.
And I search 'resolved' in github help,still not found.

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Changes Unknown when pulling 0a76682 on binsee:add-transpond-msg into ** on Chatie:master**.

@huan huan changed the title add: Message.forward(id) forward message add: Message.forward() forward message Aug 19, 2017
Copy link
Member

@lijiarui lijiarui left a comment

Choose a reason for hiding this comment

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

looks good to me, Thanks!

@binsee
Copy link
Author

binsee commented Aug 31, 2017

@hczhcz
Can I invite you to take the time to review this pr?
I hope this enhancement can be open to everyone to use, 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.

This Message.forward implementation is great!

Please run jsdoc (npm run doc) so that the new API will take effect in the documentation.

@binsee
Copy link
Author

binsee commented Sep 1, 2017

@hczhcz
This development branch is at an earlier time, and the implementation of npm run doc 's index.md does not contain the contents of message.js. The latest master branch has fixed the problem, and if I execute this command, I need to create a development branch based on the latest master branch and submit it.
I think that by @zixia merge this branch, the implementation of this order is more time-saving.

Or I can submit a new development branch that invites more than three members to review.

Please tell me which way to choose? 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.

Ok. Please go ahead. It is not necessary to create another PR, just take care when merging. (Since the code has changed much when this PR is pending, there probably be a lot of things to do in the merging process...) Anyway, do not worry about that :D Thank you for your contribution!

@huan
Copy link
Member

huan commented Sep 1, 2017

Great! Thanks all for the time of reviewing the code, appreciate it!

@huan huan merged commit 4957489 into wechaty:master Sep 1, 2017
@binsee binsee deleted the add-transpond-msg branch September 2, 2017 04:39
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.

None yet

5 participants