Closed Bug 530468 Opened 15 years ago Closed 14 years ago

Create a read-only Bugzilla::Whine object for whine events and have editwhines.cgi use it

Categories

(Bugzilla :: Whining, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: eblack, Assigned: eblack)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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, bug 530467 for read only queries, and this is the third bug for the read only whine event objects


Reproducible: Always
Blocks: 509959
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
This one is about whine events, the other one about whine queries.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
(In reply to comment #2)
> This one is about whine events, the other one about whine queries.

Whoops!  My apologies.
Assignee: whining → eblack
Status: UNCONFIRMED → NEW
Ever confirmed: true
Eric: I added you to the canconfirm and editbugs groups today, so you can now assign bugs to yourself and confirm your own bugs if you think that they should be confirmed.
Summary: Create a read-only object using Bugzilla::Object for whine events → Create a read-only Bugzilla::Whine::Event object for whine events and have editwhines.cgi use it
Max, I'm adding this patch for the event object, but if you look at editwhines, there are still alot of select statements that are joins between two tables/objects, and selects of multiple events (this object). They don't seem to belong in this object class, but I think they should be in an overall 'whine' object. I think that's what you meant in Bug 509959. Please let me know if that is correct and if I should create another bug for a read-only Bugzilla::Whine object. Thanks
Attachment #426613 - Flags: review?(mkanat)
Attachment #426613 - Attachment is patch: true
Attachment #426613 - Attachment mime type: application/octet-stream → text/plain
Hmm, an Event *is* a Whine, right? I think we should just call this one Bugzilla::Whine.

Selects of multiple events are handled by new_from_list and match(). I suspect that the joins that we have in direct SQL are not necessary and can be re-implemented in an object-oriented fashion.
Comment on attachment 426613 [details] [diff] [review]
Read only Bugzilla::Whine::Event mod and changes to editwhines.cgi

>Index: editwhines.cgi
> sub get_events {
>     my $userid = shift;
>     my $dbh = Bugzilla->dbh;
>     my $events = {};
> 
>-    my $sth = $dbh->prepare("SELECT DISTINCT id, subject, body, mailifnobugs " .
>+    my $sth = $dbh->prepare("SELECT DISTINCT id " .
>                             "FROM whine_events " .
>                             "WHERE owner_userid=?");

  This can be replaced with:

  my $events = Bugzilla::Whine->match({ owner_userid => $userid });

>+++ Bugzilla/Whine/Event.pm	2010-01-31 14:39:55.000000000 -0500
[snip]
>+sub get_schedules {

  We don't prefix our accessors with get_, so this would just be "schedules".

  Also, I didn't see editwhines.cgi using any of these complex accessors?

>+sub whine_users {

  I think this might be better as to_users?

>+    my $ids = Bugzilla->dbh->selectcol_arrayref('SELECT owner_userid 
>+                                                   FROM whine_events 
>+                                             INNER JOIN whine_queries 
>+                                                     ON whine_events.id = whine_queries.eventid 
>+                                                  WHERE query_id = ? and owner_userid != ?',

  I'm not quite familiar enough with the whine code to understand this, but why are you checking query_id as the event's id? Also, why even join whine_queries for this? Isn't owner_userid from this table, and it will just return one user?

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

  I'd rather have only "sub owner" or "sub user" that returns a Bugzilla::User object, unless there's a good reason. If there *is* a good reason to keep this, it should be called owner_id or user_id probably.
Attachment #426613 - Flags: review?(mkanat) → review-
Summary: Create a read-only Bugzilla::Whine::Event object for whine events and have editwhines.cgi use it → Create a read-only Bugzilla::Whine object for whine events and have editwhines.cgi use it
Target Milestone: --- → Bugzilla 3.8
Hey Eric. Any hope of getting an updated version of this patch some time soon?
(In reply to comment #8)
> Hey Eric. Any hope of getting an updated version of this patch some time soon?

My apologies, yes, I'll work on it. Thanks for reminding me.
I changed this from a Bugzilla::Whine::Event object to a Bugzilla::Whine object. 

I also removed the complex accessors for now as they were for the sql joins in editwhines.cgi, but I agree that they might not be necessary. I was figuring on incremental patch submissions to remove them to make it simpler, but your suggestion is simpler, and the sql joins can be handled later.

A user object is now returned rather than the userid for the 'user' method.
Attachment #426613 - Attachment is obsolete: true
Attachment #455046 - Flags: review?(mkanat)
Comment on attachment 455046 [details] [diff] [review]
New patch of read only Bugzilla::Whine object and editwhines.cgi updates to use it

This looks simple and excellent. Thank you! :-)
Attachment #455046 - Flags: review?(mkanat) → review+
Flags: approval+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified editwhines.cgi
added Bugzilla/Whine.pm
Committed revision 7270.
Status: NEW → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: