511028 - Create a read-only object using Bugzilla::Object for whine schedules

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: 3.5

Based on Bug 509959, Max Kanat-Alexander suggested I create read-only objects to make the transition easier to objects for whining.

Reproducible: Always

Comment 1

[Eric Black]

Attachment: patch for readonly whine objects

This is a first stab at a read-only object. A few notes:

* I haven't updated whine.pl for these objects yet, just editwhines.cgi. I wanted to see if I'm on the right track here first.
* It was much easier to create the Whine::Query and Whine::Schedule objects than try and fold it into a single Whine object. If that's not ok, I'll do it all in the Whine object.
* I am not removing the whine_query.query_name in this patch because of the other files that may need it yet. It may be more appropriate in bug 509959.
* There was some redundant code in editwhines.cgi at lines 171 and 234 of the new code. I originally created the two methods "verify_query" and "verify_schedule" until I realized it was redundant (it is verifying that the userid matches before allowing the delete statements, but all the overall events are selected based on the userid to begin with). I left the markers there and can put back the method if it would be less worrisome to leave it.

Comment 2

[Eric Black]

If this patch is ok, I'll start working on the update parts of the objects.

Comment 3

[Max Kanat-Alexander]

Wow. I agree that there should be separate objects. Would there be any way to post patches for just *one* of the objects at a time? This patch is huge.

Comment 4

[Eric Black]

Sure, that sounds like a good idea. How about I do it like:

1. Bugzilla::Whine::Schedule
2. Bugzilla::Install::DB and Bugzilla::DB::Schema
3. Bugzilla::Whine::Query
4. Bugzilla::Whine
5. editwhines.cgi

Any thought to the last point about "verify_schedule" and "verify_query" or do you want to hold off until the patch for editwhines?

Also, let me know if any of these should be broken out into their own bugs.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Sure, that sounds like a good idea. How about I do it like:
> [snip]

  That's fine, except that editwhines.cgi needs to be updated every step of the way, if possible. That is, as soon as an object exists, it should be used immediately.

> Any thought to the last point about "verify_schedule" and "verify_query" or do
> you want to hold off until the patch for editwhines?

  I didn't look at it--break them out and I'll see what I think.

Comment 6

[Eric Black]

Attachment: patch for read-only whine schedules and changes to editwhines.cgi for it

This is the patch for the read-only Bugzilla::Whine::Schedule object. The first select statement that is replaced is an example of the redundancy that I mentioned earlier; the select was using the userid as an input parameter, but the whole eventid selection is based on the userid to begin with.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 395470 [details] [diff] [review]
patch for read-only whine schedules and changes to editwhines.cgi for it

  Hey, sorry for the delay! This looks really nice, though! Just a few notes:

>Index: editwhines.cgi
>+                my $schedules = Bugzilla::Whine::Schedule->match({ 'WHERE' => { 'eventid = ?' => $eventid } });

  That's a misuse of match. You should just be doing:

  Bugzilla::Whine::Schedule->match({ eventid => $eventid });

  WHERE is generally only supposed to be used by subclasses directly, or by the WebService internally.

>+            my $schedules = Bugzilla::Whine::Schedule->match({ 'WHERE' => { 'eventid = ?' => $eventid } });

  Same note there.

>@@ -370,30 +359,27 @@
>+    my $schedules = Bugzilla::Whine::Schedule->match({ 'WHERE' => { 'eventid = ?' => $event_id } });

  And there.

>+        my $mailto_type = $schedule->mailto_type;
>         my $mailto = '';
>         if ($mailto_type == MAILTO_USER) {
>-            my $mailto_user = new Bugzilla::User($row->[3]);
>+            my $mailto_user = new Bugzilla::User($schedule->mailto);
>             $mailto = $mailto_user->login;

  Hmm. Instead of this whole block, why not just have the mailto accessor return the right object? Or even just have it return an array of user objects?

>+++ Bugzilla/Whine/Schedule.pm	2009-08-17 08:55:36.000000000 -0400
>+use constant VALIDATORS => {
>+};

  You don't need that if there aren't any defined.

>+use constant UPDATE_COLUMNS => qw(eventid run_day run_time run_next mailto mailto_type);

  Those should be on separate lines just like DB_COLUMNS so that it's easier and clearer to patch.

>+sub eventid     { return $_[0]->{'eventid'};     }

  I think that we should just provide an event accessor, and not really encourage people to access eventid. (For now you can have clients use {eventid} and then eventually ->event->id when such an object exists.)

>+sub run_day     { return $_[0]->{'run_day'};     }
>+sub run_time    { return $_[0]->{'run_time'};    }

  It might also be useful to have a next_run DateTime object that can be returned, but perhaps that's something you can do when you implement these things in whine.pl.

>+sub mailto_type { return $_[0]->{'mailto_type'}; }

  Since this just represents whether or not this is a group, let's call it is_group for now.

>+Does not accept a bare C<name> argument. Instead, accepts only an id.

  Hmm. I wonder if we should do NAME_FIELD => "id"?

Comment 8

[Eric Black]

Attachment: patch for read-only whine schedules and changes to editwhines.cgi for it

I will fix the "sub eventid" accessor when I create the Whine class (step 4 from comment 4) and add next_run when I do the whine.pl changes (or perhaps sooner) as you suggested.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 403371 [details] [diff] [review]
patch for read-only whine schedules and changes to editwhines.cgi for it

>+++ Bugzilla/Whine/Schedule.pm	2009-09-28 19:19:17.000000000 -0400
>+sub mailto {
>+    my $self = shift;
>+
>+    return $self->{mailto_object} if exists $self->{mailto_object};
>+    my $id = $self->{'mailto'};
>+
>+    # This is kind of unnecessary as the value should never be null
>+    return undef unless $id;

  If it's unnecessary, just leave it out. We should fail as soon as possible if there are errors.

>+sub mailto_users { 
>+    my $self = shift;
>+
>+    return $self->{mailto_users} if exists $self->{mailto_users};
>+    my $object = $self->mailto;
>+
>+    # This is kind of unnecessary as the value should never be null
>+    unless ($object) {

  Same there.

>+ my $is_group    = $schedule->is_group;

  For clarity, we might want to call it is_to_group.

  Otherwise, this looks great!

Comment 10

[Max Kanat-Alexander]

All right, on checkin I changed the accessor's name to mailto_is_group.

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

Comment 11

[Max Kanat-Alexander]

Attachment: Checked-In Version

Comment 12

[Eric Black]

Attachment: Bugzilla Query.pm changes

Thank you for updating your requested changes; I was on vacation at the time. The end of the year is extremely busy for me, so I don't know how much I can do until January, but I have attached the next portion, which is the read-only Query object. I can't do the db changes at this point (getting rid of query_name and adding query_id from namedqueries to whine_queries) because the operations to fill the query_id is not a read-only task.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> Thank you for updating your requested changes; I was on vacation at the time.
> The end of the year is extremely busy for me, so I don't know how much I can do
> until January, but I have attached the next portion, which is the read-only
> Query object. I can't do the db changes at this point (getting rid of
> query_name and adding query_id from namedqueries to whine_queries) because the
> operations to fill the query_id is not a read-only task.

  Okay, file a new bug for this one, please, and make it block bug 509959.

Comment 14

[Eric Black]

(In reply to comment #13)
>   Okay, file a new bug for this one, please, and make it block bug 509959.

Filed Bug 530467 for whine query objects and added attachment. Filed Bug 530468 for whine event objects.