530467 - Create a read-only object using Bugzilla::Object for whine queries

Status: RESOLVED FIXED

(Bugzilla :: Whining, enhancement)

Description

[Eric Black]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.13) Gecko/2009080316 Ubuntu/8.04 (hardy) Firefox/3.0.13
Build Identifier: 

Based on Bug 509959, Max Kanat-Alexander suggested I create read-only objects
to make the transition easier to objects for whining. Bug 511028 was for read only schedules, this is the second bug for the read only query objects


Reproducible: Always

Comment 1

[Eric Black]

Attachment: Whine Query object and changes to editwhines.cgi for it

This is the patch for a read only Whine Query object and the changes to editwhines.cgi using it.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 413969 [details] [diff] [review]
Whine Query object and changes to editwhines.cgi for it

>Index: editwhines.cgi
> [snip]
>         my $this_query = {
>-            'name'          => $row->[0],
>-            'title'         => $row->[1],
>-            'sort'          => $row->[2],
>-            'id'            => $row->[3],
>-            'onemailperbug' => $row->[4],
>+            'name'          => $query->name,
>+            'title'         => $query->title,
>+            'sort'          => $query->sortkey,
>+            'id'            => $query->id,
>+            'onemailperbug' => $query->onemailperbug,
>         };
>         push @{$events->{$event_id}->{'queries'}}, $this_query;

  Why not just push the object itself on to the array directly, instead of creating a $this_query object?

>--- /dev/null	2007-08-30 20:31:07.000000000 -0400
>+++ Bugzilla/Whine/Query.pm	2009-11-21 19:24:06.000000000 -0500

>+use strict;
>+
>+package Bugzilla::Whine::Query;

  Nit: Mostly, use strict goes right under the "package" declaration, in our files. (Or at least, that's the standard I'm trying to go for.)

>+use constant REQUIRED_CREATE_FIELDS => qw(eventid);
>+
>+use constant UPDATE_COLUMNS => qw(
>+    eventid 
>+    sortkey 
>+    onemailperbug 
>+    title
>+);

  We don't need those yet, so let's just skip 'em until we need them.

>+This module exists to represent a L<Bugzilla::Whine> event query.

  What's an "event query"? (I have a vague idea, but most people won't know.)


  All those things can be fixed on checkin, though, or you can post a new patch and grant yourself review.

Comment 4

[Eric Black]

Attachment: Patch with updates from comment #2

I made the requested changes and also made the following change for consistancy according to your suggestion regarding $this_query for $this_schedule:

>Index: editwhines.cgi
> [snip]
>-        my $this_schedule = {
>-            'day'         => $schedule->run_day,
>-            'time'        => $schedule->run_time,
>-            'mailto_type' => $mailto_type,
>-            'mailto'      => $mailto,
>-            'id'          => $schedule->id,
>-        };
>-        push @{$events->{$event_id}->{'schedule'}}, $this_schedule;
>+        push @{$events->{$event_id}->{'schedule'}},
>+             {
>+                 'day'         => $schedule->run_day,
>+                 'time'        => $schedule->run_time,
>+                 'mailto_type' => $mailto_type,
>+                 'mailto'      => $mailto,
>+                 'id'          => $schedule->id,
>+             };

Comment 5

[Eric Black]

Comment on attachment 414382 [details] [diff] [review]
Patch with updates from comment #2

Hope this is what you meant by asking myself for a review...

Comment 6

[Max Kanat-Alexander]

Comment on attachment 414382 [details] [diff] [review]
Patch with updates from comment #2

Okay, this looks okay, but could I get one more change? I'd rather see one_email_per_bug instead of onemailperbug. You could also just call it email_per_bug if you feel like one_email_per_bug is too long.

Also, instead of saying "a L<Bugzilla::Whine> event", you should say "a L<Bugzilla::Whine::Event>".

Comment 7

[Eric Black]

Attachment: Changes for one_email_per_bug and Bugzilla::Whine::Event from comment #6

Comment 8

[Max Kanat-Alexander]

Comment on attachment 416191 [details] [diff] [review]
Changes for one_email_per_bug and Bugzilla::Whine::Event from comment #6

Sweet, thanks! Looks great. :-)

Comment 9

[Max Kanat-Alexander]

Worked just fine on my testing before checkin. Thank you for the excellent code, Eric! :-)

Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Whine/Query.pm,v
done
Checking in Bugzilla/Whine/Query.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Whine/Query.pm,v  <--  Query.pm
initial revision: 1.1
done