Last Comment Bug 899091 - SENTRY ERROR: Can't locate object method "cf_partner_koi_tcl" via package "Bugzilla::Bug" at /loader/0x7f88f01cfd60/Bugzilla/Extension/Push/Serialise.pm line 217.
: SENTRY ERROR: Can't locate object method "cf_partner_koi_tcl" via package "Bu...
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: Extensions: TrackingFlags (show other bugs)
: Production
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: David Lawrence [:dkl]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-29 07:48 PDT by David Lawrence [:dkl]
Modified: 2013-08-13 07:32 PDT (History)
3 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
899091_1.patch (1.96 KB, patch)
2013-07-31 15:18 PDT, David Lawrence [:dkl]
no flags Details | Diff | Splinter Review
899091_1.patch (7.92 KB, patch)
2013-08-07 15:58 PDT, David Lawrence [:dkl]
glob: review-
Details | Diff | Splinter Review
899091_2.patch (11.35 KB, patch)
2013-08-11 14:43 PDT, David Lawrence [:dkl]
glob: review+
Details | Diff | Splinter Review

Description David Lawrence [:dkl] 2013-07-29 07:48:44 PDT
https://errormill.mozilla.org/bugzilla/bmo/group/55058/
Comment 1 David Lawrence [:dkl] 2013-07-29 15:20:27 PDT
One other error of the two seen so far:
https://errormill.mozilla.org/bugzilla/bmo/group/55060

I have configured all of the same flags locally and enabled Push and enabled the File connector. I am not able to get the same failure locally while recreating the same steps the user did. Both errors came from the same user and from the performing a mass bug change adding himself to the CC of a list of Firefox bugs.

https://bugzilla.mozilla.org/page.cgi?id=user_activity.html&action=run&who=bezaleel360%40gmail.com&from=2013-07-29&to=2013-07-29&sort=when

Could it had been a timing issue with when I was adding the flags and when he was making his changes? I added them around the same time although I do not remember the exact minute?

glob can you enable this on your environment as well and see if you can recreate as well? We could also re-enable the flags while being able to keep an eye and see if any others come in. 

