Opened 4 years ago

Closed 4 years ago

#2730 closed defect (fixed)

Value of notification key "inactive" is ignored

Reported by: Ross Assignee:
Priority: P4 Milestone:
Module: Sitescripts Keywords: goodfirstbug
Cc: fhd, sebastian, trev Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://github.com/adblockplus/sitescripts/pull/3

Description

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".

Change History (7)

comment:1 Changed 4 years ago by Ross

  • Summary changed from Inactive notification key value is ignored to Value of notification key "inactive" is ignored

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.

comment:2 Changed 4 years ago by Ross

  • Blocking 2162 removed
  • Tester set to Unknown

comment:3 Changed 4 years ago by sebastian

  • Cc fhd sebastian added
  • Keywords goodfirstbug added
  • Priority changed from Unknown to P4
  • Ready set

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.

comment:4 Changed 4 years ago by trev

  • Cc trev added

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.

comment:5 Changed 4 years ago by sebastian

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:6 Changed 4 years ago by abpbot

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

comment:7 Changed 4 years ago by sebastian

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.