Skip to content

Re-read last imap_email_content email when no change#36065

Merged
frenck merged 3 commits intohome-assistant:devfrom
isk0001y:feature/imap_email_content/nomessage1
May 25, 2020
Merged

Re-read last imap_email_content email when no change#36065
frenck merged 3 commits intohome-assistant:devfrom
isk0001y:feature/imap_email_content/nomessage1

Conversation

@isk0001y
Copy link
Copy Markdown
Contributor

Proposed change

In the Bug #35975 it is stated an issue that the state changes to Unknown after some time. This is actually not dependent on time, but on arrival of emails. So basically if no email arrives, the code will not find any unread email at all and default to returning None out of the read_next() method. However, this now is obviously undesired behavior if people decide to keep the emails in the mailbox.

This basically restores the original intent of staying in the previous state by trying to return the last detected email, if no change of emails occur at all, but supports still the deleted email usecase, because in that case email would become None, and state would turn to None again.

I assume that this matches more the initial intent of this piece of code

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

There is another change, which is related to the python imap library. If you for example archive emails, the uid() method returns an empty list. Looks like it was initially expected to return None, but to prevent subscript exceptions in the following line, I treat this empty list also as None.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@MartinHjelmare MartinHjelmare changed the title Try to fix #35975 - re-read last email when no change Re-read last imap_email_content email when no change May 24, 2020
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

It would be good to add explicit return None in the read_next method at all places where the method can exit.

Comment thread homeassistant/components/imap_email_content/sensor.py
@isk0001y
Copy link
Copy Markdown
Contributor Author

What is your proposal for reconnect case? It will also return None but theoretically it could be fixed in a better way. I am unsure what is the best way.

@MartinHjelmare
Copy link
Copy Markdown
Member

For now just return None at all exit places so return value is more clear. We can improve more later.

@isk0001y
Copy link
Copy Markdown
Contributor Author

The more I look at it, the more the method looks annoying...

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

imap_email_content set to unknown after 30 seconds

4 participants