Opened 6 months ago

Closed 6 months ago

Last modified 3 months ago

#7449 closed change (fixed)

Move compareVersion function into separate module and export

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: goodfirstbug
Cc: mjethani, sebastian, hfiguiere Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/51

Description (last modified by mjethani)

Background

To implement a workaround for #7430 I would like to emit an event, but only for users running Firefox versions 66 or 67. To do that I need a way to compare info.platformVersion to those numbers. This logic has already been implemented in adblockpluscore/lib/notification.js, but is not exported for use elsewhere.

What to change

  • Move the compareVersion function into its own module lib/versions.js, rename it to compareVersions and export.
  • Add unit tests for the compareVersions function.
  • Import that function from lib/notification.js and adjust the calling code to use the new name.

Hints for testers

Set up a notification.json on your test server:

{
  "notifications": [
    {
      "type": "critical",
      "title": "Hello",
      "message": "I just want to say hello.",
      "targets": [
        {
          "applicationMaxVersion": "75.0.3759.4"
        }
      ]
    }
  ],
  "version": "201904101149"
}

(Note: I'm not sure how you normally test notifications, but I imagine you redirect the request to https://notification.adblockplus.org/notification.json to your own server somehow.)

The value of applicationMaxVersion should be set to be greater than or equal to the version number you see in the About dialog of your browser.

Now do a fresh install of the extension, wait for a minute, and see that you get notified with the message I just want to say hello.

Next, edit the notification.json to make the value of applicationMaxVersion less than your browser's version number; then do a fresh install of the extension again, wait for a minute, and see that you do not get notified with the same message.

This should be enough, but you could also experiment with applicationMinVersion in the same way if you like, try different values (less than the browser version, greater than the browser version, exactly equal to the browser version), values with only the major version number, values with only the major and minor version numbers, and so on.

It would be a good idea to test this on all supported platforms since the formats of the version numbers may differ.

Change History (14)

comment:1 Changed 6 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 6 months ago by mjethani

I wouldn't mind if this went into its own lib/versions.js module with an accompanying test/versions.js test file.

While we're at it, maybe it's a good opportunity to rename the function to compareVersions() since it actually compares two versions against each other.

comment:3 Changed 6 months ago by mjethani

  • Cc hfiguiere added

comment:4 Changed 6 months ago by kzar

  • Description modified (diff)
  • Keywords goodfirstbug added
  • Summary changed from Move compareVersion function into coreUtils module and export to Move compareVersion function into separate module and export

Sounds good to me, updated the description.

comment:5 Changed 6 months ago by kzar

  • Description modified (diff)

comment:6 Changed 6 months ago by mjethani

  • Owner set to mjethani

comment:7 Changed 6 months ago by mjethani

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

comment:8 Changed 6 months ago by sebastian

Note that the compareVersion() function were previously part of the public API, but we stopped exporting it, in order to simplify things. I don't think that at the time we need it for anything outside of the notifications module.

For the workaround mentioned (#7430) where we want to target only specific Firefox versions, we will be just using parseInt() which is good enough when only checking for the major version.

comment:9 Changed 6 months ago by mjethani

Does this mean that #7430 does not depend on this change?

comment:10 Changed 6 months ago by kzar

Yea, since we could go ahead with #7430 without this change I didn't mark this issue as blocking it and I added the keyword "goodfirstbug" in case a new contributor wanted to pick it up.

Anyway, you already did a pretty good job of exporting the function and I'm in favour of the change. Even if we don't use it for #7430 it's the kind of function that will surely be useful in the future IMO. Even better now that you've added a bunch of great tests too, surely this is a good thing?

comment:11 Changed 6 months ago by mjethani

Yes, I agree it's a good idea to move the function to its own module and add some tests. For reference, we might even need it for snippets (#7451).

comment:12 Changed 6 months ago by abpbot

A commit referencing this issue has landed:
Issue 7449 - Move compareVersion() to lib/versions.js

comment:13 Changed 6 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described. Tested with a variety of targets and version numbers.

ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809

Note: See TracTickets for help on using tickets.