Last Comment Bug 556422 - (bz-oldbugmove) The existing bug-moving functionality should become an extension
(bz-oldbugmove)
: The existing bug-moving functionality should become an extension
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 3.7
: All All
: -- enhancement (vote)
: Bugzilla 4.0
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
Depends on: 539865 556695 556901 567846
Blocks: bz-bug-set-all 581690 581693 757935
  Show dependency treegraph
 
Reported: 2010-03-31 16:35 PDT by Max Kanat-Alexander
Modified: 2012-09-26 04:39 PDT (History)
3 users (show)
mkanat: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Work In Progress (25.28 KB, patch)
2010-04-02 14:24 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v1 (49.44 KB, patch)
2010-04-02 16:05 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v2 (bundle) (50.46 KB, patch)
2010-05-24 15:53 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v3 (bundle) (48.72 KB, patch)
2010-06-11 14:39 PDT, Max Kanat-Alexander
dkl: review-
Details | Diff | Splinter Review
v4 (bundle) (52.24 KB, patch)
2010-06-16 21:25 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v5 (bundle) (49.55 KB, patch)
2010-06-17 15:02 PDT, Max Kanat-Alexander
dkl: review+
Details | Diff | Splinter Review

Description Max Kanat-Alexander 2010-03-31 16:35:57 PDT
I don't know how many, if any, installations are using the bug-moving functionality. It should become an extension.

importxml.pl can stay in the main code, because it has other uses than just importing moved bugs.
Comment 1 Max Kanat-Alexander 2010-04-02 14:24:32 PDT
Created attachment 436771 [details] [diff] [review]
Work In Progress
Comment 2 Max Kanat-Alexander 2010-04-02 16:05:41 PDT
Created attachment 436796 [details] [diff] [review]
v1

  Okay, here we go. This doesn't work quite yet, because other code is required.

  I removed several parameters and replaced them with better default behavior.

  I also removed the move-multiple-bugs functionality, because, according to some folks on the support list, importxml hasn't been able to import multiple bugs for quite some time, and it made it much simpler to make this into an extension.

  I couldn't test all the changes to importxml.pl, because apparently importxml.pl isn't working at all on trunk for reading bug-move emails (maybe it hasn't been working for some time). I guess that shows you how many people use bug moving....
Comment 3 Max Kanat-Alexander 2010-05-24 15:53:22 PDT
Created attachment 447209 [details] [diff] [review]
v2 (bundle)

  Okay, provided that the dependency chain is satisfied, this one works! :-)
Comment 4 Max Kanat-Alexander 2010-06-11 14:39:16 PDT
Created attachment 450750 [details] [diff] [review]
v3 (bundle)

The v2 bundle wasn't applying on other machines than landfill. This one should work.
Comment 5 David Lawrence [:dkl] 2010-06-16 21:01:35 PDT
Some initial thoughts:

1. Why call it OldBugMove as opposed to just BugMove. BugMove still sound slike it could be useful for those that need it.

2. When moving a bug, it adds a blank comment which is confusing to the move bug which is confusing. The comment is being added in object_end_of_set() where it is setting type => CMT_MOVED_TO. I see where format_comment-type.html.tmpl is supposed to be adding the text but it is not working for me.

3. When trying to delete the MOVED resolution in editvalues.cgi, instead of throwing a proper error it instead dies with:

Undefined subroutine &Bugzilla::Extension::OldBugMove::ThrowUserError called at ./extensions/OldBugMove/Extension.pm line 78.

extensions/OldBugMove/Extension.pm needs to import Bugzilla::Error.

4. Should the [Move bug to...] button still be visible when a bug has already been moved and set to RESOLVED/MOVED.

5. The product and component config for GetOptions in importxml.pl needs to
be "product=s" and "component=s" respectively.
 
6. importxml.pl when importing a bug that has a product that does not exist, without specifying a default product, instead of displaying the descriptive error you wrote it dies instead with:

Bad argument param sent to Bugzilla::Product::new function.
 at ./importxml.pl line 1301

Similar with a default product but no component specified, it dies with:

Bad argument name sent to Bugzilla::Component::new function.
 at ./importxml.pl line 1301

When specifying a default product and default component, it dies with the error: 

Bad argument name sent to Bugzilla::Component::new function.
 at ./importxml.pl line 1301

-        $component = new Bugzilla::Component({
-           component => $default_component_name, product => $product });
+        $component = new Bugzilla::Component({
+           name => $default_component_name, product => $product });

7. The pod documentation in importxml.pl needs to be updated so that importxml.pl --help shows the new default product and component options.

8. Due to Bugzilla::Error not being in Extension.pm, when manually changing a bug to RESOLVED/MOVED without clicking the [Move bug to...] button, it dies with the following error:

Undefined subroutine &Bugzilla::Extension::OldBugMove::ThrowCodeError called at ./extensions/OldBugMove/Extension.pm line 137.

Dave
Comment 6 Max Kanat-Alexander 2010-06-16 21:04:24 PDT
(In reply to comment #5)
> 1. Why call it OldBugMove as opposed to just BugMove. BugMove still sound slike
> it could be useful for those that need it.

  Because there will be another solution for moving bugs in the future, using RPC.

  Everything else, I will fix! :-)
