Closed Bug 496488 Opened 16 years ago Closed 15 years ago

Hooks for creating, updating, and deleting groups (editgroups)

Categories

(Bugzilla :: Extensions, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: bbaetz, Assigned: mkanat)

References

Details

(Whiteboard: [es-ita])

Attachments

(1 file, 5 obsolete files)

Attached patch Add hooks and docs (obsolete) — Splinter Review
Add some hooks to editgroups for add/update/delete, allowing an extension to use that to make other changes/logging/etc
Attachment #381686 - Flags: review?(mkanat)
Whiteboard: [es-ita]
Comment on attachment 381686 [details] [diff] [review] Add hooks and docs Let's not add a delete hook for now, until we actually use $group->remove_from_db. Also, all the hooks need docs in Bugzilla::Hook.
Attachment #381686 - Flags: review?(mkanat) → review-
Attached patch Include template bits (obsolete) — Splinter Review
Was going to do this as a separate patch, but its all related. Add a field hook for list (create/update already have one) and let the changes be shown with a messages hook.
Attachment #381686 - Attachment is obsolete: true
Attachment #381688 - Flags: review?(mkanat)
Attached patch Add Hook.pm comments (obsolete) — Splinter Review
Our comments crossed - no midair protection! Delete is useful - theres no conflict with editgroups because either the change is rejected (via Throw*Error) in which case nothing happens, or it happens and changes made to the group object aren't important because it no longer exists.
Attachment #381688 - Attachment is obsolete: true
Attachment #381691 - Flags: review?(mkanat)
Attachment #381688 - Flags: review?(mkanat)
Need to wrap group creation into a transaction too (had this, but didn't apply it for the diff)
Attachment #381691 - Attachment is obsolete: true
Attachment #381709 - Flags: review?(mkanat)
Attachment #381691 - Flags: review?(mkanat)
What means [es-ita] ?? Espagna - Italia??
OS: Linux → All
Hardware: x86 → All
(In reply to comment #5) > What means [es-ita] ?? Espagna - Italia?? It's an internal tag used by Everything Solved to denote that this bug belongs to and is being funded by a particular client.
Attachment #381709 - Flags: review?(mkanat) → review-
Comment on attachment 381709 [details] [diff] [review] Wrap group creation into a transaction too >Index: editgroups.cgi > if ($action eq 'new') { >+ my $dbh = Bugzilla->dbh; >+ $dbh->bz_start_transaction(); >+ > check_token_data($token, 'add_group'); Hmmm...if there is a UserError or a CodeError during the transaction, should the token actually stay valid? (That's what putting the start_transaction above there will do.) >+ $dbh->bz_start_transaction(); >+ >+ Bugzilla::Hook::process('editgroups-delete', { >+ group => Bugzilla::Group->new($gid), >+ }); I'd like to see that created outside as: my $group = Bugzilla::Group->new($gid); So that other people will correctly re-use that object when they refactor this code in the future. >@@ -397,6 +411,8 @@ > $dbh->do('DELETE FROM groups WHERE id = ?', > undef, $gid); > >+ $dbh->bz_commit_transaction(); >+ > delete_token($token); Similarly to the above, should deleting the token be inside the transaction? >Index: Bugzilla/Hook.pm >+=head2 editgroups-create Let's call these group-before_update and group-before_create instead. We could even put it inside Bugzilla::Group->update/create if you want--would that satisfy our requirements? In fact, you could hook Bugzilla::Object->update/create and then just expect people to use $object->isa() in their code. I'm OK with hooking Object->update/create--they're not performance-critical code. >+=back >+ >+=head2 editgroups-delete And this would be group-before_delete. Ideally this should live in Bugzilla::Group->remove_from_db, but I think we don't use that yet. >Index: extensions/example/code/editgroups-create.pl >+my $params = Bugzilla->hook_args->{params}; >+ >+# Send mail/log/etc that a group has been created >+# This runs *before* the group has been created, so you can do any >+# extra checks you want, but there isn't a group object available You should probably demonstrate the sort of thing you would add to $params, even if you comment-out the line. >Index: extensions/example/code/editgroups-delete.pl >+use strict; >+use warnings; >+use Bugzilla; >+ >+my $group = Bugzilla->hook_args->{group}; >+ >+# Send mail/log/etc that a group has been deleted >+# This runs *before* the deletion actually happens, so you can do extra >+# checks and bail out if needed And an example of what you might do with $group here could be interesting, too. >Index: extensions/example/code/editgroups-update.pl >+my $group = Bugzilla->hook_args->{group}; >+ >+# Send mail/log/etc that a group has been updated >+# This runs *before* the update actually happens, so you can do extra >+# checks and bail out if needed And some other example here as well. Everything else looks great.
(In reply to comment #7) > (From update of attachment 381709 [details] [diff] [review]) > >Index: editgroups.cgi > > if ($action eq 'new') { > >+ my $dbh = Bugzilla->dbh; > >+ $dbh->bz_start_transaction(); > >+ > > check_token_data($token, 'add_group'); > > Hmmm...if there is a UserError or a CodeError during the transaction, should > the token actually stay valid? (That's what putting the start_transaction above > there will do.) I'd say it should - eg if I'm using a token to reset my password but the two passwords aren't the same/are too long/etc then I shouldn't need a new token. > > >+ $dbh->bz_start_transaction(); > >+ > >+ Bugzilla::Hook::process('editgroups-delete', { > >+ group => Bugzilla::Group->new($gid), > >+ }); > > I'd like to see that created outside as: > > my $group = Bugzilla::Group->new($gid); > > So that other people will correctly re-use that object when they refactor > this code in the future. I actually don't want them to do that without thinking about it. If/When we start doing updates through the group, we'll have to be sure to still pass in a group with useful variables. > > >@@ -397,6 +411,8 @@ > > $dbh->do('DELETE FROM groups WHERE id = ?', > > undef, $gid); > > > >+ $dbh->bz_commit_transaction(); > >+ > > delete_token($token); > > Similarly to the above, should deleting the token be inside the transaction? If the token delete fails then the group changes should still happen. > >Index: Bugzilla/Hook.pm > >+=head2 editgroups-create > > Let's call these group-before_update and group-before_create instead. We > could even put it inside Bugzilla::Group->update/create if you want--would that > satisfy our requirements? > > In fact, you could hook Bugzilla::Object->update/create and then just expect > people to use $object->isa() in their code. I'm OK with hooking > Object->update/create--they're not performance-critical code. Hmmmmmm. Seems a bit non-OO to do that. > >+=back > >+ > >+=head2 editgroups-delete > > And this would be group-before_delete. Ideally this should live in > Bugzilla::Group->remove_from_db, but I think we don't use that yet. Right.
I'm going to take this, just because I need it within a certain timeframe for a client.
Assignee: bbaetz → mkanat
Target Milestone: --- → Bugzilla 3.6
Summary: editgroups hooks → Hooks for creating, updating, and deleting groups (editgroups)
Depends on: 520318
Attached patch v5 (obsolete) — Splinter Review
Okay, here we go. This adds transactions around the create, update, and remove stuff. It adds the hooks, documentation, and example code. It requires bug 520318 in order to apply.
Attachment #381709 - Attachment is obsolete: true
Attachment #404491 - Flags: review?(dkl)
Comment on attachment 404491 [details] [diff] [review] v5 Granting myself review as module owner. Though I may remove group-before_delete in favor of object-before_delete. We'll see.
Attachment #404491 - Flags: review?(dkl) → review+
Flags: approval+
This is pending approval, waiting on the blocker.
Flags: approval+ → approval?
Component: Administration → Extensions
Flags: approval? → approval+
Comment on attachment 404491 [details] [diff] [review] v5 This patch needs to be updated to work with the new Hooks system.
Flags: approval+ → approval?
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Target Milestone: Bugzilla 3.8 → Bugzilla 3.6
(In reply to comment #13) > This patch needs to be updated to work with the new Hooks system. Can we get some traction on this one? We need the transactions it contains.
Flags: blocking3.6+
Attached patch v6Splinter Review
Okay, I've updated the patch for the modern extensions system, with no other changes.
Attachment #404491 - Attachment is obsolete: true
Attachment #425718 - Flags: review+
Flags: approval?
Flags: approval3.6+
Flags: approval+
Committing to: bzr+ssh://mkanat%40bugzilla.org@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Group.pm modified Bugzilla/Hook.pm modified extensions/Example/Extension.pm Committed revision 6965 Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/ modified Bugzilla/Group.pm modified Bugzilla/Hook.pm modified extensions/Example/Extension.pm Committed revision 6960.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: