750742 - Create new BMO extension called TrackingFlags to move current tracking flags away from custom fields

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[David Lawrence [:dkl]]

bugzilla.mozilla.org is currently utilizing custom fields to aid in tracking releases for Firefox, Thunderbird, and other projects. This is not ideal as every release requires even more custom fields to be created causing certain tables in Bugzilla to grow very large in number of columns. This new TrackingFlags extension will move the fields to their own tables which will allow us to continue to grow without affecting the core Bugzilla tables and also hopefully improve performance as a result. 

More info:
https://wiki.mozilla.org/BMO/TrackingFlags

Comment 1

[David Lawrence [:dkl]]

Hmm. Note to self to investigate:

Migrating custom tracking field cf_blocking_192
.......................585/585 (100%)
Deleting the 'cf_blocking_192' column from the 'bugs' table...
Dropping the 'cf_blocking_192' table...
Failed SQL: [DROP TABLE cf_blocking_192] Error: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails [for Statement "DROP TABLE cf_blocking_192"] at Bugzilla/DB.pm line 906.

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch to implement new tracking flags along with old (v1)

Things to do:

1) Implement (edit) type hiding for the new tracking fields. Currently always visible. 
2) Quite a bit of errors from the sanity tests due to missing errors, filters, etc.

Otherwise working and just wanted to go ahead and get your feedback on the UI changes, etc.

dkl

PS. I will also remove any trailing white space before committing :)

Comment 4

[:glob ✱]

Comment on attachment 727710 [details] [diff] [review]
Patch to implement new tracking flags along with old (v1)

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

from a quick look, will continue to poke around:

please remove trailing whitespace :)

auditing needs to be disabled on the Bugzilla::Extension::TrackingFlags::Flag::Bug object.

there are a lot of errors thrown by the test suite which need fixing.

i suspect the following are all symptoms of the same problem:
- i don't see the new fields on the advanced search page (not in 'custom search', nor 'search by change history')
- the fields are missing from a bug's xml format
- the fields are not valid field names for quicksearch, nor are they listed on qs's help page

::: extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ -20,5 @@
> -  # Contributor(s):
> -  #   Byron Jones <[e-mail]>
> -  #
> -  # ***** END LICENSE BLOCK *****
> -  #%]

mpl 2 license

::: extensions/TrackingFlags/Extension.pm
@@ +223,5 @@
> +                },
> +            },
> +        ],
> +    };
> +    # TODO indices

we'll probably want these before we go live :)

@@ +229,5 @@
> +
> +sub install_update_db {
> +    my $dbh = Bugzilla->dbh;
> +
> +    return; # XXX remove this to run the migration

it would be better to split the migration code into a stand-alone script.  that will remove one-shot code from the extension, and make it easier to run when required.

@@ +467,5 @@
> +
> +    my $bugs_join = {
> +        table => 'tracking_flags_bugs',
> +        as    => $bugs_alias,
> +        from  => "bugs.bug_id",

you can't assume the bugs table is 'bugs', use $args->{bugs_table}

::: extensions/TrackingFlags/lib/Constants.pm
@@ +1,2 @@
> +package Bugzilla::Extension::TrackingFlags::Constants;
> +

this file needs a license.

@@ +30,5 @@
> +    },
> +    {
> +        name        => 'b2g',
> +        description => 'B2G Flags',
> +        collapsed   => 1,

'collapsed' hasn't been implemented.

when true, the group should be displayed like 'tracking flags' currently does, with an (edit) link.
when false, the group should always be displayed as expanded, like 'project flags' does.

when running side-by-side with the existing custom-fields implementation, the fields need to be displayed inside the correct div (eg. tracking fields added by this code need to be placed inside the cf implementation's collpased div).

::: extensions/TrackingFlags/lib/Flag.pm
@@ +88,5 @@
> +    # We will create the entry as a custom field with a 
> +    # type of FIELD_TYPE_FREETEXT so Bugzilla will not
> +    # try to look for other external value tables.
> +    # Also we set custom = 2 so that it doesn't show
> +    # up in editfields.cgi (hack:)

can you add a comment here about why we create the flag as obsolete.

::: extensions/TrackingFlags/template/en/default/hook/admin/admin-end_links_right.html.tmpl
@@ +11,5 @@
> +    <a href="page.cgi?id=tracking_flags_admin_list.html">Release Tracking Flags</a>
> +  </dt>
> +  <dd>
> +    blah...
> +  </dd>

s/blah...//

::: extensions/TrackingFlags/template/en/default/hook/bug/create/create-form.html.tmpl
@@ +12,5 @@
> +    [% tracking_flag_names.push(flag.name) %]
> +  [% END %]
> +
> +  <script type="text/javascript">
> +  <!--

remove the <!-- and --> wrappers; we don't support browsers which lack js capabilities.

@@ +14,5 @@
> +
> +  <script type="text/javascript">
> +  <!--
> +    var tracking_flags = new Array([% product.components.size %]);
> +    var tracking_flag_names = ['[% tracking_flag_names.join("','") %]'];

you'll need a filter here.

::: extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_list.html.tmpl
@@ +57,5 @@
> +todo:
> +  - bulk edit of values and visibility
> +    - add value/visibility to "these flags"
> +    - remove value/visibility from "these flags"
> +</pre>

remove this :)

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to implement new tracking flags along with old (v2)

WIP with most of the fixes you suggested except:

1) Still no collapse support. Will work on that last.
2) Quicksearch doesn't work. Somehow it is treating them as normal flags :(
3) Does not show up in changed fields in advanced query.

