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 attachment type (#175) #184

Merged
merged 18 commits into from
Dec 13, 2021
Merged

Fix attachment type (#175) #184

merged 18 commits into from
Dec 13, 2021

Conversation

System233
Copy link
Contributor

Fix #175
ref #178

Sorry about that, #178 cannot be opened again.

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 very much for the fix!

Please make the CI green first, then I'll be able to add some improvements on it under the protection of the unit tests.

const mockedResUploadMedia = {
MediaId: 'MediaId',
}
const getExtName = (filename: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why we re-define this function, instead of using the puppet.getExtName() for testing?

const getExtName = (filename: string) => {
return extname(filename).slice(1)
}
const extToType = (ext: string): WebMessageType => {
Copy link
Member

@huan huan Dec 13, 2021

Choose a reason for hiding this comment

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

Why we re-define this function, instead of using the puppet.extToType() for testing?

I think we should test those methods on the puppet, please remove the defines in the tests, and use the existing code in puppet instead.

This PR will be ready to be merged after we finished improving testing codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,I just want to prevent extToType and getExtName from being accidentally modified

@huan
Copy link
Member

huan commented Dec 13, 2021

It is not a best practice for using force push when updating the PR.

I have pushed some modifications in this PR and has just been overwritten by your force push.

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.

Please let me know when you believe this PR is ready to be merged, and I'll add some comments to the code then merge it.

Thank you very much for all efforts for fixing and improving the puppet wechat, appreciate it!

@System233
Copy link
Contributor Author

It is not a best practice for using force push when updating the PR.

I have pushed some modifications in this PR and has just been overwritten by your force push.

Sorry, I am merging commits to make diff clearer.

@System233
Copy link
Contributor Author

System233 commented Dec 13, 2021

Thanks for your advice and help.
In real device test process, the patch works correctly, but sometimes getaddrinfo ENOTFOUND exception occurs, it may another string concat error.

Error: getaddrinfo ENOTFOUND wx2.qq.comundefined
    at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26)

image

Probably wx2.qq.comundefined comes from here:

const r = {
headers,
json: checkData,
url: `https://${hostname}${checkUploadUrl}`,
}

@huan huan merged commit 073e3f8 into wechaty:main Dec 13, 2021
@huan
Copy link
Member

huan commented Dec 13, 2021

Thank you very much for sending this PR for fixing the attachments sending bug!

The code will be published on [email protected] or above versions.

@huan
Copy link
Member

huan commented Dec 13, 2021

BTW: this PR reverted some changes from #106 made by @su-chang

I'd like to suggest @su-chang review the code and make sure the new modification is the right way to improve the old PR.

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.

PDF file sent as unknown file
2 participants