174988 - post_bug.cgi and process_bug.cgi need to be combined

Status: RESOLVED WONTFIX

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Jeff Hedlund]

post_bug and process_bug have a lot of common code and need to be combined.

Comment 1

[Bradley Baetz (:bbaetz)]

This needs to be done via Bug.pm - its the sensible way, + the way of the future!

Comment 2

[Ronaldo Maia]

Attachment: clean up on post_bug.cgi

Comment 3

[Christian Reis]

Essentially this patch moves the Bug-specific code from post_bug to Bug.pm. It's
pretty much a cut-n-paste job with some added methods to do updating of the Bug
object.

The cool part about this is that process_bug can use these methods and become
quite a bit simpler. This is one of the large cleanups we need to do for
Bugzilla in order to survive.

Fixing a dependency -- it was really blocks.

Fred, do you think we should split this part into a separate bug?

Comment 4

[Frédéric Buclin]

kiko, please leave this dependency alone!! bug 242318 is a bug we want to fix
for 2.20. Yours is targetted 2.22.

And yes, this patch should be splitted into smaller pieces... as soon as the
actual blockers and the security patches have landed (this patch will bitrot
very soon).

The right way to go is to clean up both process_bug.cgi and post_bug.cgi at the
same time, meaning that the code doing the same job must be moved at the same
time. What is done here is not efficient, as a routine which fits perfectly for
post_bug.cgi has a high probability to need some modifications to suit for
process_bug.cgi.

Comment 5

[Christian Reis]

A few points:

  a) The reason I changed the depenency was that it becomes very easy to fix the
bug 242318 once this bug is fixed; I'm sorry I didn't notice the milestone differed.

  b) This dependency is pretty much useless as it stands, then -- there is no
patch on the dependency, and there is one here. The fact that the change there
will bitrot this patch doesn't seem to important enough to justify a dependency
-- any other patch which touches post_bug will do this.
  
  c) I completely refuse to accept that both process_bug and post_bug need to be
done at the same time. This is plain blocking good work done -- if you say "this
can't be done incrementally" then the odds are it won't be done at all. There is
nothing that stops us from doing post_bug and then adapting the methods when
process_bug is done -- in fact, that is the way it /should/ be done.

  d) There is nothing to split this patch out into if the review process isn't
as slow as molasses -- it simply cut-n-pastes code out of post_bug.cgi, and the
swifter, and the least that changes in that process, the better it is.

The only splitting that I suggested was splitting this bug off into a "Convert
post_bug.cgi to use Bug.pm"-themed bug, which I'll ask Ronaldo to do now. I can
do the review myself if noone else makes himself available in able time.

Comment 6

[Frédéric Buclin]

Comment on attachment 188450 [details] [diff] [review]
clean up on post_bug.cgi


>+for my $field ($cgi->param()) {
>+    if ( scalar(@{%form_data->{$field}} = $cgi->param($field)) == 1) {
>+        %form_data->{$field} = $cgi->param($field);
>     }
> }

Great! We did deFORMication (removing %::FORM stuff) and you bring it back
here.


>+my $bug = Bugzilla::Bug::CreateBug(\%form_data, $comment);

Why has $comment a special status?


>+    &::SendSQL("SELECT user_id FROM user_group_map

Deprecated routines. Please use DBI stuff.

Moreover, the patch doesn't apply cleanly anymore, see the dependent bugs.

Comment 7

[Ronaldo Maia]

Attachment: v2 - romaia

Comment 8

[Ronaldo Maia]

Attachment: v4-romaia - minor changes

fixing bitrot

Comment 9

[Vlad Dascalu]

Comment on attachment 191524 [details] [diff] [review]
v4-romaia -  minor changes

Nit:

-if (defined $cgi->param('product')) {
-    if (defined $cgi->param('version')) {
+if (defined $cgi->param('product') && defined $cgi->param('version')) {
	 $cgi->send_cookie(-name => "VERSION-$product",
			   -value => $cgi->param('version'),
			   -expires => "Fri, 01-Jan-2038 00:00:00 GMT");
-    }
 }


That needs to have the identation fixed as well.

Comment 10

[Max Kanat-Alexander]

Would it be at all possible to first add those "add_" functions to Bugzilla::Bug
in another bug, and move process_bug code to use them, and then do the whole
CreateBug thing in a separate bug?

This patch is just too big to review over and over, or even once.

Also, by the summary of this bug, all this work should be happening in a
blocker, not in this bug, anyhow.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 191524 [details] [diff] [review]
v4-romaia -  minor changes

Yeah, in fact, this definitely does generally need to be split into smaller
patches somehow.

Comment 12

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 13

[Frédéric Buclin]

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".

Comment 14

[Frédéric Buclin]

Max, agree to mark this bug as WONTFIX? I see no benefit in merging post_bug.cgi and process_bug.cgi together.

Comment 15

[Max Kanat-Alexander]

Yes, agreed.