418342 - (bz-bug-set-all) Implement Bugzilla::Bug set_all, which takes parameters from a hash and does all updates in the right order

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Right now if we want to re-use the Bugzilla::Bug update()-based code anywhere else (like in the WebService or the email interface) we would have to basically copy all of process_bug.cgi.

Instead, we should have a method called set_all that just takes a hash of parameters and does all of the "setting" in the right order.

Comment 1

[Noura Elhawary]

Attachment: patch for new bug object method Bugzilla::Bug->set_all($args)

Hi Max,, I have started writing the code for the function Bugzilla::Bug->set_all , basically I used most of the code in process_bug.cgi ,, Please take a look when you can and let me know if it is any close to what you had in mind. 

Thanks,
Noura

Comment 2

[Max Kanat-Alexander]

Comment on attachment 304665 [details] [diff] [review]
patch for new bug object method Bugzilla::Bug->set_all($args)

This looks basically good, but you need to actually make process_bug use it, as part of this patch.

>+sub set_all {
>+                                          other_bugs => $args->{bug_objects},

  Call it other_bugs instead.

>+        $product_change ||= $changed;                                         

  You've never defined this variable.
                                          
>+    # add/remove groups
>+    $self->remove_group($_) foreach @{$args->{remove_group}};
>+    $self->add_group($_) foreach @{$args->{add_group}};

  What if those are undef?

>+    # set keywords , the hash key for the keywords can either be 
>+    # add_keywords, delete_keywords, or makeexact_keywords
>+    # get the action from the hash key name
>+    foreach my $key_name (keys %$args){
>+        if ($key_name =~ /([a-z]*)_keywords$/){

  [a-z]+, not *. And it should be ^([a-z]+)

>+    # Component, target_milestone, and version are in here just in case
>+    # the 'product' field wasn't defined in the args hash. It doesn't hurt 
>+    # to set them twice.
>+    my @set_fields = qw(op_sys rep_platform priority bug_severity
>+                        component target_milestone version
>+                        bug_file_loc status_whiteboard short_desc
>+                        deadline remaining_time estimated_time);

  That should go into a constant, I think. But maybe not. What do you think?

>+    push(@set_fields, 'assigned_to') if !$args->{set_default_assignee};
>+    push(@set_fields, 'qa_contact')  if !$args->{set_default_qa_contact};

>+    my @custom_fields = Bugzilla->get_fields({custom => 1, obsolete => 0});

  Use the new Bugzilla->active_custom_fields instead.

>+    my %methods = (
>+        bug_severity => 'set_severity',
>+        rep_platform => 'set_platform',
>+        short_desc   => 'set_summary',
>+        bug_file_loc => 'set_url',
>+    );

  Instead of having this here, the parameters should be named correctly. That is, instead of short_desc we should be calling the input parameter "summary".

  There are probably more comments, but that's as far as I got in the review for now.


  Also, generally, you should wait to write this patch until we've done a bit more work on process_bug.cgi.

Comment 3

[Noura Elhawary]

> >+    # the 'product' field wasn't defined in the args hash. It doesn't hurt 
> >+    # to set them twice.
> >+    my @set_fields = qw(op_sys rep_platform priority bug_severity
> >+                        component target_milestone version
> >+                        bug_file_loc status_whiteboard short_desc
> >+                        deadline remaining_time estimated_time);
> 
>   That should go into a constant, I think. But maybe not. What do you think?
> 
I think it is a good idea to put it into a constant as these values will have to be called in both set_all and also process_bug.cgi ,, 

I am attaching a patch for process_bug.cgi that calls set_all function.

Comment 4

[Noura Elhawary]

Attachment: v2 for Bugzilla::Bug::set_all + process_bug.cgi

Hi Max,,

here is the patch with process_bug.cgi that can call the set_all function ,, please review when you can.

Thanks,
Noura

Comment 5

[Noura Elhawary]

Attachment: v2 for Bugzilla::Bug::set_all + process_bug.cgi

Please ignore previous patch, it had little mistake.

Comment 6

[Noura Elhawary]

This bug will be divided to smaller bugs and will be used to track them. 

Comment 7

[Tiago Mello [:timello]]

Hi Noura,

I see you have been working on this bug and I'm really interested in your patch. I'm looking forward to have bug 415813 implemented. Do you have any status on that? How is your progress?

Thank you in advance.

Comment 8

[Noura Elhawary]

Attachment: set_all function for base class Bugzilla/Bug.pm

Function set_all needs to be added to base class Bugzilla/Bug.pm for webservice function Bug.update to work properly

Comment 9

[Noura Elhawary]

Attachment: WebService function Bug.update

WebService function Bug.update to be added to Bugzilla/WebService/Bug.pm and it depends on the function set_all included in the previous attachment

Comment 10

[Noura Elhawary]

Hi Tiago,

Sorry for the late reply, dkl mentioned to me your request,, We have actually got the webservice function Bug.update working in our redhat bugzilla instance, I attached our code for this function which is the set_all function and the Bug.update function, I hope this would help you. Let me know if you have any questions.

Cheers,
Noura

(In reply to comment #7)
> Hi Noura,
> 
> I see you have been working on this bug and I'm really interested in your
> patch. I'm looking forward to have bug 415813 implemented. Do you have any
> status on that? How is your progress?
> 
> Thank you in advance.

Comment 11

[Tiago Mello [:timello]]

Comment on attachment 378335 [details]
set_all function for base class Bugzilla/Bug.pm

Hi Max! Could you review those patches? If Noura doesn't mind I can fix further issues with the patches.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 378336 [details]
WebService function Bug.update

Why is this patch even on this bug? Wrong bug.

Comment 13

[Noura Elhawary]

(In reply to comment #11)
> (From update of attachment 378335 [details])
> Hi Max! Could you review those patches? If Noura doesn't mind I can fix further
> issues with the patches.

Hi Tiago,

Upstream's approach to this bug was to break down the set_all function to little pieces and do the needed change to process_bug.cgi, there has already been some work towards that approach, but in redhat bugzilla we needed this functionality asap so we had to implement it our way, I attached for you those patches taken from our code and I haven't tested it against upstream bugzilla , so it is not really a patch for upstream bugzilla, it was just to help you with your request.

Noura

Comment 14

[Michael Thomas (Mockodin)]

Attachment: Bugzilla::Bug - Bugzilla-3_6-BRANCH (raw complete edited file)

Bulk ported checks from process_bug.cgi, a number of items which we first_bug only now set for all bugs, pretty much everything except flags which is still in process_bugs.cgi

Bugs being moved will exit set_all early, leaves out set_status to be processed in existing block in process_bug.cgi

Comment 15

[Michael Thomas (Mockodin)]

Attachment: process_bug.cgi - Bugzilla-3_6-BRANCH (raw complete edited file)

Moves all set calls into Bugzilla::Bug->set_all ( attachment 426181 [details] ) except those noted in previous comment

Comment 16

[Michael Thomas (Mockodin)]

Attachment: v3 against Bugzilla-3_6-BRANCH

It was pointed out to me that I should have just gone ahead and posted a patch. Sorry about that, I hadn't originally planned on doing so but agree that I ought to have.

Comment 17

[Michael Thomas (Mockodin)]

Attachment: v4 against Bugzilla-3_6-BRANCH

removed two junk items from the patch

Comment 18

[Max Kanat-Alexander]

Comment on attachment 426190 [details] [diff] [review]
v4 against Bugzilla-3_6-BRANCH

This patch is too big; I'd really like it to be split up into separate patches.

Also, set_all is supposed to be an implementation of Bugzilla::Object::set_all, which takes a set of parameters. It is not supposed to directly read Bugzilla->input_params.

The way that this should be done is that anything that wouldn't pass should_set should not be passed to the function.

The simplest start to this would be to convert the section that involves @set_fields into a mostly-standard implementation of Bugzilla::Object::set_all. (That would be done in a separate bug that blocks this one.)

Comment 19

[Max Kanat-Alexander]

Done! Thanks to dkl and timello for the reviews!

Comment 20

[Max Kanat-Alexander]

Added to the release notes in bug 604256.