1263198 - Do not automatically set tracking flags status-firefoxXX affected for bugs filed under SeaMonkey

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Philip Chee]

Possible regression of Bug 1170179

Comment 2

[David Lawrence [:dkl]]

Attachment: 1263198_1.patch

Ok strangle this seems like a bug in the older versions of Template::Toolkit. For some reason, somevar.lower() is returning an empty string on tt2.22 (RHEL6) but works on newer versions. This was cause the auto_affected hash to have a single empty '' hash key which always matched product.name.lower (which also is empty '') which caused all products where the cf_status_firefox[highest] was visible. 

I have removed the user of lower as I am not sure we even need to do this as we will not have two same product names of differing case in BMO (at least not on MySQL). glob can bring me up to speed on why we needed to lower() when he originally wrote this.

Otherwise my patch works in the different scenarios. 

dkl

Comment 3

[:glob ✱]

Comment on attachment 8747959 [details] [diff] [review]
1263198_1.patch

Review of attachment 8747959 [details] [diff] [review]:
-----------------------------------------------------------------

.lower is there because product and component names are case insensitive in bugzilla.
without it you need to ensure the products and components in auto_affected are exactly the same as the db.

Comment 4

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #3)
> Comment on attachment 8747959 [details] [diff] [review]
> 1263198_1.patch
> 
> Review of attachment 8747959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> .lower is there because product and component names are case insensitive in
> bugzilla.
> without it you need to ensure the products and components in auto_affected
> are exactly the same as the db.

Sure. But I highly doubt someone will be changing the case of a product name without also changing the name itself somehow. And we already have hard coded product names in extensions/BMO/lib/Data.pm that we do not first lowercase before comparison. Not saying that that is an excuse not to do the check. Will need to figure out how to do lower in a different way since tt2 is obviously not working as expected. Maybe a regex transform. Also maybe we should move auto_affected to a constant outside of the template.

dkl

Comment 5

[:glob ✱]

sorry i wasn't clear before .. i think not using .lower is ok; i was just explaining why it was there initially.

> Will need to figure out how to do lower in a different way since tt2 is obviously not working as expected.

create a case-insensitive string comparison template vmethod?
eg. https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/BugModal/Extension.pm#L142

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8747959 [details] [diff] [review]
1263198_1.patch

Review of attachment 8747959 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

This is fine. If you'd like to instead define a .lower() vmethod that might be useful.
Are we using tt2 .lower() elsewhere in the code?

Comment 7

[David Lawrence [:dkl]]

Went with the custom lower() vmethod which also fixed this issue with much smaller amount of code change.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   5a9a4e8..a07ff20  master -> master

dkl

Comment 9

[David Lawrence [:dkl]]

This has been pushed to production.

dkl

Comment 10

[rsx11m]

I've filed a couple of new SeaMonkey bugs since, the firefox-* flag didn't reappear. Thanks!