However the new fields do show up in the boolean fields as well as change columns.

All sanity errors fixed. t/012throwables.t was not really our fault (bug 855072). Will work on this some more tonight/tomorrow morning.

dkl

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch to implement new tracking flags along with old (v3)

Comment 7

[:glob ✱]

Comment on attachment 730720 [details] [diff] [review]
Patch to implement new tracking flags along with old (v3)

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

due to the missing js file, i'm unable to review the show_bug changes.
i'm not going to review migration, as we don't need the for go-live.

the following lack tracking flags:
  - config.cgi
  - show_bug format=multiple
  - quicksearch help
  - bulk bug editing
  - webservice bug.get
  - push's serialisation -- add a hook to extensions/Push/lib/Serialise.pm::_bug()
  - release tracking report (after bug 853483 is fixed)
  - the "x-bugzilla-tracking header" set by extensions/BMO/Extension::mailer_before_send()
  - the 'tracking flags' blurb in bugmail's footer
  - bugzilla.dtd
  - the create-winqual custom bug form

when the flags are added as a column to search results, flags without values are displayed as an empty string instead of '---'.

::: extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ -20,5 @@
> -  # Contributor(s):
> -  #   Byron Jones <[e-mail]>
> -  #
> -  # ***** END LICENSE BLOCK *****
> -  #%]

please re-license this to mpl2

::: extensions/TrackingFlags/Extension.pm
@@ +22,5 @@
> +use Bugzilla::Product;
> +use Bugzilla::Component;
> +use Bugzilla::Error;
> +use Bugzilla::Extension::BMO::Data;
> +use Bugzilla::Install::Util qw(indicate_progress);

indicate_progress isn't used here

@@ +259,5 @@
> +                  "        AND tracking_flags_bugs.bug_id = bugs.bug_id)";
> +        $columns->{$flag->name} = {
> +            name  => $sql,
> +            title => $flag->description
> +        };

hrm, this will result in a dependent subquery for each visible flag, which we'd want to avoid.
i think we're better off returning $flag->name as the name, and augmenting COLUMN_JOINS in bugzilla/search.

::: extensions/TrackingFlags/lib/Flag.pm
@@ +85,5 @@
> +    # We will create the entry as a custom field with a
> +    # type of FIELD_TYPE_FREETEXT so Bugzilla will not
> +    # try to look for other external value tables.
> +    # Also we set custom = 2 so that it doesn't show
> +    # up in editfields.cgi (hack:)

can you add a comment here about why we create the flag as obsolete.

::: extensions/TrackingFlags/lib/Flag/Bug.pm
@@ +7,5 @@
> +
> +package Bugzilla::Extension::TrackingFlags::Flag::Bug;
> +
> +use base qw(Bugzilla::Object);
> +

