moralmunky / Home-Assistant-Mail-And-Packages

Home Assistant integration providing day of package counts and USPS informed delivery images.
MIT License
607 stars 77 forks source link

ISSUE: amazon_fwd returns "value should be a string" on existing valid entry #937

Closed disconn3ct closed 3 weeks ago

disconn3ct commented 2 months ago

Describe the bug When reconfiguring the plugin, the first page worked fine but the second wouldn't submit. The existing Amazon email address triggered an error. (It looked like it was objecting to the list, per screenshot, but the error is one field below.)

Making a noop edit to the existing email address allowed it to submit successfully.

Environment (please complete the following information):

Logs value should be a string From the console: {"errors":{"amazon_fwds":"value should be a string"}}

Nothing in the HA logs.

Screenshots image

Additional context Thinking it was referring to the list, I toggled off each provider one at a time with no affect. Noop edit of the email address worked (add a space, remove a space, hit submit)

firstof9 commented 1 month ago

I believe this should be solved in the latest beta.

disconn3ct commented 1 month ago

Using b4.

I went through the reconfig and it did not throw any errors, but the email is showing as (none). It accepts the correct address but when I go through reconfigure again, it has reset to (none).

It also isn't detecting today's Amazon delivery, but I am not positive that was working 100% to begin with.

image

firstof9 commented 1 month ago

Are you entering an amazon email address in the field?

disconn3ct commented 1 month ago

Yes. It takes the address that Amazon initially sends to, before any forwarding, right?

firstof9 commented 1 month ago

You don't enter @amazon.com email addresses into the forwarder setting.

disconn3ct commented 1 month ago

Is it not supposed to be my address? Which @amazon address should I use?

Oh. You are doing something that has been a bad idea for literal decades: do not make assumptions about email addresses.

https://github.com/moralmunky/Home-Assistant-Mail-And-Packages/blob/master/custom_components/mail_and_packages/config_flow.py#L59

Please explain why "dis@amazonpotato.com" is invalid. Or "amazon@example.com".

firstof9 commented 1 month ago

Is it not supposed to be my address?

IF you are forwarding amazon emails to another email address AND it doesn't happen to be gmail, you'd enter whatever email address the amazon emails are being forwarded from.

Which @amazon address should I use?

None.

"amazon@example.com"

This would be fine.

Please explain why "dis@amazonpotato.com" is invalid.

The validation will need to be adjusted.

disconn3ct commented 1 month ago

"amazon@example.com" is exactly the form that doesn't work. Are there any tests that exercise that ill-advised validation?

firstof9 commented 1 month ago

Yes there's a test for using an "@amazon.com" email in the forwarder, it is supposed to be throwing an error message at you however.

https://github.com/moralmunky/Home-Assistant-Mail-And-Packages/blob/dev/tests/test_config_flow.py#L1481-L1532

disconn3ct commented 1 month ago

Is there a test that other forms of the match string still work? Because amazon@example.com explicitly does not. (And there was no error, but that is tangential to 'validating' email addresses.)

If you are going to insist on doing email validation, you should test success/edge cases too. (One of the examples from that article is potato\@amazon@example.com. Anything and everything is permitted prior to the @. Including more @s.)

firstof9 commented 1 month ago

I'm working on revising the validation portion as well as adding more tests.

firstof9 commented 1 month ago

Please give 0.4.0b6 a try see if that fixes your issue.

disconn3ct commented 1 month ago

Sorry for the delay. b6 (and b7) doesn't show the field at all: image

firstof9 commented 1 month ago

It should be the screen after this one where the Amazon setup is.

disconn3ct commented 1 month ago

Weird it says 2 of 2. Still no error, and it still comes up null if I go back in after setting it.

firstof9 commented 1 month ago

Nothing in the home-assistant.log? I added error messages when things are not as they should be.

firstof9 commented 1 month ago

Oh I see where the issue is, I'm blind. Give me a few.

firstof9 commented 1 month ago

0.4.0b8 should solve the missing forwards from the reconfigure.

disconn3ct commented 1 month ago

That seems to have fixed the disappearing values. It accepts amazon@example.com and leaves it. During configuration, it logs ERROR (MainThread) [custom_components.mail_and_packages.config_flow] Error creating folder array trying period

Putting in valid addresses from the article I linked throws errors, but the error is at least visible.

What is the goal of trying to predict every valid email address? The RFC explicitly says that there is no way to validate the local (user) portion. Any attempts to do so are, by definition, incorrect. There are about 10 valid examples in there that do not pass your validation.

Edit to add: today's messages haven't come in yet so I don't know if it is working or not

firstof9 commented 1 month ago

ERROR (MainThread) [custom_components.mail_and_packages.config_flow] Error creating folder array trying period

This is from the list of mailboxes.

What is the goal of trying to predict every valid email address?

I am not attempting to, only filter out anyone trying to use the amazon domain.

disconn3ct commented 1 month ago

I am not attempting to, only filter out anyone trying to use the amazon domain.

Then why does it look at the local user portion? That is not related to the domain and it is explicitly incorrect to parse it at all unless you are the receiving mailserver. At least it should be a warning. The user knows more about their email than you do. You should let them define it. (Have you never met someone who works at amazon? Their emails, stunningly, tend to end in @amazon.com)

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.