663722 - The blocklist output is including severity="0" where it shouldn't be

Status: RESOLVED FIXED

(Toolkit :: Blocklist Policy Requests, defect, P2)

Description

[Dave Townsend [:mossop]]

Sometime on Friday the blocklist.xml results changed significantly. I'm assuming this is dur to switching to zamboni for the blocklist data. The data is incorrect though. It looks to me like it is including severity="0" everywhere where the old blocklist didn't include a severity attribute. This has the effect of changing all hard-blocks to not being blocks at all, also possibly causing bug 663676 though that may be a client issue too.

The diff we have seen is: http://hg.mozilla.org/mozilla-central/rev/e802d263c27e

We need to either fix this or revert it a.s.a.p.

Comment 1

[Wil Clouser [:clouserw]]

switch to zamboni has been reverted in production.  -> jbalogh for the code change

Comment 2

[David Spalding]

Attachment: blocklist.xml, pluginreg.dat, from system before the problem occurred

I have what appears to be an earlier blocklist.xml with tags

VERSIONRANGE SEVERITY=""

with values other than 0.

Comment 3

[Jeff Balogh (:jbalogh)]

Mossop: the correct behavior is to not include a severity attribute if severity==0, right?

Comment 4

[Dave Townsend [:mossop]]

If you don't include a severity attribute then we default to a severity of 3.

Comment 5

[Jeff Balogh (:jbalogh)]

https://github.com/jbalogh/zamboni/commit/771b18f

Comment 6

[Justin Scott [:fligtar]]

If severity is expected to be null now, the blocklist admin tool will allow the entry to be blank, yes?

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #5)
> https://github.com/jbalogh/zamboni/commit/771b18f

I question whether this is the right fix. The test there seems to verify that if you have a severity=0 in the database then it won't show up in the xml, but that doesn't seem right to me, if you don't include it in the xml then we'll assume it to be severity="3", quite a different behaviour to severity="0"

Comment 8

[Jeff Balogh (:jbalogh)]

(In reply to comment #7)
> (In reply to comment #5)
> > https://github.com/jbalogh/zamboni/commit/771b18f
> 
> I question whether this is the right fix. The test there seems to verify
> that if you have a severity=0 in the database then it won't show up in the
> xml, but that doesn't seem right to me, if you don't include it in the xml
> then we'll assume it to be severity="3", quite a different behaviour to
> severity="0"

severity=0 is in the database. The old PHP page did not output a severity attribute in this case.

I don't know what the different severity levels mean, I'm just trying to follow the old behavior and your comments:

(From comment #0)
> It looks to me like it is including severity="0"
> everywhere where the old blocklist didn't include a severity attribute.

Comment 9

[Dave Townsend [:mossop]]

Ok, then I'd say it might be better to convert all those severity=0 in the database to severity=3 which is what we actually interpret them as. That way if we actually want to use severity="0" in the blocklist.xml in the future (which can be used to mark plugins as outdated) it won't be crazy difficult.

Comment 10

[Justin Scott [:fligtar]]

I've been setting them as 0 in the past so that the blocklist wouldn't display them so that the file would be smaller. Converted them all to 3 would add to the output, unless we fix it so severity 3 is the same as not showing the severity.

Comment 11

[Dave Townsend [:mossop]]

I think it'll lead to less confusion (and less chance of problems) if the values in your DB match the values the client is interpreting them as. Hiding values of "3" is just fine.

Comment 12

[krupa raj[:krupa]]

(In reply to comment #11)
> Hiding values of "3" is just fine.

This is not happening. I added a blocklist item with severity= 3, and value of severity still shows up at https://addons.allizom.org/z/blocklist/3/%7Bec8030f7-c20a-464f-9b0e-13a3a9e97384%7D/5.0/Firefox/20110217/Darwin/en-US/release/Darwin/default/default

Reopening

Comment 13

[Jeff Balogh (:jbalogh)]

(In reply to comment #12)
> (In reply to comment #11)
> > Hiding values of "3" is just fine.
> 
> This is not happening. I added a blocklist item with severity= 3, and value
> of severity still shows up at
> https://addons.allizom.org/z/blocklist/3/%7Bec8030f7-c20a-464f-9b0e-
> 13a3a9e97384%7D/5.0/Firefox/20110217/Darwin/en-US/release/Darwin/default/
> default
> 
> Reopening

Please don't. That is not a requirement.

Comment 14

[Justin Scott [:fligtar]]

It is. We have never shown hardblock severity in the blocklist because it's unnecessary bandwidth and I don't want to start now.

Comment 15

[Jeff Balogh (:jbalogh)]

(In reply to comment #14)
> It is. We have never shown hardblock severity in the blocklist because it's
> unnecessary bandwidth and I don't want to start now.

You're talking about 5 bytes after gzip.

Anyways, you can set severity to null and it won't show up.

https://github.com/jbalogh/zamboni/commit/8cfe09b