285689 - quip_list_entry_control conversion code is not converting the enablequips 'off' value

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P2)

Description

[Cédric Caron]

The code in Bugzilla::Config::UpdateParams() to convert from enablequips to 
quip_list_entry_control is not converting the 'off' value of enablequips (Bug 
41972)

Comment 1

[Shane H. W. Travis]

I was fairly certain that I had thought this through, and that I hadn't just 
ignored this possibility, but I couldn't remember my reasoning so I wondered if 
I had just forgotten.

Today, I remembered.

The call to Bugzilla::Config::UpdateParams takes place on line 1293 of (today's 
version of) checksetup.pl. The call to $dbh->bz_setup_database() doesn't take 
place until line 1629.

So, at the time that Parameters are updated, the settings table has not yet 
been created, which means it's not possible to do something like set the quips 
entry in the setting table to 'off/disabled' based on the Param because that 
entry doesn't exist yet. By the time it does exist, the Param no longer exists; 
it has been removed from the data/params table and moved to old-params.txt. 

I suppose it might be possible to retrieve it from there and do something with 
it, but as far as I know the customary practice is simply to relnote things 
like this so admins are aware to look out for it. 

I've added mkanat to the cc: list in the hopes of getting his input, since he's 
the current checksetup guru. :)

Comment 2

[Cédric Caron]

OK but with the current code if enablequips is set to off 
quip_list_entry_control is not initialised corectly !!!

Comment 3

[Max Kanat-Alexander]

Hrm... Yeah, that's an interesting dillema.

I'm not actually certain that params have to exist in order to set up the 
database -- that depends only upon localconfig. However, I suspect that the 
dependencies of Bugzilla::DB might pull in Config or Auth somewhere, so we 
can't actually create the database before params are ready. (Since, as I 
recall, Config does its initialization code outside of any subroutine.)

We might want to investigate eventually moving the param migration code to 
after the initial table creation. After all, if we ever move any of the params 
to the database, we'll have to do that anyway.

For now... you'll probably want to read it out of old-params, if that's 
possible.

Comment 4

[Shane H. W. Travis]

Attachment: Code patch for tip

Aaah, Cedric, I finally understand what you are saying! 

Yes, Max, this would be nice if we could read from the old-params and put it in
the database... but what Cedric is saying (I finally understand) is that if
Param(enablequips) == 'off' prior to the upgrade, then we simply don't handle
it!

Reading it out of old-params and filling in the tables appropriately would be
nice, but it's not critical; that's definitely an enhancement. As described,
this is definitely a bug, and needs fix ASAP.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 177374 [details] [diff] [review]
Code patch for tip

Ohhhhh. :-) OK!

Comment 6

[Shane H. W. Travis]

Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.37; previous revision: 1.36
done