584846 - Blocklist script does not properly handle version=0

Status: RESOLVED WORKSFORME

(Toolkit :: Blocklist Policy Requests, defect)

Description

[Michael Morgan [:morgamic]]

If minVersion or maxVersion is set to 0, blocklist.php resolves a 0 as being empty, which corrupts the XML output of the script.

For example, if a blocklist item has a version range that starts with 0 and ends with 4.0b1, the entire range will not be displayed.  Why:
- in order to view an item versionRange, both values need to be set and non-zero
- the PHP function empty() which is currently used for versions evaluates "0" as being empty and/or false
- consequently, the entire versionRange for that item will not be shown

Historically, the workaround hack for this was to use a space for the minVersions that should syntactically be 0.  (I am not joking...)

Looking into this in bug 548443 let rob strong to comment (correctly) that " " should actually be 0.  The behavior in the browser is identical, but a numerical minVersion is proper.

So, we should change the blocklist service to properly check for versions that are set to 0.

Comment 1

[Michael Morgan [:morgamic]]

Attachment: v1, fixes the stupidness, but looks stupid

This does the trick, but I hate it.  Diff of prod output is as follows:

morgamic@carebear:~/Desktop$ diff -u newblocklist.xml blocklist.xml 
--- newblocklist.xml	2010-08-05 11:55:35.000000000 -0700
+++ blocklist.xml	2010-08-05 11:41:24.000000000 -0700
@@ -15,19 +15,10 @@
       <versionRange severity="3"/>
     </emItem>
     <emItem id="[e-mail]">
-      <versionRange minVersion="0" maxVersion="6.9.8">
-        <targetApplication id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
-           <versionRange minVersion="3.7a1pre" maxVersion="*"/>
-        </targetApplication>
-      </versionRange>
-      <versionRange minVersion="2.1" maxVersion="3.3">
-        <targetApplication id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}">
-           <versionRange minVersion="3.0a1" maxVersion="*"/>
-        </targetApplication>
-      </versionRange>
+      <versionRange minVersion="2.1" maxVersion="3.3"/>
     </emItem>
     <emItem id="[e-mail]">
-      <versionRange minVersion="0" maxVersion="1.0.0.5"/>
+      <versionRange minVersion=" " maxVersion="1.0.0.5"/>
     </emItem>
     <emItem id="[e-mail]">
       <versionRange minVersion="2.0.5" maxVersion="2.0.5">
@@ -38,7 +29,7 @@
     </emItem>
     <emItem id="{2224e955-00e9-4613-a844-ce69fccaae91}"/>
     <emItem id="{3f963a5b-e555-4543-90e2-c3908898db71}">
-      <versionRange minVersion="0" maxVersion="8.5"/>
+      <versionRange minVersion=" " maxVersion="8.5"/>
     </emItem>
     <emItem id="{4B3803EA-5230-4DC3-A7FC-33638F3D3542}">
       <versionRange minVersion="1.2" maxVersion="1.2">
@@ -49,7 +40,7 @@
     </emItem>
     <emItem id="{8CE11043-9A15-4207-A565-0C94C42D590D}"/>
     <emItem id="{B13721C7-F507-4982-B2E5-502A71474FED}">
-      <versionRange minVersion="0" maxVersion="3.3.0.3970" severity="1"/>
+      <versionRange minVersion=" " maxVersion="3.3.0.3970" severity="1"/>
     </emItem>
     <emItem id="{E8E88AB0-7182-11DF-904E-6045E0D72085}"/>
   </emItems>

However, I am not happy with that.  Need to write tests for it so I am working on one of two outcomes:
1. fix the php tests so they work and accomodate the (min|max)Version=0 cases
2. write new Python tests and earn street cred but lose a couple days to learning curve
3. writing standalone Python tests that can be called from the command line using an arg for the docroot of test instance you want to point to

Right now I will probably do 1 or 3 since 2 would likely require a test rewrite which incurs a lot of overhead (incorporation of tests into zamboni, significant load testing, possible effing up of metrics via blocklist cookie rules, etc.).

I would, however, like to use fixtures to set up test data on the fly.  So fixtures + 3 might be a possibility.  Do you have ideas or suggestions on how to do this better?

Comment 2

[Michael Morgan [:morgamic]]

s/test rewrite/service rewrite/

Comment 3

[Michael Morgan [:morgamic]]

For the record, the test data is here:
http://svn.mozilla.org/addons/trunk/site/app/tests/data/remora-test-data.sql

The existing services tests are located here:
http://svn.mozilla.org/addons/trunk/site/app/tests/services/blocklist.test.php

Comment 4

[Robert Strong (they/them - no direct email)]

fun...

Reference bug is actually bug 578443.

I think it might be a good thing to put some checks for the values as well. For example, a maxVersion that starts with 0 (more technically correct would be starts with 0 and isn't followed by a number greater than 0 but I don't see allowing that as valuable) should be rejected.

Comment 5

[Michael Morgan [:morgamic]]

Yeah, bug 578443.   My bad.

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

This should already be fixed. There are tons of blocks that use 0 for min versions.