936468 - Move OS: Windows 8 Metro to Windows 8.1

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Administration, task, P2)

Description

[Jim Mathies [:jimm]]

This was originally created to isolate metro related bugs from product firefox. Later added a new product, so this isn't needed anymore. We would like to delete this os moving any bug under it to os: Windows 8.1 (hopefully without generating any bugspam?).

This change was discussed on the metro public mailing list.

Comment 1

[:glob 🎈]

i've updated the field value so it's no longer an option.
we'll have to write a script to update the 1962 bugs currently using that o/s.

Comment 2

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

Attachment: bug-936468-v1.patch

This is decidedly a one-off script. Current behavior is to reset the OS field to 'Other', I'm not sure what else would be defined behavior.

Comment 3

[:glob 🎈]

Comment on attachment 8439730 [details] [diff] [review]
bug-936468-v1.patch

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

::: contrib/reorg-tools/remove-os.pl
@@ +8,5 @@
> +
> +use 5.10.1;
> +use strict;
> +use warnings;
> +use lib qw(. lib);

while most reorg script don't current do this, i think it's nicer to allow these scripts to work regardless of the cwd:

use FindBin '$RealBin';
use lib "$ReadBin/../..", "$RealBin/../../lib";

@@ +32,5 @@
> +
> +$dbh->bz_start_transaction;
> +
> +my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +my $field     = Bugzilla::Field->new({name => 'op_sys', cache => 1});

nit: add spaces after { and before }

i'd like to see a sanity check here that default_os exists in the db (stranger things have happened!).
check the result of Bugzilla::Field::Choice->type($field)->match({ value => $DEFAULT_OS })

@@ +35,5 @@
> +my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +my $field     = Bugzilla::Field->new({name => 'op_sys', cache => 1});
> +
> +my $bugs = $dbh->selectall_arrayref(q{SELECT bug_id FROM bugs WHERE bugs.op_sys = ?},
> +    { Slice => {} }, $os);

rather than slicing to a single element hashref, use selectcol_arrayref

@@ +42,5 @@
> +    my $bug_id = $bug->{bug_id};
> +    warn "Removing OS '$os' from $bug_id...\n";
> +    $dbh->do(q{INSERT INTO bugs_activity(bug_id, who, bug_when, fieldid, removed, added)
> +               VALUES (?, ?, ?, ?, ?, ?)},
> +                undef, $bug_id, 1, $timestamp, $field->id, $os, $DEFAULT_OS);

don't hard code the user-id.  load the nobody user explicitly with $user = Bugzilla::User->check({ name => '[e-mail]' })

@@ +63,5 @@
> +    remove-os.pl --os 'Windows 8 Metro'
> +
> +=head1 AUTHOR
> +
> +Dylan William Hardison <[e-mail]>

remove the author section

Comment 4

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

Attachment: bug-936468-v2.patch

And here is something I was working on when I was sick. Note that using FindBin under taint mode requires untainting $RealBin.

Comment 5

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

Attachment: bug-936468-v2.1.patch

Ignore the previous version, pretty sure I made a mistake there.

Comment 6

[:glob 🎈]

Comment on attachment 8454235 [details] [diff] [review]
bug-936468-v2.1.patch

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

::: contrib/reorg-tools/remove-os.pl
@@ +1,1 @@
> +#!/usr/bin/perl -wT

nit: don't need both -w and "use warnings"; remove -w from here.

@@ +11,5 @@
> +use warnings;
> +
> +use FindBin '$RealBin';
> +BEGIN { ($RealBin) = $RealBin =~ /^(.+)$/ }
> +use lib "$RealBin/../..", "$RealBin/../../lib";

as it's a command line tool with a single source of user input, i'm not convinced the acrobatics required to work around taint issues are worth enabling of taint checking.
i'll leave it up to you if you want to leave taint mode enabled.

@@ +34,5 @@
> +trick_taint($os);
> +
> +my $check_default_os = Bugzilla::Field::Choice->type('op_sys')->match({ value => $DEFAULT_OS });
> +die "This should not happen: Cannot remove $os because $DEFAULT_OS doesn't exist.\n"
> +    unless @$check_default_os == 1;

it's probably a good idea to also check that $os is a valid op_sys.
currently if you pass in an invalid OS no output is produced by the script, which is confusing.

nit: "This should not happen: " isn't necessary for error messages... errors in general shouldn't happen :)

@@ +40,5 @@
> +my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +my $field     = Bugzilla::Field->check({ name => 'op_sys', cache => 1 });
> +my $nobody    = Bugzilla::User->check({ name => '[e-mail]', cache => 1 });
> +my $bug_ids   = $dbh->selectcol_arrayref(q{SELECT bug_id FROM bugs WHERE bugs.op_sys = ?}, undef, $os);
> +

please add a confirmation prompt here as this script could be destructive (eg. as per movebugs.pl)

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8454235 [details] [diff] [review]
bug-936468-v2.1.patch

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

::: contrib/reorg-tools/remove-os.pl
@@ +23,5 @@
> +use Pod::Usage;
> +
> +Bugzilla->usage_mode(USAGE_MODE_CMDLINE);
> +
> +my $DEFAULT_OS = 'Other';

In the case of the metro bugs, I'd prefer they get moved to Windows 8.1. Otherwise I'm still going to have to spam the entire list updating to that. :/ Maybe this script should accept an optional command line param specifying the os to switch bug into?

Comment 8

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

(In reply to Jim Mathies [:jimm] from comment #7)
> In the case of the metro bugs, I'd prefer they get moved to Windows 8.1.
> Otherwise I'm still going to have to spam the entire list updating to that.
> :/ Maybe this script should accept an optional command line param specifying
> the os to switch bug into?

This would be a more useful script to have in the future, and is trivial to implement. Good call.

Comment 9

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

Attachment: bug-936468-v3.patch

While we had agreed this wouldn't need another round of review, given that the nature of the script changed slightly (movement rather than removal) I figured it warranted another r?

Comment 10

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

Or, rather -- given that the nature of this script changed, it deserves additional sanity checking than would be warranted.

Comment 11

[:glob 🎈]

Comment on attachment 8457737 [details] [diff] [review]
bug-936468-v3.patch

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

r=glob

::: contrib/reorg-tools/move_os.pl
@@ +10,5 @@
> +use strict;
> +use warnings;
> +
> +use FindBin '$RealBin';
> +BEGIN { ($RealBin) = $RealBin =~ /^(.+)$/ }

you don't need to detaint $RealBin

Comment 12

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

To [e-mail]:webtools/bmo/bugzilla.git
   b1f98ae..10bd960  master -> master