415813 - (bz-ws-update) Implement Bug.update() as an API for WebServices

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement, P1)

Description

[Frédéric Buclin]

We should now be able to update bugs using Webservice. That was the goal to implement $bug->update. The format could be of the form:

{ id => 106983, attributes => {status => 'NEW', priority => 'P3', severity => 'critical'} }

and Bug.update() would return all changes so that the caller can check what has been altered.

Probably not a blocker for 3.2 (despite I wouldn't be against), but definitely a highly wanted feature.

Comment 1

[Olav Vitters]

What about mass updates? Meaning: multiple bugs in one call.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> What about mass updates? Meaning: multiple bugs in one call.

OK, so:

{bug_ids => [106983, 88520], attributes => {status => 'NEW', priority => 'P3'}}

Comment 3

[Max Kanat-Alexander]

Bugzilla 3.2 is frozen as of Monday, so this would go into Bugzilla 4.0.

"bug_ids" should just be "ids". I'm also not sure there's a reason to have the "attributes" in their own hash, although that does make the interface more extendable in the future, so perhaps it's a good idea.

This requires that we first implement a set_all function in Bugzilla::Bug that can take a hash of values and call all the set_ or add_ functions in the right order.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> Bugzilla 3.2 is frozen as of Monday, so this would go into Bugzilla 4.0.

This is ridiculous. Note only should we take improvements about webservice till 3.2rc1 as long as it doesn't affect the core code, but the whole goal of implementing $bug->update was to be able to do such changes from the webservice. That's definitely a non-sense.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
>. Note only should we take improvements about webservice till
> 3.2rc1 as long as it doesn't affect the core code

  Absolutely not.

  An API is one of the most important things to be stable in any computer system. When you have to change or repair your APIs in future releases, it causes *extreme *complexity for the users of that API, and also for us as maintainers. Once an API function is in, we cannot even release an RC until we're fairly sure the actual parameters and return values will be stable forever, and that takes time, the kind of time that delays releases.

  Also, as we have consistently seen, adding WebService functions is not as simple as "just call some core code". Look at the security issues we've introduced, the bugs we've had to fix, and the general complexity of implementing some of our WebService functions.

  This function in particular would currently require that we COPY ALL OF PROCESS_BUG.CGI into WebService::Bug, a massive amount of code duplication that would take forever to stabilize and introduce numerous bugs, some of them possibly very serious. The other option is to implement Bugzilla::Bug->set_all as I said in comment 3, which is *another* huge, invasive change.

  Every time you make some enhancing change to a system, no matter how small, you delay its release date. Every single enhancement we make to the Bugzilla 3.2 branch delays the release of Bugzilla 3.2, which is already months and months late, and we haven't even had a Release Candidate yet! 

  The firmer I hold the position of "no enhancements on this branch," the sooner we release, and that is my goal.

Comment 6

[Reed Loden [:reed]]

We want this for bmo eventually (sooner rather than later).

Comment 7

[Max Kanat-Alexander]

This is a "no", unfortunately on blocking3.2, for the reasons listed in comment 5. I did all of the process_bug work for 3.2 specifically to enable this, but it took too long and now we have to release. There are a lot of great enhancements in Bugzilla 3.2. Unfortunately, this won't be one of them, but there are still enough by far to justify a release.

Comment 8

[Noura Elhawary]

Hi ,, I started looking at this bug and as Max said it seems like it will need a lot of work, So I thought I'd better start discussing the design with you before i start writing any code just to get the right guidelines .

Bugzilla::WebService::Bug::update() design
------------------------------------------
1- function Params:
{ids => [106983, 88520], attributes => {status => 'ASSIGNED', priority => 'P3'}} 

2- Return values:
a hash with a signle value "changes" pointing to an array of hashes with each hash including the bug id and the changes made like this for example

{ changes => [
               {id => 106983, old_status => 'NEW' , new_status => 'ASSIGNED',
                old_priority => 'P1', new_priority => 'P3'},

               {id => 88520, old_status => 'NEW' , new_status => 'ASSIGNED',
                old_priority => 'P2', new_priority => 'P3'},
             ]
} 

does that look good, maybe we don't need to mention old and new , only mention the new value ?

3- Things to check before we can perform any bug updates "I guess this should be done in the beginning of the Bug.update function code":
  - The user is logged in "LOGIN_REQUIRED"
  - if no bug ids are passed then throw an error no_bugs_chosen
  - validate bug ids
  - get bug objects 
  - for each bug check if the user can edit it based on its product, using
    $user->can_edit_product
  - if it is a mass bug change, the following field changes can not be  
    implemented:  
    "blocked, dependson, bug_alias, setting dupe id for the bug"
    So what shall we do here ? shall we throw an error if the user 
    passes more than one bug id and one of the above fields in the attributes 
    hash? 
  - the product change as in process_bug.cgi is performed before all the other 
    changes , but it is more complicated as it needs the user to supply 
    with the new product the component, version, target milestone, also the 
    bug groups will be affected ,, shall we consider the product change 
    here in Bug.update? or shall it have its own function?
  - check for mid air collision when updating only single bug.
  - also for cclist changes there is a special case , as to the cclist
    we can either add or remove from the list for changing , shall this be 
    also a separate WebService function as it requires an action to be 
    specified by the user add/remove
  - same thing with bug groups, we can either add or remove bug group
    shall this also be separate function?


4- after all the above points passes and all good then we are ready to do the updates ,, using a loop we go through the bug id list that is passed to Bug.update we call the function "to be developed" Bugzilla::Bug->set_all , we will pass to this function a single bug id and the attributes hash

5- The internal work of Bugzilla::Bug will be that the function Bugzilla::Bug->set_all will call the appropriate set/add function for each value passed to it based on the hash keys, then it should return all the new values that were set in a hash.

6- Bugzilla::WebService::Bug::update() emails all the relevant recipients the changes and also checks if the dupe id is set then the users on the other bug also will be emailed with the changes. and it returns  a hash with the changes to the user who called the WebService function.

please let me know what you think?

Thanks,
Noura

Comment 9

[Max Kanat-Alexander]

Hey Noura. Actually, right now I'd say don't even *think* about this bug until bug 418342 is done.

I do have a few notes on what you said, although this isn't everything:

I'm not sure we should call the input hash "attributes", that's a very complex word.

Your return value should be more like a hash of bug IDs pointing to the "changes" hash returned by $bug->update() for each bug. Note that somehow you will have to correctly call type() on all the values that need it in the return value.

Yes, you should throw an error if somebody tries to mass-change a field they can't mass-change. Also, we should make those fields mass-changeable before we implement this interface. (Except alias, which never will be mass-changeable.)

There is no such thing as a mid-air collision in the XML-RPC interface.

I guess for CC's we can call the parameters add_cc and remove_cc. (For groups, add_group and remove_group. Note that you will have to have parameters like this for all multi-select fields, also.)

The method is $bug->set_all. It's not going to be some class function (that would be stupid). It'll be an object method.

update() does not yet send emails, but it probably will in the future. I suppose we should have it do that before we start with this bug.

Comment 10

[Noura Elhawary]

(In reply to comment #9)
> Hey Noura. Actually, right now I'd say don't even *think* about this bug until
> bug 418342 is done.
> 
Hey Max ,, yeah makes sense to me that we should get the set_all function implemented first , so I will focus for now on helping out with bug 418342.

Comment 11

[Max Kanat-Alexander]

Attachment: Work In Progress

Comment 12

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go! This requires the entire dependency chain in order to apply and work. I have tested it minimally, but it probably needs a lot more testing. I suspect that it won't get *fully* tested until I write a Selenium script for it, so I might just go ahead and do that.

There are some things that I know are incomplete or don't yet work, marked by FIXME in the patch. However, it's more or less ready for review.

Comment 13

[David Lawrence [:dkl]]

Since I have review+ bug 556422, I assume this patch is now ready to go but I get the following when I try to apply it:

[dkl@localhost trunk]$ patch -p0 < ~/Downloads/bug-update.diff 
patching file Bugzilla/Bug.pm
Hunk #2 succeeded at 1452 (offset -86 lines).
Hunk #3 succeeded at 1469 (offset -86 lines).
Hunk #4 succeeded at 1927 (offset -85 lines).
Hunk #5 succeeded at 1977 (offset -85 lines).
Hunk #6 succeeded at 2024 (offset -85 lines).
Hunk #7 succeeded at 2459 (offset -89 lines).
Hunk #8 succeeded at 2491 (offset -89 lines).
patching file Bugzilla/Object.pm
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file Bugzilla/Object.pm.rej
patching file Bugzilla/WebService/Bug.pm
patching file Bugzilla/WebService/Constants.pm
patching file process_bug.cgi
Hunk #1 succeeded at 247 (offset 13 lines).
Hunk #2 succeeded at 259 (offset 13 lines).
Hunk #3 succeeded at 270 (offset 13 lines).
patching file template/en/default/global/user-error.html.tmpl

Will the patch need to be updated now that bug 556422 is complete and bug 418342 can be closed?

Dave

Comment 14

[David Lawrence [:dkl]]

Also, t/001compile.t reports the following:

# syntax error at process_bug.cgi line 265, near ")]"
# process_bug.cgi had compilation errors.
not ok 43 - process_bug.cgi
#   Failed test 'process_bug.cgi'
#   at t/001compile.t line 73.
# --ERROR

Comment 15

[Max Kanat-Alexander]

Ah, there are a few things that need to be fixed. Also, I think to make it easier for you, I'm going to split the non-WS changes into a separate patch.

Comment 16

[Max Kanat-Alexander]

Attachment: v2

Okay, here's a patch with *just* the Bug.update changes. The keywords and set_all blocker need to be resolved in order for this to work. The "groups" blocker does not need to be resolved in order for this to work, but the "groups" item will not work properly until that blocker is resolved.

Comment 17

[Max Kanat-Alexander]

Note to self: I also need to add a clear warning to Bugzilla::Bug that adding any new set_ function will automatically expose that function as an API in the WebServices, via Bug.update, now.

Comment 18

[Frédéric Buclin]

dkl: ping? This is the last enhancement bug we want for 4.0. Then, 4.0 will be feature complete.

Comment 19

[David Lawrence [:dkl]]

(In reply to comment #18)
> dkl: ping? This is the last enhancement bug we want for 4.0. Then, 4.0 will be
> feature complete.

Back from holiday now so will get to this one tomorrow

Dave

Comment 20

[David Lawrence [:dkl]]

Oops. Patch needs to be updated now due to the creator patch committed already.

[dkl@localhost trunk]$ patch -p0 < ~/Downloads/bug-update.diff 
patching file Bugzilla/Bug.pm
Hunk #1 FAILED at 274.
1 out of 1 hunk FAILED -- saving rejects to file Bugzilla/Bug.pm.rej
patching file Bugzilla/WebService/Bug.pm
Hunk #1 succeeded at 47 (offset 1 line).
Hunk #2 succeeded at 449 (offset 23 lines).
Hunk #3 succeeded at 2133 (offset 51 lines).
patching file Bugzilla/WebService/Constants.pm

I will fix in my checkout and continue my review.

Dave

Comment 21

[David Lawrence [:dkl]]

Comment on attachment 452381 [details] [diff] [review]
v2

Bug.update needs the following added to allow certain fields to be updated properly:

$params = Bugzilla::Bug::map_fields($params);

After adding that myself to continue testing, I have had mostly success in updating all of the fields as outlined in the POD documentation. 

With one exception. Updating 'summary' does not work as you have 'summary' mapping to 'short_desc' in Bugzilla::Bug::FIELD_MAP which is incorrect. There is not a set_short_desc() function in Bugzilla::Bug, so passing in value for 'summary' to Bug.update has no effect. If I comment out the 'summary' line in Bugzilla::Bug::FIELD_MAP all works as expected.

Dave

Comment 22

[Max Kanat-Alexander]

Attachment: v3

Ah, thanks for catching that!

There is still one FIXME in the POD, but I actually plan to leave that there until after we do QA, which will be the most effective way of actually determining all the errors that can be thrown.

Comment 23

[David Lawrence [:dkl]]

Comment on attachment 456371 [details] [diff] [review]
v3

Looks good and works as expected. r=dkl

Comment 24

[Max Kanat-Alexander]

  Woo hoo!! Thanks, dkl! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Bug.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 7335.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Bug.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 7310.

Comment 25

[Max Kanat-Alexander]

Added to the release notes in bug 604256.