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

Empty watchlist fixes #575

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Empty watchlist fixes #575

merged 3 commits into from
Jun 18, 2024

Conversation

dadak-dom
Copy link
Collaborator

While I was poking around with testing our web crawler, I discovered a bug where, if a user decided to input a Street Address to their watchlist that didn't include a region or zip code (i.e., they only input the city, which is the bare minimum for an entry), then the extension would get flooded with false-positive results, like so:

Thankfully, I just had to make sure that empty regions and zip codes weren't being added to the list of keywords to look out for. I've also included some fixes for how the watchlist is displayed when only a city is the input to the Street Address. @atlasharry, I think this is a good chance for you to take a first look at the extension code. I've done a fair amount of testing on these changes and I'm fairly confident they'll work, but I'd like for someone else to check just in case.

@dadak-dom dadak-dom added the bug Something isn't working label Jun 13, 2024
@dadak-dom dadak-dom requested a review from atlasharry June 13, 2024 19:05
@dadak-dom dadak-dom self-assigned this Jun 13, 2024
@SebastianZimmeck
Copy link
Member

Good find, @dadak-dom!

I believe we had an earlier conversation about this point, though, I am not 100% sure if it relates to the same point. You can check the issue here. I am good with your change, especially, since it decreases false positives. But also take a look at the previous issue and check if it is related and your change does not revert back something we had considered problematic (or your point is more important).

@dadak-dom
Copy link
Collaborator Author

Good point, @SebastianZimmeck. I believe my changes are in line with what Joe was going for, since they don't remove the ability for the user to set a null address/region/zip code 👍 .

@SebastianZimmeck
Copy link
Member

Sounds good, @dadak-dom!

@SebastianZimmeck
Copy link
Member

Can you review this PR, @atlasharry?

@atlasharry
Copy link
Member

Sorry @dadak-dom @SebastianZimmeck , I did not receive this notification until I manually browsed this PR. I will double-check my settings and make sure I will not miss your message again. I will go through your PR right now!

@SebastianZimmeck
Copy link
Member

Sorry @dadak-dom @SebastianZimmeck , I did not receive this notification until I manually browsed this PR. I will double-check my settings and make sure I will not miss your message again. I will go through your PR right now!

No worries!

Copy link
Member

@atlasharry atlasharry left a comment

Choose a reason for hiding this comment

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

excelllent!

@dadak-dom dadak-dom merged commit 4ca0fbe into main Jun 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants