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 breaking on HTML email without images #22143

Merged
merged 4 commits into from Mar 19, 2019

Conversation

Projects
None yet
5 participants
@dagobert
Copy link
Contributor

commented Mar 17, 2019

Breaking Change:

no

Description:

If using html emails with no images the code breaks since it is not tested for empty (uninitialized) key images.

Related issue (if applicable): fixes #22142

Example entry for configuration.yaml (if applicable):

- alias: notify_test
  initial_state: 'on'
  trigger:
      platform: time_pattern
      minutes: '/2'
  action:
      service: notify.smtp
      data_template:
          title: >
              [ShuFu] Test
          message: |
              Test text
          data:
              html: >
                  <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//DE" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
                  <html lang="de" xmlns="http://www.w3.org/1999/xhtml">
                  <head>
                  </head>
                  <body>
                  TEST html
                  </body>
                  </html>

Checklist:

  • [x ] The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • [x ] There is no commented out code in this PR.
Fix breaking on HTML email without images
If using html emails with no images the code breaks since it is not tested for empty (uninitialized) key images.
name = os.path.basename(atch_name)
try:
with open(atch_name, 'rb') as attachment_file:
attachment = MIMEImage(attachment_file.read(), filename=name)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 17, 2019

line too long (81 > 79 characters)

except FileNotFoundError:
_LOGGER.warning("Attachment %s [#%s] not found. Skipping",
atch_name, atch_num)
if images is not None:

This comment has been minimized.

Copy link
@cgtobi

cgtobi Mar 17, 2019

Collaborator

This should be enough.
if images:

This comment has been minimized.

Copy link
@dagobert

dagobert Mar 17, 2019

Author Contributor

It is best practice and recommended to use is not None (see Python docs)

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

Also, beware of writing if x when you really mean if x is not None -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

This comment has been minimized.

Copy link
@cgtobi

cgtobi Mar 18, 2019

Collaborator

Thanks for pointing that out.

@awarecan

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

You can change line 157 to

message, images=data.get(ATTR_IMAGES, []))
@dagobert

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

You can change line 157 to

message, images=data.get(ATTR_IMAGES, []))

Thank for this tip. Unfortunately you referenced the wrong line. I had to change line 154 . 157 has / should stay the same since it covers the option of having images without html. If we change it also in that line we allow data key without any subkey such as html or images. That could be the of the main preference developers to avoid errors in the log but imho it is not necessary because it does not make to have data without any subkey.

I will change the pull request.

Implemented suggested better solution
Better solution to allow data -> html email without images.
@awarecan

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

I think we better guard both HTML and multi part message usage.

Protecting data -> without html key from crashing
If the data key does not contain the html key, sending the email would crash this "script". Preventing this by returning an default empty array.
@dagobert

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I think we better guard both HTML and multi part message usage.

After rethinking it, I agree.

@awarecan awarecan merged commit db07e45 into home-assistant:dev Mar 19, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 93.683%
Details

@ghost ghost removed the in progress label Mar 19, 2019

@dagobert dagobert deleted the dagobert:patch-2 branch Mar 20, 2019

@balloob balloob referenced this pull request Apr 3, 2019

Merged

0.91.0 #22688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.