User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax) I'd like to do the following things to Whining: 1) Create a new DB table, whine_recipients, which is used to store the recipients for some given whine_schedule. The new table would have the following columns: schedule_id (indexed, not null): The ID of the associated schedule (foreign key whine_schedules.id). user_id (null): If not NULL, this user is one recipient for the given schedule (foreign key profiles.userid). group_id (null): If not NULL, the members of this group are one recipient for the given schedule (foreign key groups.id). 2) Create the following classes: Whine (used to retrieve, update, and store whines), Whine::Queries (same as Whine, except for whining queries), Whine::Schedule (same for whining schedules), Whine::Recipient (same for whining recipients. Whine.pl and editwhines.cgi would be updated as needed to use the new classes. I'm mainly thinking of doing this so that future changes will be easier to implement. For example, implementing (1) means it will be easier for one schedule to be associated with more than one recipient. Reproducible: Always Steps to Reproduce:

A Whine::Recipient should just be a Bugzilla::User, yes? And a Whine::Query is a Bugzilla::Search. However, having a Whine object is a good idea. All of these separate changes (object, table structure, code re-working) should happen in separate bugs.

(In reply to comment #1) > A Whine::Recipient should just be a Bugzilla::User, yes? Right now a whining recipient can either be a user or a group. > And a Whine::Query is a Bugzilla::Search. Not entirely. A query is a reference to a saved search, with a whine-specific title, sorting key, and at least one additional option.

This is the class diagram that I am using. It shows all Whine classes that will appear in the upcoming Patch v1. Prefix "Bugzilla::" to all class names to get the actual Perl package/class names. Attributes are shown at the top of each class; each attribute matches a column in the associated database table. Attributes with the weird T shape next to them are foreign keys. Attributes with a / get their values from other attributes. Methods are shown at the bottom. The attached notes list all keys and indexes used by the database table associated with the class.

This is the class diagram that I am using. It shows all Whine classes that will appear in the upcoming Patch v1. Prefix "Bugzilla::" to all class names to get the actual Perl package/class names. Attributes are shown at the top of each class; each attribute matches a column in the associated database table. Attributes with the weird T shape next to them are foreign keys. Attributes with a / get their values from other attributes. Methods are shown at the bottom. The attached notes list all keys and indexes used by the database table associated with the class.

This is the class diagram that I am using. It shows all Whine classes that will appear in the upcoming Patch v1. Prefix "Bugzilla::" to all class names to get the actual Perl package/class names. Attributes are shown at the top of each class; each attribute matches a column in the associated database table. Attributes with the weird T shape next to them are foreign keys. Attributes with a / get their values from other attributes. Methods are shown at the bottom. The attached notes list all keys and indexes used by the database table associated with the class.

OK, here, finally, is the patch! I hopefully it'll only be attached once... The biggest changes are the additions of the 4 new Perl Modules. Bugzilla::Whine is directly associated with the whine_events table, and reads/validates/writes entries in/to that table. The same thing exists for Bugzilla::Whine::Schedule and whine_schedules, Bugzilla::Whine::Query and whine_queries, and Bugzilla::Whine::Recipients and whine_recipients. Once question you might have is "Why separate the recipients from the schedules"? Well, I'm looking forward to the time when you might have multiple recipients listed in a single schedule (which is bug 305370). All 4 modules include several operations: new, all_*_for_*, can_access, delete, validate, and write_to_database. Those operations are fairly self-explanatory, except for all_*_for_*, which returns a reference to an array of all Bugzilla::Whine::Query objects that are associated with a given Whine, all Bugzilla::Whine::Recipient objects that are associated with a given Bugzilla::Whine::Schedule, etc. Accessors are also included. Full PODs are also included. Bugzilla::Whine::Recipient deserves special mention since it contains a method to expand a recipient list (a user name and/or group name), replacing a group name with a list of member users (eliminating duplicates that appear). Bugzilla::Whine::Schedule also deserves special mention since it includes a method that calculates the next execution time of a schedule (not the easiest thing in the world) without relying on database queries. There have been some DB changes, including the addition of a new table. Bugzilla/DB/Schema.pm has been updated to reflect the changes. checksetup.pl has also been updated to incorporate the changes. whine.pl and editwhines.cgi have both been modified to take advantage of the new Perl Modules. Both scripts have been enhanced with more error checking and reporting, which means changes have been made to the global/user-error and global/code-error templates. sanitycheck.cgi has been updated to verify foreign key references in the couple of areas where they weren't checked. Other whining-related checks have also been added. Requesting review from LpSolit.

Comment on attachment 195382 [details] [diff] [review] Patch v1 joel: You seem to know a lot about whining. Would you be able to do a review of this patch?

This is just a basic test program that I used to test the Whine classes as I was creating them. I don't know if it will help anyone, but here it is just in case!

Comment on attachment 195382 [details] [diff] [review] Patch v1 And here comes person #3! Somewhere on the Internet someone is taking bets over who will come in with a review first 8-)

This is a modification of attachment 195382 [details] [diff] [review]. I found a few areas of Bugzilla where the whining-related tables were being accessed directly, and have updated them.

Karl: I've crawled out from under my rock. Want another reviewer?

Comment on attachment 196393 [details] [diff] [review] Patch v1.2 (In reply to comment #11) > Karl: > > I've crawled out from under my rock. Want another reviewer? Certainly! After some looking around I noticed that joel & jouni had reviewed previous major whining changed, so I imagine that they would like a break!

drat

Comment on attachment 196393 [details] [diff] [review] Patch v1.2 This patch has become at least a little bitrotten in the time since I requested review, so I'm pulling the review requests until I can get a newly-derotted version in. erik, jouni: If you managed to look over the patch at all, would you mind posting your comments? Thanks!

This bug is retargetted to Bugzilla 3.2 for one of the following reasons: - it has no assignee (except the default one) - we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1) - it's not a blocker If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0. If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug. If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2. This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.