dkl
Comment 2 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-07-29 22:07:47 PDT
(In reply to David Lawrence [:dkl] from comment #1)
> We could also re-enable the flags while being able to keep an eye and see if any
> others come in. 

i don't think it would be wise to do that until we've figured out what's going on.


i'll investigate.
Comment 3 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-07-29 23:20:43 PDT
neither of these are the result of a bulk bug change (the referrer is show_bug.cgi).

for 55060, the user loaded the bug normally and tried to add themselves to the cc list.  this failed at 7:35, but worked at 7:38.

i can't reproduce this locally :(

one issue i see is (from push's serialise class):

>    my @custom_fields = Bugzilla->active_custom_fields;
>    foreach my $field (@custom_fields) {
>        my $name = $field->name;
>        # skip custom fields that are hidded from this product/component
>        next if Bugzilla::Extension::BMO::cf_hidden_in_product(
>            $name, $bug->product, $bug->component);
>        $rh->{$name} = _custom_field($field, $bug->$name);
>    }

cf_hidden_in_product() wouldn't apply to the tracking-flags fields.

we probably want to pass in the bug's product and component to active_custom_fields(), and drop the cf_hidden_in_product call.

i don't see how that would cause this particular issue, but it looks wrong, and it's in a relevant section of code.
Comment 4 David Lawrence [:dkl] 2013-07-30 07:14:18 PDT
(In reply to Byron Jones ‹:glob› from comment #3)
> neither of these are the result of a bulk bug change (the referrer is
> show_bug.cgi).
 
My bad. According to user activity report, the user added himself to the cc list of a lot of bugs in a short time so I too quickly assumed it was a mass edit.

> for 55060, the user loaded the bug normally and tried to add themselves to
> the cc list.  this failed at 7:35, but worked at 7:38.
> 
> i can't reproduce this locally :(

Yeah, i tried quite a few variations and all were successful which made it the more strange.

> one issue i see is (from push's serialise class):
> 
> >    my @custom_fields = Bugzilla->active_custom_fields;
> >    foreach my $field (@custom_fields) {
> >        my $name = $field->name;
> >        # skip custom fields that are hidded from this product/component
> >        next if Bugzilla::Extension::BMO::cf_hidden_in_product(
> >            $name, $bug->product, $bug->component);
> >        $rh->{$name} = _custom_field($field, $bug->$name);
> >    }
> 
> cf_hidden_in_product() wouldn't apply to the tracking-flags fields.
> 
> we probably want to pass in the bug's product and component to
> active_custom_fields(), and drop the cf_hidden_in_product call.
> 
> i don't see how that would cause this particular issue, but it looks wrong,
> and it's in a relevant section of code.

Yeah. The fields I added yesterday would not match the regex used by cf_hidden_in_product so it would be harmless. But if I were to create a new flag that did match it would override the new flags visibility settings possibly.

I can make a patch for that here for review.

dkl
Comment 5 David Lawrence [:dkl] 2013-07-31 15:18:57 PDT
Created attachment 784037 [details] [diff] [review]
899091_1.patch

Changes:

1. When working on this found out that Bugzilla->active_custom_fields was loading all tracking flags even though product/component was provided to filter them down. I fixed it by altering Bugzilla->active_custom_fields to skip fields of type FIELD_TYPE_EXTENSION and allow the hook to properly add the new tracking flags in to the list of fields.

2. Updated Push::Serialize::_bug to filter the custom fields by product/component instead of calling cf_hidden_in_product directly. The BMO hook in Bugzilla->active_custom_fields will still provide the same filtering.

3. Quick add to sort the tracking flags by sortkey in the admin list.

dkl
Comment 6 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-08-06 16:01:19 PDT
Comment on attachment 784037 [details] [diff] [review]
899091_1.patch

moving this attachment to bug 902212
Comment 7 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-08-06 16:03:33 PDT
this looks like a race condition as these errors were all created at the exact same time the new tracking fields were added.

given the frequency which fields are added, i don't think this issue is as critical as first thought - dropping priority.

on way of fixing this would be to enable disable_bug_updates around the field creation.
Comment 8 David Lawrence [:dkl] 2013-08-07 15:58:57 PDT
Created attachment 787186 [details] [diff] [review]
899091_1.patch
Comment 9 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-08-09 08:37:23 PDT
Comment on attachment 787186 [details] [diff] [review]
899091_1.patch

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

this mostly looks and works well.

however when i delete a field, i get:
 Invalid flag ID (0).

::: extensions/TrackingFlags/lib/Flag.pm
@@ +90,5 @@
>      my $dbh = Bugzilla->dbh;
> +    my $flag;
> +
> +    eval {
> +        SetParam('disable_bug_updates',  1);

set the param outside of the eval

@@ +115,5 @@
>               $params->{'name'},
>               $params->{'description'},
>               $params->{'sortkey'},
>               FIELD_TYPE_EXTENSION,
>               1, 0, 1);

the indentation here is incorrect

@@ +217,5 @@
> +
> +    # Check to see if tracking_flags_bugs table has records
> +    if ($self->has_bug_values) {
> +        ThrowUserError('customfield_has_contents', { 'name' => $self->name });
> +    }

create a new error message for these, as tracking flags are not custom fields.

@@ +220,5 @@
> +        ThrowUserError('customfield_has_contents', { 'name' => $self->name });
> +    }
> +
> +    eval {
> +        SetParam('disable_bug_updates',  1);

this should happen outside of the eval

@@ +226,5 @@
> +
> +        $dbh->bz_start_transaction();
> +
> +        # With ON DELETE CASCADE, this will remove the rows from the
> +        # all of the tracking flags tables as well so nothing more to do.

this comment isn't necessary

::: extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl
@@ +181,4 @@
>  
>  </form>
>  
> +[% END %]

you either need to revert this change, or correctly indent the bulk of this file as it's now within an IF block.  my preference is for the change to be reverted.
Comment 10 David Lawrence [:dkl] 2013-08-11 14:43:55 PDT
Created attachment 788724 [details] [diff] [review]
899091_2.patch
Comment 11 Byron Jones ‹:glob› [PTO until 2016-10-10] 2013-08-13 07:18:44 PDT
Comment on attachment 788724 [details] [diff] [review]
899091_2.patch

r=glob
Comment 12 David Lawrence [:dkl] 2013-08-13 07:32:22 PDT
Thanks

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2
modified extensions/TrackingFlags/Extension.pm
modified extensions/TrackingFlags/lib/Admin.pm                                                                            
modified extensions/TrackingFlags/lib/Flag.pm
modified extensions/TrackingFlags/template/en/default/hook/global/messages-messages.html.tmpl
modified extensions/TrackingFlags/template/en/default/hook/global/user-error-errors.html.tmpl
modified extensions/TrackingFlags/template/en/default/pages/tracking_flags_admin_edit.html.tmpl
Committed revision 8936. 

dkl

Note You need to log in before you can comment on or make changes to this bug.