this object needs to be excluded from auditing

::: extensions/TrackingFlags/template/en/default/hook/bug/create/create-bug_flags_end.html.tmpl
@@ +5,5 @@
> +  # This Source Code Form is "Incompatible With Secondary Licenses", as
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% IF new_tracking_flags.size %]

rather than wrapping an IF block around the entire file, do
[% RETURN UNLESS new_tracking_flags.size %]

(this applies to multiple files)

::: extensions/TrackingFlags/template/en/default/hook/bug/show-bug_end.xml.tmpl
@@ +1,1 @@
> +[% IF new_tracking_flags.size %]

this file needs a license.

::: extensions/TrackingFlags/template/en/default/hook/bug/show-header-end.html.tmpl
@@ +5,5 @@
> +  # This Source Code Form is "Incompatible With Secondary Licenses", as
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% javascript_urls.push('extensions/TrackingFlags/web/js/tracking_flags.js') %]

web/js/tracking_flags.js is missing from the patch

::: extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_list.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% PROCESS global/header.html.tmpl
> +  title = "Relase Tracking Flags"

s/Relase/Release/

::: show_bug.cgi
@@ +122,5 @@
>      $displayfields{$_} = undef;    
>  }
>  
> +use Data::Dumper;
> +print STDERR Dumper \%displayfields;

remove debugging code

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch to implement new tracking flags along with old (v5)

Hopefully getting closer :)

dkl

Comment 9

[:glob ✱]

Comment on attachment 738197 [details] [diff] [review]
Patch to implement new tracking flags along with old (v5)

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

once a flag is set, someone without permission to set the flag is unable to clear it.
steps:
  1. create a flag with --- ? and +
  2. allow anyone to set the flag to --- and ?, but require admin to set it to +
  3. set the flag on a bug to +
  4. load the bug with a non-admin user
expected:
  the non-admin user should be able to set the flag back to --- or ?
actual:
  neither --- or ? options are present in the select

when the flags are added as a column to search results, flags without values are displayed as an empty string instead of '---'.

places where these flags need to appear:
- the 'tracking flags' blurb in bugmail's footer
- push's serialisation
- release tracking report

non-blockers:
- in the admin pages, if you get an error and then go back, values and visibility changes are lost
- in the admin pages, after you have created a flag it should return back to the list

::: extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ -20,5 @@
> -  # Contributor(s):
> -  #   Byron Jones <[e-mail]>
> -  #
> -  # ***** END LICENSE BLOCK *****
> -  #%]

nit: wrong licenseplate.

::: extensions/TrackingFlags/Extension.pm
@@ +87,5 @@
> +    if ($file =~ m{email/bugmail(html|txt)\.tmpl}) {
> +        $vars->{'tracking_flags'} = Bugzilla::Extension::TrackingFlags::Flag->match({
> +            bug_id => $vars->{'bug'}->id
> +        });
> +    }

all the "if"s in template_before_process should be "elsif"s, except the first one of course.

::: extensions/TrackingFlags/lib/Flag/Value.pm
@@ +54,5 @@
> +###############################
> +
> +sub _check_value {
> +    my ($invocant, $value) = @_;
> +    $value || ThrowCodeError('param_required', { param => 'value' });

should be defined($value)

::: extensions/TrackingFlags/template/en/default/hook/bug/create/create-bug_flags_end.html.tmpl
@@ +18,5 @@
> +    <tr>
> +      <td>
> +        <table class="tracking_flags">
> +        <tr>
> +          <th style="text-align:left">

move this alignment change to edit_bug.css

@@ +19,5 @@
> +      <td>
> +        <table class="tracking_flags">
> +        <tr>
> +          <th style="text-align:left">
> +            [% type.name FILTER ucfirst FILTER html %] Flags:

this should be [% type.description FILTER html %]:

::: extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ +87,5 @@
> +    <td>
> +      [% IF bug.check_can_change_field('flagtypes.name', 0, 1) %]
> +        [% IF user.id %]
> +          <span id="edit_tracking_flags_action">
> +            (<a name="tracking" class="edit_tracking_flags_link">edit</a>)

this needs a href to give it the correct cursor

@@ +151,5 @@
> +    [% END %]
> +    [% IF flag_list.size %]
> +      <tr>
> +        <td class="field_label">
> +          <label>[% type.name FILTER ucfirst FILTER html %] Flags:</label>

this should be [% type.description FILTER html %]

@@ +157,5 @@
> +        <td>
> +          [% IF bug.check_can_change_field('flagtypes.name', 0, 1) %]
> +            [% IF user.id && type.collapsed %]
> +              <span id="edit_[% type.name FILTER html %]_flags_action">
> +                (<a name="[% type.name FILTER html %]" class="edit_tracking_flags_link">edit</a>)

this needs a href to give it the correct cursor

::: extensions/TrackingFlags/template/en/default/hook/bug/show-header-end.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% IF NOT javascript_urls.contains('js/yui3/yui/yui-min.js') %]
> +  [% javascript_urls.push('js/yui3/yui/yui-min.js') %]
> +[% END %]

