Closed
Bug 496488
Opened 16 years ago
Closed 15 years ago
Hooks for creating, updating, and deleting groups (editgroups)
Categories
(Bugzilla :: Extensions, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.6
People
(Reporter: bbaetz, Assigned: mkanat)
References
Details
(Whiteboard: [es-ita])
Attachments
(1 file, 5 obsolete files)
4.87 KB,
patch
|
mkanat
:
review+
|
Details | Diff | 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)
Reporter | ||
Updated•16 years ago
|
Whiteboard: [es-ita]
Assignee | ||
Comment 1•16 years ago
|
||
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-
Reporter | ||
Comment 2•16 years ago
|
||
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)
Reporter | ||
Comment 3•16 years ago
|
||
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)
Reporter | ||
Comment 4•16 years ago
|
||
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)
![]() |
||
Comment 5•16 years ago
|
||
What means [es-ita] ?? Espagna - Italia??
Assignee | ||
Updated•16 years ago
|
OS: Linux → All
Hardware: x86 → All
Assignee | ||
Comment 6•16 years ago
|
||
(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.
Assignee | ||
Updated•16 years ago
|
Attachment #381709 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 7•16 years ago
|
||
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.
Reporter | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Summary: editgroups hooks → Hooks for creating, updating, and deleting groups (editgroups)
Assignee | ||
Comment 10•15 years ago
|
||
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)
Assignee | ||
Comment 11•15 years ago
|
||
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+
Assignee | ||
Updated•15 years ago
|
Flags: approval+
Assignee | ||
Comment 12•15 years ago
|
||
This is pending approval, waiting on the blocker.
Flags: approval+ → approval?
Assignee | ||
Updated•15 years ago
|
Component: Administration → Extensions
Assignee | ||
Updated•15 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 13•15 years ago
|
||
Comment on attachment 404491 [details] [diff] [review]
v5
This patch needs to be updated to work with the new Hooks system.
Assignee | ||
Updated•15 years ago
|
Flags: approval+ → approval?
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Assignee | ||
Updated•15 years ago
|
Target Milestone: Bugzilla 3.8 → Bugzilla 3.6
![]() |
||
Comment 14•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Flags: blocking3.6+
Assignee | ||
Comment 15•15 years ago
|
||
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+
Assignee | ||
Updated•15 years ago
|
Flags: approval?
Flags: approval3.6+
Flags: approval+
Assignee | ||
Comment 16•15 years ago
|
||
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.
Description
•