Comment 7 Max Kanat-Alexander 2010-06-16 21:25:36 PDT
Created attachment 451830 [details] [diff] [review]
v4 (bundle)

Okay, this fixes all the issues that you found. Thank you for finding all of those! :-)

I fixed the importxml.pl POD in general so that it's actual normal pod2usage style, now.
Comment 8 David Lawrence [:dkl] 2010-06-17 12:42:55 PDT
Ok, all of my suggestions have been addressed except scenario #8 from comment 5 is doing something different. When I try to do the manual move by changing a bug to RESOLVED/MOVED without clicking the [Move bug to ...] button, I get the following internal error:

Bugzilla has suffered an internal error. Please save this page and send it to dkl@redhat.com with details of what you were doing at the time this message appeared.

URL: http://10.211.55.113/bugzilla/process_bug.cgi
You cannot set the resolution of a bug to MOVED without moving the bug.

Traceback:

 at ./extensions/OldBugMove/Extension.pm line 138
	Bugzilla::Extension::OldBugMove::_check_bug_resolution(...) called at ./extensions/OldBugMove/Extension.pm line 127
	Bugzilla::Extension::OldBugMove::__ANON__(...) called at Bugzilla/Object.pm line 302
	Bugzilla::Object::set(...) called at Bugzilla/Bug.pm line 2322
	Bugzilla::Bug::set_resolution(...) called at Bugzilla/Bug.pm line 2385
	Bugzilla::Bug::set_bug_status(...) called at Bugzilla/Object.pm line 329
	Bugzilla::Object::set_all(...) called at Bugzilla/Bug.pm line 1980
	Bugzilla::Bug::set_all(...) called at /var/www/html/bugzilla/process_bug.cgi line 380
Comment 9 Max Kanat-Alexander 2010-06-17 14:57:48 PDT
(In reply to comment #8)
> Ok, all of my suggestions have been addressed except scenario #8 from comment 5
> is doing something different. When I try to do the manual move by changing a
> bug to RESOLVED/MOVED without clicking the [Move bug to ...] button, I get the
> following internal error:

  That's actually the correct behavior--it's a CodeError. I suppose a UserError would make more sense, though. I'll change it.
Comment 10 Max Kanat-Alexander 2010-06-17 15:02:32 PDT
Created attachment 452095 [details] [diff] [review]
v5 (bundle)

Okay, here's the patch with that error changed to a UserError.
Comment 11 David Lawrence [:dkl] 2010-06-18 10:22:26 PDT
Comment on attachment 452095 [details] [diff] [review]
v5 (bundle)

Looks good now. Testing all works good for me. r=dkl
Comment 12 Max Kanat-Alexander 2010-06-18 13:50:35 PDT
  Awesome, thanks for reviewing that huge patch, dkl!! :-)


Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified importxml.pl
modified process_bug.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Constants.pm
modified Bugzilla/DB.pm
modified Bugzilla/User.pm
modified Bugzilla/Field/ChoiceInterface.pm
added extensions/OldBugMove
added extensions/OldBugMove/Config.pm
added extensions/OldBugMove/Extension.pm
added extensions/OldBugMove/disabled
added extensions/OldBugMove/lib
added extensions/OldBugMove/template
renamed Bugzilla/Config/BugMove.pm => extensions/OldBugMove/lib/Params.pm
added extensions/OldBugMove/template/en
added extensions/OldBugMove/template/en/default
added extensions/OldBugMove/template/en/default/admin
added extensions/OldBugMove/template/en/default/hook
added extensions/OldBugMove/template/en/default/admin/params
renamed template/en/default/admin/params/bugmove.html.tmpl => extensions/OldBugMove/template/en/default/admin/params/oldbugmove.html.tmpl
added extensions/OldBugMove/template/en/default/hook/bug
added extensions/OldBugMove/template/en/default/hook/global
added extensions/OldBugMove/template/en/default/hook/bug/edit-after_comment_textarea.html.tmpl
added extensions/OldBugMove/template/en/default/hook/bug/format_comment-type.txt.tmpl
added extensions/OldBugMove/template/en/default/hook/global/user-error-auth_failure_action.html.tmpl
added extensions/OldBugMove/template/en/default/hook/global/user-error-errors.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/format_comment.txt.tmpl
modified template/en/default/global/code-error.html.tmpl
modified template/en/default/global/field-descs.none.tmpl
modified template/en/default/global/user-error.html.tmpl                       
modified template/en/default/list/edit-multiple.html.tmpl
modified template/en/default/pages/fields.html.tmpl
Committed revision 7217.
Comment 13 Max Kanat-Alexander 2010-10-21 19:39:43 PDT
Added to the release notes in bug 604256.
Comment 14 Frédéric Buclin 2011-10-04 14:42:09 PDT
$user->is_mover no longer exists, and so must be removed from the POD.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/User.pm
Committed revision 7973.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/User.pm
Committed revision 7939.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/User.pm
Committed revision 7650.

Note You need to log in before you can comment on or make changes to this bug.