686971 - Leaving the See Also field empty or undefined while adding one using the Bug.update WebServices method no longer triggers 'that argument was not set'

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Frédéric Buclin]

webservice_bug_update.t expects to get '... input argument, and that argument was not set' as error message when leaving the See Also field empty or undefined while trying to add one to a bug using Bug.update. But in 4.2, we get 'is not a valid URL to a bug. See Also URLs should point to one of: ...'

Is this change expected? Or is it a failure in the validator to make sure that the value is set? I would expect that check_required_create_fields() would catch this.

Comment 1

[Tiago Mello [:timello]]

That because we don't have a validator (ie.: _check_see_also) for see_also in Bug.pm... and Bugzilla::BugUrl->class_for is where it takes care of the validation. I think the point is more architectural than a problem. If we allow adding see also in Bug.update so I think we should add the validator and expect the same behavior of the others fields.

Comment 2

[Tiago Mello [:timello]]

@mkanat │ LpSolit: Hmmm. Most empty fields return info that they are an empty field. 
@mkanat │ LpSolit: I would think that the see also validation internally should perhaps do that.
@mkanat │ Unless it complicates the architecture.

Comment 3

[Max Kanat-Alexander]

Oh, you know, it does say "See Also" in the error message. I think that's enough. We can fix the test to expect the new error message, as long as the error code hasn't changed.

Comment 4

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #3)
> Oh, you know, it does say "See Also" in the error message. I think that's
> enough. We can fix the test to expect the new error message

I'm fine with fixing the tests, as long as someone can confirm that the current code works as expected, and that the new error message is indeed correct. If it appears that the error message is different because something is broken internally, then this is a blocker. Else this bug no longer blocks. mkanat/timello?

Comment 5

[Tiago Mello [:timello]]

Attachment: v1

mkanat, as per your previous comment on IRC:

"@mkanat timello: I think ignoring empty values will probably make life easier for implementors."

So, here is that patch.

Comment 6

[Frédéric Buclin]

Comment on attachment 577625 [details] [diff] [review]
v1

>=== modified file 'Bugzilla/Bug.pm'

>     $input = trim($input);
> 
>+    return if !$input;

Nit: the whitespace between these two lines is useless.

r=LpSolit. I will fix Selenium scripts accordingly. Note that this change should be relnoted as this is an API change. We no longer throw an error but silently ignore such empty URLs.

Comment 7

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/                              
modified Bugzilla/Bug.pm
Committed revision 7967.


Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                            
modified Bugzilla/Bug.pm
Committed revision 8019.

Comment 8

[Frédéric Buclin]

Added to relnotes in bug 713346.