we aren't using yui3

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to implement new tracking flags along with old (v6)

> once a flag is set, someone without permission to set the flag is unable to
> clear it.
> steps:
>   1. create a flag with --- ? and +
>   2. allow anyone to set the flag to --- and ?, but require admin to set it
> to +
>   3. set the flag on a bug to +
>   4. load the bug with a non-admin user
> expected:
>   the non-admin user should be able to set the flag back to --- or ?
> actual:
>   neither --- or ? options are present in the select

Switched to current behavior per our discussion.

> when the flags are added as a column to search results, flags without values
> are displayed as an empty string instead of '---'.

Fixed
 
> places where these flags need to appear:
> - the 'tracking flags' blurb in bugmail's footer

Fixed

> - push's serialisation

Worked itself out :)

> - release tracking report

Fixed

I also figured out search by simplifying the way I was doing buglist_column_joins. Since I already know the tracking flags id, I was able to cut out the extra left join.

dkl

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to implement new tracking flags along with old (v7)

New patch:

1. Had to update schema :( Added field_id column to tracking_flags table which mirrors the id value from fielddefs. This was so places in the core code which needed to access $field->id would get the fielddefs value instead of the id from tracking_flags.

2. Updated search to work better by utilizing the flag id from tracking_flags instead of requiring an additional table join. Search now seems to work properly when so some simple side-side comparisons of pre and post migrated data. More verification is needed and another set of eyes when you test the migration.

3. Update migration script.

dkl

Comment 12

[:glob ✱]

Comment on attachment 748032 [details] [diff] [review]
Patch to implement new tracking flags along with old (v7)

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

getting real close now..

- unable to make a value inactive.  no error, checkbox remains checked after hitting submit
- the fields on enter_bug should link to fields.html (for parity with the existing fields)
  - once we've migrated all the fields we could remove that link, because the text there is useless, and instead link to a wiki page which describes tracking/project/... flags
- the fields aren't returned by the web services
- the create-winqual custom bug form doesn't have the fields

::: Bugzilla/Search/Quicksearch.pm
@@ +387,5 @@
> +            addChart('flagtypes.name', 'substring', $1, $negate);
> +            $chart++; $and = $or = 0; # Next chart for boolean AND
> +            addChart('requestees.login_name', 'substring', $2, $negate);
> +            return 1;
> +        }

this will need to be updated following changes in bug 828344

::: extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
@@ -20,5 @@
> -  # Contributor(s):
> -  #   Byron Jones <[e-mail]>
> -  #
> -  # ***** END LICENSE BLOCK *****
> -  #%]

this is still the wrong license.

