Fix utf-8 encoding #5096

[mmitropoulou]

Hello, this possibly fixes #5096

Fixes #5096, closes #5506

[wRAR]

Can you please also add tests and/or modify the existing ones so that this change is tested properly?

[mmitropoulou]

Hi @wRAR. I changed the positioning of set_charset method for mails with attachments (it is now similar to how it as was at the begining) and removed it for mails without attachments (it is called anyway from the get_payload method if you pass a value for charset) because i feel like this is a better approach. I didn't create new tests or modify the tests a lot, because i feel that is not needed (everything eg. charset, body etc is already tested and I didnt add anything new that needs testing). I would like to hear your thoughts on it and get feedback. Thank you!

[codecov]

Codecov Report

Merging #5118 (bec25e0) into master (00527fd) will not change coverage.
The diff coverage is 100.00%.

❗ Current head bec25e0 differs from pull request most recent head 7ad7682. Consider uploading reports for the commit 7ad7682 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5118   +/-   ##
=======================================
  Coverage   88.92%   88.92%           
=======================================
  Files         163      163           
  Lines       11538    11538           
  Branches     1877     1877           
=======================================
  Hits        10260    10260           
  Misses        969      969           
  Partials      309      309           

Files Changed	Coverage Δ
scrapy/mail.py	77.77% <100.00%> (ø)

[Gallaecio]

I didn't create new tests or modify the tests a lot, because i feel that is not needed (everything eg. charset, body etc is already tested and I didnt add anything new that needs testing).

If you fixed #5096, then that’s something new that needs to be tested. You should add/modify tests as needed so that, if you reverted your fix, tests would now break.

On a separate note, I wonder if msg.set_charset(charset) is needed at all after your change. When there are attachments, the message body is already added with a charset parameter specified. Does removing msg.set_charset(charset) make any difference provided you keep passing the charset when adding the payload text without attachments?

[wRAR]

Thanks! I've checked the change with my original use case and it fixes it. I've also checked that the changed test doesn't pass without the code changes, not sure if we need to add an explicit check that the body is base64-encoded, though we don't do that in the other test so that's fine to leave as is.

[wRAR]

If you fixed #5096, then that’s something new that needs to be tested. You should add/modify tests as needed so that, if you reverted your fix, tests would now break.

The test change covers it, because of the added decode=True.

I wonder if msg.set_charset(charset) is needed at all after your change. When there are attachments, the message body is already added with a charset parameter specified. Does removing msg.set_charset(charset) make any difference provided you keep passing the charset when adding the payload text without attachments?

Removing breaks the message, as it now lacks Content-Type and Content-Transfer-Encoding top-level headers.