-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(puppet-web): send any type file. #714
Conversation
Fix can't send pdf and more type file. Now, you can use `m.say(new MediaMessage(file))` send any type file. Fix wechaty#710
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @binsee, Thanks for fixing this issue!
Your code looks great, there's only one problem with the typing of any
. Please follow my reviews, I believe the one more job we need to do is to add an Interface
for the new MediaData
Please let me know if you have any problem, I will be able to merge this PR after you replace any
with the Interface
.
Thanks!
src/puppet-web/bridge.ts
Outdated
@@ -397,15 397,14 @@ export class Bridge { | |||
} | |||
} | |||
|
|||
public sendMedia(toUserName: string, mediaId: string, type: number): Promise<boolean> { | |||
if (!toUserName) { | |||
public sendMedia(mediaData: any): Promise<boolean> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a type like any
can make you feel very convenience at start, But it will cause problems in the future because it disables all the benefits from the static type checking.
So please do not use an any
type here. I have two suggestions:
- Keep the original style.
Then we will have a function like:
public sendMedia(
toUserName: string,
mediaId: string,
msgType,
fileName,
fileMd5,
fileSize,
fileType,
mmFileExt,
mmFileId,
)
It's very verbose, but it's more clear and prevent us to make mistakes in the future.
- Or do it the better way: create an interface for this data type.
interface MediaData {
toUserName: string,
mediaId: string,
msgType,
fileName,
fileMd5,
fileSize,
fileType,
mmFileExt,
mmFileId,
}
public sendMedia(mediaData: MediaData): Promise<boolean> {
I'll prefer the method 2, but you can use either of them, but not the any
one.
src/puppet-web/puppet-web.ts
Outdated
@@ -336,7 338,7 @@ export class PuppetWeb extends Puppet { | |||
} | |||
} | |||
|
|||
private async uploadMedia(mediaMessage: MediaMessage, toUserName: string): Promise<string> { | |||
private async uploadMedia(mediaMessage: MediaMessage, toUserName: string): Promise<any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same suggestion as the above review: do not use any
.
I think here need interface
too.
src/puppet-web/puppet-web.ts
Outdated
@@ -437,7 450,7 @@ export class PuppetWeb extends Puppet { | |||
}) | |||
if (!mediaId) | |||
throw new Error('upload fail') | |||
return mediaId as string | |||
return { mediaId, mediaData } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add mediaId into mediaData?
like:
Object.assign(mediaData, {
mediaId.
})
If we could do this, then here will be more clear, because we can return the interface of MediaData
here.
src/puppet-web/puppet-web.ts
Outdated
@@ -455,17 468,22 @@ export class PuppetWeb extends Puppet { | |||
destinationId = to.id | |||
} | |||
|
|||
const mediaId = await this.uploadMedia(message, destinationId) | |||
const { mediaId, mediaData } = await this.uploadMedia(message, destinationId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use one return value with an interface here instead of using {}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll make changes as you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code refactoring has been done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your pr! It helps a lot.
But I fill a little bit confused as I commented.
src/puppet-web/puppet-web.ts
Outdated
const msgType = UtilLib.msgType(message.ext()) | ||
|
||
mediaData.FileType = 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does 7
means here? Can it use enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference source: https://res.wx.qq.com/a/wx_fed/webwx/res/static/js/index_4a48aef.js
But the use is unknown, and I'll test it again
var d,
u = {
FromUserName: c.getUserName(),
ToUserName: e.ToUserName,
FileSize: e.size,
FileMd5: t,
FileName: e.name,
FileType: 7
};
if (i) {
var m = angular.extend(u, c.getBaseRequest());
d = angular.extend({}, u),
n({
method: "POST",
url: r.API_checkupload,
data: m
}).success(function(t) {
0 == t.BaseResponse.Ret ? (d = angular.extend(d, {
AESKey: t.AESKey,
Signature: t.Signature
}), e.Signature = t.Signature, a(e, t.MediaId, t)) : (e.MMSendMsg && (e.MMSendMsg.MMFileStatus = r.MM_SEND_FILE_STATUS_FAIL, e.MMSendMsg.MMStatus = r.MSG_SEND_STATUS_FAIL), alert(t.BaseResponse.ErrMsg))
}).error(function(t) {
e.MMSendMsg && (e.MMSendMsg.MMFileStatus = r.MM_SEND_FILE_STATUS_FAIL, e.MMSendMsg.MMStatus = r.MSG_SEND_STATUS_FAIL),
alert("上传失败")
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing, no valid use of the file type parameter has been found and has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
use: demo code: .on('message', async m => {
try {
const room = m.room()
console.log((room ? '[' room.topic() ']' : '')
'<' m.from().name() '>'
':' m.toStringDigest(),
)
if (/^([a-zA-z0-9]{1,5})$/i.test(m.content()) && !m.self()) {
const file = __dirname '/test.' m.content()
const pathInfo = path.parse(file)
const fileStat = fs.statSync(file)
if (fs.existsSync(file) && fileStat.isFile()) {
await m.say(`请求 ${m.content()} 类型文件`)
log.info('Bot', `REPLY: File[${pathInfo.base}] size: ${fileStat.size ? fileStat.size : 0}`)
await m.say(new MediaMessage(file))
} else {
await m.say(`请求 ${m.content()} 类型文件失败!没有此类型的文件。`)
}
}
} catch (e) {
log.error('Bot', 'on(message) exception: %s', e)
}
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the code reflecting, with the new interface MediaData
, it looks fantastic!
I have a tiny question for you, which had posted as reviews, but I think that's not a problem.
In order to get this PR merged, please get Approved from at least 3 reviewers.
I'll approve this PR now for my vote.
Thanks again for your great work, Cheers!
@@ -384,6 389,8 @@ export class PuppetWeb extends Puppet { | |||
const first = cookie.find(c => c.name === 'webwx_data_ticket') | |||
const webwxDataTicket = first && first.value | |||
const size = buffer.length | |||
const id = 'WU_FILE_' this.fileId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you save WU_FILE_fileId to id, but in the following code of formData
you use this.fileId
instead.
Is this a mistake, or by design?
src/puppet-web/puppet-web.ts
Outdated
const formData = { | ||
id: 'WU_FILE_1', | ||
id: this.fileId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here. fileId
or just id
? (const id = 'WU_FILE_' this.fileId
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a mistake and I will fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks!
Fix can't send pdf and more type file.
Now, you can use
m.say(new MediaMessage(file))
send any type file.Fix #710