::: extensions/TrackingFlags/Extension.pm
@@ +80,5 @@
> +            product   => $vars->{'one_product'}->name,
> +            is_active => 1
> +        });
> +    }
> +    elsif ($file =~ m{email/bugmail(html|txt)\.tmpl}) {

there's no need for the overhead of a regex here, use || instead

::: extensions/TrackingFlags/lib/Constants.pm
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +#
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +package Bugzilla::Extension::TrackingFlags::Constants;

nit: need a blank line after the boilerplate.

::: extensions/TrackingFlags/lib/Flag.pm
@@ +123,5 @@
> +
> +    # Update the fielddefs entry
> +    $dbh->do("UPDATE fielddefs SET name = ?, description = ? WHERE name = ?",
> +             undef,
> +             $self->name, $self->description, $old_self->name);

use the field_id here instead of the name in the WHERE clause

@@ +299,5 @@
> +######################################
> +# Compatibility with Bugzilla::Field #
> +######################################
> +
> +sub id                     { return $_[0]->{'field_id'};  }

please add a comment here which states why we don't return the actual id of this object, or at least draw attention to this as it's unusual (but required).

Comment 13

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #12)
> extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.
> tmpl
> @@ -20,5 @@
> > -  # Contributor(s):
> > -  #   Byron Jones <[e-mail]>
> > -  #
> > -  # ***** END LICENSE BLOCK *****
> > -  #%]
> 
> this is still the wrong license.

This whole file has been removed and moved to the TrackingFlags extension which is why you see the old license in the patch.
 
dkl

Comment 14

[David Lawrence [:dkl]]

Ok, current revised version of this patch is now on https://bugzilla-dev.allizom.org. We will discuss how best to put out the word to some testers to get them to beat on it a little before we update the schema on production.

dkl

Comment 15

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v8)

Comment 16

[:glob ✱]

Comment on attachment 759918 [details] [diff] [review]
748032: Patch to implement new tracking flags along with old (v8)

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

::: extensions/TrackingFlags/Extension.pm
@@ +333,5 @@
> +            $object->{$flag->name} = $flag->bug_flag->value;
> +            my $accessor = sub {
> +                my ($self) = @_;
> +                return $self->{$flag->name};
> +            };

it would be better to store $flag->name into a var, and have the closure use that:
my $accessor = sub { $_[0]->{$flag_name} };

@@ +338,5 @@
> +            my $name = "Bugzilla::Bug::" . $flag->name;
> +            {
> +                no strict 'refs';
> +                next if defined *{$name};
> +                *{$name} = $accessor;

you're creating class-level methods based on the bug-level visibility of the field, so this will break if you have multiple bug objects loaded.  also object_end_of_new won't be called for bugs objects created via new_from_list.

instead you should be hooking _create_cf_accessors, and create accessors for all fields regardless of per-bug visibility.  the calling code should take care to not referencing invalid fields.

::: extensions/TrackingFlags/lib/Admin.pm
@@ +60,5 @@
> +
> +    } elsif ($input->{save}) {
> +        # save
> +
> +        use Data::Dumper; print STDERR Dumper $input;

oops

::: extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
@@ +9,5 @@
> +[% FOREACH flag = flag_list %]
> +  [% NEXT IF !new_bug && (!user.id && flag.bug_flag.value == '---') %]
> +  <tr id="row_[% flag.name FILTER html %]">
> +    <td class="field_label">
> +      <label for="[% flag.name FILTER html %]">

fields should only have labels on enter_bug, not on show_bug.

with this revision fields on show_bug are incorrectly linked to fields.html, and have a bold weight.

Comment 17

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v9)

Thanks for the review. This new patch addresses the changes you suggested.

dkl

Comment 18

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v10)

New patch that preloads all set bug values in bug_create_cf_accessors hook the first time it is called.

dkl

Comment 19

[:glob ✱]

Comment on attachment 764902 [details] [diff] [review]
748032: Patch to implement new tracking flags along with old (v10)

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

functionality-wise, this is looking in good shape.

however there's a lot of redundant sql queries generated when viewing a bug which need to be addressed.

::: extensions/TrackingFlags/lib/Flag.pm
@@ +171,5 @@
> +    if ($bug_id) {
> +        foreach my $flag (keys %flag_hash) {
> +            $flag_hash{$flag}->bug_flag($bug_id);
> +        }
> +    }

this pre-loading triggers a db call per flag.
it needs to instead create the objects from a list generated by a single query.

