Last Comment Bug 415813 - (bz-ws-update) Implement Bug.update() as an API for WebServices
: Implement Bug.update() as an API for WebServices
[wanted-bmo][roadmap: 3.8]
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: 3.1.3
: All All
: P1 enhancement with 1 vote (vote)
: Bugzilla 4.0
Assigned To: Max Kanat-Alexander
: default-qa
: 621025 (view as bug list)
Depends on: bz-process_bug 413215 bz-bug-set-all 573170 573172 573173
Blocks: 432910 366296
  Show dependency treegraph
Reported: 2008-02-05 13:06 PST by Frédéric Buclin
Modified: 2011-09-23 15:41 PDT (History)
11 users (show)
mkanat: approval+
mkanat: approval4.0+
mkanat: blocking4.0+
mkanat: blocking3.2-
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

Work In Progress (1.60 KB, patch)
2010-05-24 16:58 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v1 (21.49 KB, patch)
2010-05-24 20:22 PDT, Max Kanat-Alexander
no flags Details | Diff | Splinter Review
v2 (15.32 KB, patch)
2010-06-18 16:41 PDT, Max Kanat-Alexander
dkl: review-
Details | Diff | Splinter Review
v3 (15.89 KB, patch)
2010-07-07 19:19 PDT, Max Kanat-Alexander
dkl: review+
Details | Diff | Splinter Review

Description Frédéric Buclin 2008-02-05 13:06:06 PST
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 2008-02-05 13:21:53 PST
What about mass updates? Meaning: multiple bugs in one call.
Comment 2 Frédéric Buclin 2008-02-05 13:42:41 PST
(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 2008-02-05 13:48:56 PST
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 2008-02-05 13:53:51 PST
(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 2008-02-05 14:43:33 PST
(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] (use needinfo?) 2008-02-05 16:31:00 PST
We want this for bmo eventually (sooner rather than later).
Comment 7 Max Kanat-Alexander 2008-02-05 16:32:52 PST
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 2008-02-18 20:45:32 PST
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
  - if it is a mass bug change, the following field changes can not be  
    "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 
  - 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?

Comment 9 Max Kanat-Alexander 2008-02-18 21:17:48 PST
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 2008-02-18 22:24:52 PST
(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 2010-05-24 16:58:24 PDT
Created attachment 447239 [details] [diff] [review]
Work In Progress
Comment 12 Max Kanat-Alexander 2010-05-24 20:22:57 PDT
Created attachment 447268 [details] [diff] [review]

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] 2010-06-18 10:37:02 PDT
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/
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/
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/
patching file Bugzilla/WebService/
patching file Bugzilla/WebService/
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?

Comment 14 David Lawrence [:dkl] 2010-06-18 10:54:04 PDT
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.
Comment 15 Max Kanat-Alexander 2010-06-18 16:14:46 PDT
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 2010-06-18 16:41:04 PDT
Created attachment 452381 [details] [diff] [review]

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 2010-06-28 03:22:20 PDT
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 2010-07-05 18:00:56 PDT
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] 2010-07-06 18:38:59 PDT
(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

Comment 20 David Lawrence [:dkl] 2010-07-07 12:50:02 PDT
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/
Hunk #1 FAILED at 274.
1 out of 1 hunk FAILED -- saving rejects to file Bugzilla/
patching file Bugzilla/WebService/
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/

I will fix in my checkout and continue my review.

Comment 21 David Lawrence [:dkl] 2010-07-07 14:31:45 PDT
Comment on attachment 452381 [details] [diff] [review]

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.

Comment 22 Max Kanat-Alexander 2010-07-07 19:19:55 PDT
Created attachment 456371 [details] [diff] [review]

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] 2010-07-11 12:12:52 PDT
Comment on attachment 456371 [details] [diff] [review]

Looks good and works as expected. r=dkl
Comment 24 Max Kanat-Alexander 2010-07-12 18:42:33 PDT
  Woo hoo!! Thanks, dkl! :-)

Committing to: bzr+ssh://                       
modified Bugzilla/
modified Bugzilla/WebService/
modified Bugzilla/WebService/
Committed revision 7335.

Committing to: bzr+ssh://
modified Bugzilla/
modified Bugzilla/WebService/
modified Bugzilla/WebService/
Committed revision 7310.
Comment 25 Max Kanat-Alexander 2010-10-21 19:40:11 PDT
Added to the release notes in bug 604256.
Comment 26 Frédéric Buclin 2010-12-22 12:36:19 PST
*** Bug 621025 has been marked as a duplicate of this bug. ***

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