Fix Watchlist Notification Functionality

[JoeChampeau]

The bell buttons that allow you to enable or disable notifications for watchlist items don't seem to save your preferences if you edit an item, and newly made watchlist terms cannot have notifications disabled.

[JoeChampeau]

Should be fixed.

Although, this raises another issue: while ensuring notification preferences get saved even when a user edits their IP address, I noticed that the extension always auto-populates the watchlist with the correct IP and corresponding location if the IP address is ever changed or deleted.

If the intention for those two fields (specifically the IPInfo-generated IP address and street address fields) is for users to not be able to manually edit them, would it make sense to disable the edit/delete dropdown?

[SebastianZimmeck]

If the intention for those two fields (specifically the IPInfo-generated IP address and street address fields) is for users to not be able to manually edit them, would it make sense to disable the edit/delete dropdown?

Good point, @JoeChampeau! I just tried it, and that is indeed true.

I think both the IP address as well as the street address should be editable (or, at least, the IP address should be).

It seems to me that the root cause of this problem is that originally we had the functionality the way that we did not automatically update the IP address and street address. But then the question was that it does not make much sense that people get notified of IP addresses that they no longer have when they connect to a different WiFi. So, we decided to automatically detect the IP address and change the watchlist value to look for the new value, e.g., that is what @JustinCasler implemented (#391).

So, what I think is happening is that once the IP address (even the same one as before) is detected again, it overwrites the manually entered value (and the same is true for the street address).

So, the bottom lines is that we would need to find a balance between on one side updating the IP address (and street address) automatically as necessary but not overwrite the user's choices if that is what they want.

My sense is that for IP addresses we can disable the manual editing of the value. So, whatever is detected is used as watchlist value to look for.

For the street address, I am not sure. I would imagine that the manually entered value should stay as is, if in doubt.

@JoeChampeau (and everyone), what would make most sense of you if you were a user?

[JoeChampeau]

I agree that the IP address probably shouldn't be able to be changed by the user. Based on Justin's reasoning in the issue you linked, it seems like the primary issue centered on the possibility of IPinfo inferring an incorrect location from the user's IP address.

Users probably should be able to correct this, although that does raise the question that if IPinfo determines an incorrect location, are tracking services likely to do the same? In that case, the original, maintaining the original, erroneous value may be more helpful to determine if a website is actively trying to track someone's location (i.e. if I'm in Middletown and IPinfo reads Portland , other services may also be reading Portland and, so, manually setting it to Middletown may cause PP to miss those requests), but then, of course, the user is probably less likely to care about tracking if it involves wrong information.

We could implement a system which allows location edits but issues a popup to ask the user to confirm automatic changes to the location whenever their IP address changes, but that sounds a little cumbersome.

Maybe instead we maintain a separate, toggleable setting for automatic generation + disabled edits vs. no automatic generation + enabled edits, and have it set to the former by default?

[SebastianZimmeck]

Maybe instead we maintain a separate, toggleable setting for automatic generation + disabled edits vs. no automatic generation + enabled edits, and have it set to the former by default?

Yeah, that sounds like a good solution to me. It is (1) either completely automatic with automatic updates or (2) completely manual with manual updates.

Do you have this setting in mind for just street address or also IP address?

[JoeChampeau]

I suppose we might as well allow edits to the IP address as well, so long as the auto functionality remains the default option.

[SebastianZimmeck]

I suppose we might as well allow edits to the IP address as well, so long as the auto functionality remains the default option.

Sounds good!

[JoeChampeau]

New functionality has been pushed to the branch, and now the IPinfo data will be properly refreshed when the location data it generates is altered (previously, it would only refresh when it detected a change to the IP address).

As a quick aside, I noticed when editing location values in the watchlist that the form throws an error if you try to submit an address without a street specified, despite the fact that the IPinfo generated location never provides one. Does anyone know if this is intended functionality? Should we allow users to omit the street address?

[SebastianZimmeck]

Excellent, @JoeChampeau!

The bell buttons that allow you to enable or disable notifications for watchlist items don't seem to save your preferences if you edit an item ...

Just to confirm, this also covers the situation when you disable notifications for IP addresses, restart the browser, and now you start receiving IP address notifications again, right? I experienced that bug a few days ago.

[SebastianZimmeck]

Feel free to open a PR, @JoeChampeau. @dadak-dom can review (with @danielgoldelman's and @jjeancharles' help as necessary).

[JoeChampeau]

@SebastianZimmeck I'm having a hard time replicating the bug you mentioned. My hunch is that if you activate a VPN or move to a different network, your IP address will change and the notification settings you had set will be reverted when the IP address and location are updated.

That bug should be fixed in #549-fix-watchlist-functionality, but I'll continue doing testing to see if there's another culprit.

[SebastianZimmeck]

@SebastianZimmeck I'm having a hard time replicating the bug you mentioned.

Hm, OK, I just tried it, and, indeed, I could not get that behavior again either. I will an eye on it, but for the time being, we can move ahead.