::: extensions/TrackingFlags/lib/Flag/Bug.pm
@@ +104,5 @@
> +sub value            { return $_[0]->{'value'};            }
> +
> +sub bug {
> +    my ($self) = @_;
> +    $self->{'bug'} ||= Bugzilla::Bug->new($self->bug_id);

cache => 1

::: extensions/TrackingFlags/lib/Flag/Value.pm
@@ +73,5 @@
> +    my ($invocant, $group) = @_;
> +    if (blessed $group) {
> +        return $group->id;
> +    }
> +    $group = Bugzilla::Group->new($group)

use the object cache here

@@ +113,5 @@
> +
> +sub setter_group {
> +    my ($self) = @_;
> +    if ($self->setter_group_id) {
> +        $self->{'setter_group'} ||= Bugzilla::Group->new($self->setter_group_id);

use the object cache here too.  this one should make a major difference (removed 230 redundant db hits in my testing).

::: extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% FOREACH flag = flag_list %]
> +  [% NEXT IF !new_bug && (!user.id && flag.bug_flag.value == '---') %]

flag.bug_flag needs the bug_id passed to it (this needs to be fixed multiple times in this template).

Comment 20

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v11)

Hopefully this is the one :) Thanks for the review.

Comment 21

[:glob ✱]

Comment on attachment 765936 [details] [diff] [review]
748032: Patch to implement new tracking flags along with old (v11)

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

migrate_tracking_flags.pl should be attached to bug 880829 not this one.
looks like you accidentally included robots.txt too.

i still see many db calls to load bug flags and values individually:
> SELECT id,tracking_flag_id,bug_id,value FROM tracking_flags_bugs WHERE tracking_flag_id = '236' AND bug_id = '0'
> SELECT id,tracking_flag_id,setter_group_id,value,sortkey,is_active FROM tracking_flags_values WHERE tracking_flag_id = '239'  ORDER BY sortkey
these are both called 27 times when i load a bug (one for each tracking_flag_id).

the first call, with id 0, is because you aren't passing in the bug id to flag.bug_flag() in the template, however given they are already preloaded it looks like these calls are redundant.

loading of the values from tracking_flags_values should happen with a single db select, which loads all values for the relevant flags.

::: extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% FOREACH flag = flag_list %]
> +  [% NEXT IF !new_bug && (!user.id && flag.bug_flag.value == '---') %]

flag.bug_flag needs the bug_id passed to it (this needs to be fixed multiple times in this template).

Comment 22

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v12)

Comment 23

[:glob ✱]

Comment on attachment 774870 [details] [diff] [review]
748032: Patch to implement new tracking flags along with old (v12)

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

i'm still seeing many redundant database queries:

i guess these are the preloads:

SELECT id,field_id,name,description,type,sortkey,is_active FROM tracking_flags WHERE is_active = '1' AND  id IN ('1','199','200','201','202','203','204','205','206','207','208','209','210','211','212','213','214','215','216','217','218','219','220','221','222','223','224','225','226','227','228','229','230','231','232','233','234','235','236','237','238','239','240','241','242','243','244','245','246','247','248','249','250','251','252','253','254','255','256','257','258','259','260','261','262','263','264','265','266','267','268','269','270','271','272','273','274','275','276','277','278','279','280','281','282','283','284','285','286','287','288','289','290','291','292','293','294','295','296','297','298','299','300','301','342','343','344','345')   ORDER BY sortkey
SELECT id,tracking_flag_id,setter_group_id,value,sortkey,is_active FROM tracking_flags_values WHERE  tracking_flag_id IN ('241','234','245','239','295','294','1','244','292','235','301','300','297','240','233','259','253','248','236','258','231','345','243','296','232','293','343','242')   ORDER BY sortkey

however soon after there's still a query for each flag:

SELECT id,field_id,name,description,type,sortkey,is_active FROM tracking_flags WHERE id = '240'
SELECT id,field_id,name,description,type,sortkey,is_active FROM tracking_flags WHERE id = '243'
SELECT id,field_id,name,description,type,sortkey,is_active FROM tracking_flags WHERE id = '234'
...
over the loading of a single Core bug with about 30 tracking flags (ie. the normal bmo amount), there are 108 of these queries, so there's significant gains to be had by eliminating them.

