Value of notification key "inactive" is ignored

[Ross]

Environment

ABP 2.6.9.3953
Firefox 38.0.5 / Windows 8.1 x64

How to reproduce

1. Set up the notification below on the server.
2. In Firefox, navigate to about:config.
3. Reset the adblockplus.notificationdata key.
4. Restart Firefox.
5. Wait one minute for the notification data to be fetched.
6. Observe the notification is not included (as expected).
7. Change "inactive = yes" to "inactive = no" in the notification file and recommit.
8. Repeat Step 2 to Step 5.
9. Observe the notification is still not included in the response (not expected).

Test data

  inactive = yes
  severity = information
  target = extension=adblockplus
  title.en-US = Test notification
  message.en-US = Test notification with link
  links = adblock_browser_android_beta_community

Observed behaviour

If the word "inactive" is on a line of a notification, it is made inactive, regardless of if the value is set to yes or no.

Expected behaviour

The notification file already present on the server when testing started had "inactive=yes" implying that "inactive=no" should work too. This would also make it match the format of other key/values (instead of just being a "keyword".

[Ross]

Reading through the code this is working as expected (if "inactive" in file, notification is inactive) however it's confusing at first and as already mentioned, doesn't match the format of the other keys.

[sebastian]

The code is infact only checking for the key inactive being present, ignoring it's value. While it's probably not too important, I agree that it would be nice to check the value as well.

BTW, any reason we don't use ​ConfigParser to parse the file? Then we could simply use ConfigParser.getboolean() here.

[trev]

IMHO, ConfigParser is the wrong tool here - not only is using it not exactly straightforward (no section header), it will also introduce plenty of unnecessary syntax complexity (variable expansion). Checking the value against "0", "no", "false" and "off" is fairly trivial without it as well.

[abpbot]

A commit referencing this issue has landed:
​https://hg.adblockplus.org/sitescripts/rev/bc0ce524663d