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

Fix bug that can't send file. #106

Merged
merged 6 commits into from
Jul 5, 2019
Merged

Fix bug that can't send file. #106

merged 6 commits into from
Jul 5, 2019

Conversation

su-chang
Copy link
Member

@su-chang su-chang commented Jul 4, 2019

Fix the bug that can't send file. #91

su-chang added 3 commits July 4, 2019 17:56
Change WebMediaType
1 -> pic
2 -> video
4 -> doc
and deprecate Audio.
Fix the bug 
If the file.size > 1M, the uploadMedia() function will run error.
Split the buffer, and then uploadFile one by one.
@su-chang su-chang changed the title Fix bug Fix bug that can't send file. Jul 4, 2019
Simplify the logic code.
}
// use promise.all() make the former upload of this buffer quickly
Copy link
Member

Choose a reason for hiding this comment

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

You could do something like this here to get the last one:

const lastBuffer = bufferData.pop()
await Promise.all(bufferData.map(getMediaId))
const mediaId = await getMediaId(lastBuffer, bufferData.length)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

await Promise.all(funcList)
const lastOne = bufferData.length - 1
mediaId = await getMediaId(bufferData[lastOne], lastOne)
} catch {
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 we need to catch the error and log it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

su-chang added 2 commits July 4, 2019 20:43
1. graceful code
2. catch error log
upload media by order
Copy link
Member

@windmemory windmemory 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 for the fix! Approved.

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.

@su-chang @windmemory Thank you very much for the PR and the review! This bug had been a while and many developers will be benefited from your fix.

According to your code, it seems the most important part is to change the 1 to 'pic', 2 to 'video', etc.

Could you please add a link to the webwx-app code so that we could be able to make it clear, and make sure we are on the right direction.

The code can be linked from webwx-app-tracker repository from https://github.com/Chatie/webwx-app-tracker/blob/master/formatted/webwxApp.js.

It would be wonderful if you can also add the exact change commit that made this breaking change. You can find all the webwx-app changes from https://github.com/Chatie/webwx-app-tracker/commits/master

I'll merge this PR and make it published so that the community could test it after you finished adding the related links from the webwxApp.js and the commits.

Again, thank you very for the effects to make this puppet better, you did a great job!

@su-chang
Copy link
Member Author

su-chang commented Jul 5, 2019

@huan
I have found the change 1 to pic, 2 to video, 3 to doc relate to this commit. =>Commit Detail Code.

It means that:

r.MSGTYPE_IMAGE: n = "pic";
r.MSGTYPE_VIDEO: n = "video";
default: n = "doc"

and then

n will assign to mediatype.

About puppeteer, our WebMediaType is also be used to assign mediatype for upload media.

This commit be committed on 19 Nov 2018, but the problem cant send file in puppeteer appearance these days. so it makes me confuse.

BTW, I find split the uploading file into chunks when I find issue about can't send file, this PR also resolved this issue #12.

Thx for ur reading.

@windmemory
Copy link
Member

windmemory commented Jul 5, 2019

This commit be committed on 19 Nov 2018, but the problem cant send file in puppeteer appearance these days. so it makes me confuse.

I think the verification is on the server side, and during the debug phase, we saw that using different mediaType will result in different mediaId. The way of uploading the file is not the problem, the point is what mediaId is allowed to call the sendMedia endpoint. So my guess is that, WeChat updated their way of sending media file, but their api still allow the new mediaId and the old one. And recently, they add the validation for different mediaId.

@huan
Copy link
Member

huan commented Jul 5, 2019

@su-chang Thank you very much for the additional information and they are very valuable.

I'll merge this PR for now, and it would be fantastic if we could refactor it to use the factory code inside puppeteer.

Good work!

@huan huan merged commit 1b0269c into wechaty:master Jul 5, 2019
@huan
Copy link
Member

huan commented Jul 5, 2019

This PR should be published on v0.17.23 or later version.

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