i also see multiple calls to tracking_flags_visibility during a single bug show_bug, which appear to be redundant too:

SELECT id FROM tracking_flags_visibility WHERE 1=1 AND  product_id IN (1)  AND ( component_id IN (460,650,295,354,362,3,576,463,670,761,1561)  OR component_id IS NULL)

SELECT id FROM tracking_flags_visibility WHERE 1=1 AND  product_id IN (1)  AND ( component_id IN (460)  OR component_id IS NULL)

SELECT id FROM tracking_flags_visibility WHERE 1=1 AND  product_id IN (1)  AND ( component_id IN (460)  OR component_id IS NULL)


when loading the xml form of a bug, the same query is executed 147 times:
SELECT id,tracking_flag_id,bug_id,value FROM tracking_flags_bugs WHERE bug_id = '749922'  ORDER BY id
as this is used by bzapi, it's another important place to minimise the number of round-trips to the database.

::: extensions/BMO/template/en/default/bug/create/create-winqual.html.tmpl
@@ +617,5 @@
>    [%# crash-signature gets custom handling %]
> +  [% IF field.name == 'cf_crash_signature' %]
> +    [% show_crash_signature = 1 %]
> +    [% NEXT %]
> +  [% END %] 

trailing whitespace

::: extensions/TrackingFlags/lib/Flag/Bug.pm
@@ +67,5 @@
> +    if ($param) {
> +        $self = $class->SUPER::new(@_);
> +        if (!$self) {
> +            $self = DEFAULT_FLAG_BUG;
> +            bless ($self, $class);

nit: remove space between bless and (

@@ +72,5 @@
> +        }
> +    }
> +    else {
> +        $self = DEFAULT_FLAG_BUG;
> +        bless ($self, $class);

nit: remove space between bless and (

Comment 24

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v13)

Found a few places where I had issues with proper preloading that was causing the extra queries when not necessary. Please do another check as it looks better to me but need your check as well.

dkl

Comment 25

[:glob ✱]

Comment on attachment 778117 [details] [diff] [review]
748032: Patch to implement new tracking flags along with old (v13)

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

this patch is missing the trackingflags extension

Comment 26

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (v13)

le sigh

Comment 27

[:glob ✱]

Comment on attachment 779202 [details] [diff] [review]
748032: Patch to implement new tracking flags along with old (v13)

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

excellent work on reducing the db hits :)
ok, let's get this committed.

r=glob with the following nits addressed.

::: extensions/TrackingFlags/Extension.pm
@@ +269,5 @@
> +
> +    # Create a hash of current fields based on field names
> +    my %field_hash = map { $_->name => $_ } @$$fields;
> +
> +     my @tracking_flags;

nit: indentation

@@ +338,5 @@
> +        my $name = "Bugzilla::Bug::$flag_name";
> +        {
> +            no strict 'refs';
> +            *{$name} = $accessor;
> +        }

under mod_perl this throws lots of Subroutine Bugzilla::Bug::cf_* redefined at extensions/TrackingFlags/Extension.pm line 341.

if (!Bugzilla::Bug->can($flag_name)) {
    no string 'refs';
    *{$name} = $accessor;
}

::: extensions/TrackingFlags/lib/Flag.pm
@@ +91,5 @@
> +    $dbh->bz_start_transaction();
> +
> +    $params = $class->run_create_validators($params);
> +
> +    # We ihave to create an entry for this new flag

s/ihave/have/

@@ +99,5 @@
> +    # do not need. Also we do this so as not to add a
> +    # another column to the bugs table.
> +    # We will create the entry as a custom field with a
> +    # type of FIELD_TYPE_EXTENSION so Bugzilla will skip
> +    # thesl field types in certain parts of the core code.

s/thesl/these/

@@ +146,5 @@
> +    if (exists $cache->{'tracking_flags'}
> +        && exists $cache->{'tracking_flags'}->{$self->flag_id})
> +    {
> +        $cache->{'tracking_flags'}->{$self->flag_id} = $self;
> +    }

nit: there's no need to check if it exists before deleting it; just check if $cache->{tracking_flags} exists.

@@ +154,5 @@
> +
> +sub match {
> +    my $class = shift;
> +    my ($params) = @_;
> +    my $cache = Bugzilla->request_cache;

$cache isn't used here

@@ +209,5 @@
> +    if (exists $cache->{'tracking_flags'}
> +        && exists $cache->{'tracking_flags'}->{$self->flag_id})
> +    {
> +        delete $cache->{'tracking_flags'}->{$self->flag_id};
> +    }

nit: there's no need to check if it exists before deleting it; just check if $cache->{tracking_flags} exists.

Comment 28

[David Lawrence [:dkl]]

Schema changes only. Will attach a patch with the rest that glob will commit after this weeks first push.

dkl

Committing to: bzr+ssh://[e-mail]/bmo/4.2
added extensions/TrackingFlags
added extensions/TrackingFlags/Config.pm
added extensions/TrackingFlags/Extension.pm
Committed revision 8893.

Comment 29

[David Lawrence [:dkl]]

Attachment: 748032: Patch to implement new tracking flags along with old (POST SCHEMA UPDATE) (v15)

This patch is to be committed and pushed after the current code is pushed which updates the schema. Glob should be the one performing the pushes so he will be committing this patch.

dkl

Comment 30

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified post_bug.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Constants.pm
modified Bugzilla/Object.pm
modified Bugzilla/Search/Quicksearch.pm
modified extensions/BMO/Extension.pm
modified extensions/BMO/lib/Util.pm
modified extensions/BMO/lib/Reports/ReleaseTracking.pm
modified extensions/BMO/template/en/default/bug/create/create-winqual.html.tmpl
modified extensions/BMO/template/en/default/email/bugmail.txt.tmpl
missing extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
deleted extensions/BMO/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
modified extensions/BMO/web/js/edit_bug.js
modified extensions/TrackingFlags/Extension.pm
modified skins/custom/create_bug.css
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/field.html.tmpl
modified template/en/default/bug/create/create.html.tmpl
modified template/en/default/global/field-descs.none.tmpl
Committed revision 8896.

Comment 31

[:glob ✱]

oops ..

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
added extensions/TrackingFlags/lib
added extensions/TrackingFlags/template
added extensions/TrackingFlags/web
added extensions/TrackingFlags/lib/Admin.pm
added extensions/TrackingFlags/lib/Constants.pm
added extensions/TrackingFlags/lib/Flag
added extensions/TrackingFlags/lib/Flag.pm
added extensions/TrackingFlags/lib/Flag/Bug.pm
added extensions/TrackingFlags/lib/Flag/Value.pm
added extensions/TrackingFlags/lib/Flag/Visibility.pm
added extensions/TrackingFlags/template/en
added extensions/TrackingFlags/template/en/default
added extensions/TrackingFlags/template/en/default/bug
added extensions/TrackingFlags/template/en/default/hook
added extensions/TrackingFlags/template/en/default/pages
added extensions/TrackingFlags/template/en/default/bug/tracking_flags.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/admin
added extensions/TrackingFlags/template/en/default/hook/bug
added extensions/TrackingFlags/template/en/default/hook/global
added extensions/TrackingFlags/template/en/default/hook/admin/admin-end_links_right.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create
added extensions/TrackingFlags/template/en/default/hook/bug/edit-after_custom_fields.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/field-editable.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/field-non_editable.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/show-header-end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-bug_flags_end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-form.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-project_flags_end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-tracking_flags_end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-winqual-bug_flags_end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-winqual-project_flags_end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/bug/create/create-winqual-tracking_flags_end.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/global/code-error-errors.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/global/messages-messages.html.tmpl
added extensions/TrackingFlags/template/en/default/hook/global/user-error-errors.html.tmpl
added extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl
added extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_list.html.tmpl
added extensions/TrackingFlags/web/js
added extensions/TrackingFlags/web/styles
added extensions/TrackingFlags/web/js/admin.js
added extensions/TrackingFlags/web/js/tracking_flags.js
added extensions/TrackingFlags/web/styles/admin.css
added extensions/TrackingFlags/web/styles/edit_bug.css
Committed revision 8897.