440615 - Remove ValidateBugID from Bugzilla::Bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

In bug 440612, I reduced usage of ValidateBugID down to just Bugzilla::Bug->check. Now we can remove it entirely and merge the two methods.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

This patch depends on the patch from bug 440612.

This also fixes two long-standing but minor bugs:

1) Previous to this patch, an alias that started with the "#" character wouldn't be accessible, because of how ValidateBugID works.
2) Bugzilla::Bug->new should have been returning InvalidBugId as the error sometimes instead of NotFound.

This also now allows you to specify bug ids to Bugzilla::Bug->new like "#2323" and have them work.

It also should be much more efficient, since we've eliminated two SQL calls previously done by ValidateBugID.

Comment 2

[Frédéric Buclin]

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

> sub check {

>+    if ($self->{error}) {
>+        if ($self->{error} eq 'NotFound') {
>+             ThrowUserError("bug_id_does_not_exist", { bug_id => $id });
>+        }
>+        if ($self->{error} eq 'InvalidBugId') {
>+            ThrowUserError("improper_bug_id_field_value",
>+                              { bug_id => $id,
>+                                field  => $field });
>+        }
>+    }

This looks good. One thing to fix though: Bugzilla::Bug->new() removes the leading # if present, but check() ignores this and passes the original $id (with "#" in it) to the error message. You should pass $self directly as it contains the cleaned up value.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Okay, here's a new patch with that fixed.

Comment 4

[Frédéric Buclin]

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

There are still some problems with your patch:
- your patch doesn't pass runtests.pl 12:

t/012throwables......NOK 16
#   Failed test 'Bugzilla/Bug.pm has 2 error(s):
# code error tag 'NotFound' is used at line(s) (275) but not defined for language(s): any
# code error tag 'InvalidBugId' is used at line(s) (272) but not defined for language(s): any'

IMO, that's because t/012throwables.t line 119 has:

        if ($line =~
/^[^#]*(Throw(Code|User)Error|error\s+=>)\s*\(?\s*["'](.*?)['"]/) {

This line was edited in bug 337701 to take errors from AUTH_ERROR into account, but this should not be taken into account here. This needs to be fixed, else all tinderboxes are going to burn.



- It treats show_bug.cgi?id=%235f (%23 means #) as show_bug.cgi?id=5 despite it should read #5f or 5f (which could be seen as an alias). Not sure we want to care about this point, though. I think that's because the regexp below is not 100% correct:

>+    # Remove leading "#" mark if we've just been passed an id.
>+    if (!ref $param && $param =~ /^\s*#(\d+)\s*/) {
>+        $param = $1;

The second \s* looks wrong to me. It should rather be \b I think, so that it doesn't split #5f.



- And finally, the bug ID displayed in these error messages should be modified:

>+        if ($user->id) {
>+            ThrowUserError("bug_access_denied", { bug_id => $id });
>+        } else {
>+            ThrowUserError("bug_access_query", { bug_id => $id });
>+        }

I think you should use the bug ID returned by $self->id instead of the unvalidated $id passed by the user. Not sure what should be displayed if the user passed an alias instead of a bug ID. But this method is one of the most important ones in Bugzilla, so we should be very precise about how it works.

Sorry for not catching that before.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> I think you should use the bug ID returned by $self->id instead of the
> unvalidated $id passed by the user. Not sure what should be displayed if the
> user passed an alias instead of a bug ID.

I just checked how ValidateBugID() works, and it always displays the validated bug ID in error messages, i.e. it converts aliases to IDs first. So we should do the same and use $self->id in all cases, as I suggested in my previous review comment.

Comment 6

[Max Kanat-Alexander]

Attachment: v3

Okay, fixed everything. The problem with the regex was that it was missing a $.

Comment 7

[Frédéric Buclin]

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

>+    # Remove leading "#" mark if we've just been passed an id.
>+    if (!ref $param && $param =~ /^\s*#(\d+)\s*$/) {

Please add |$param = trim($param) unless ref $param;| before this test and remove both \s* from the regexp. This way we are consistent with how ValidateBugID() works in 3.2 and older.

Otherwise it looks good and seems to work fine. r=LpSolit

Comment 8

[Max Kanat-Alexander]

On checkin, I put trim() in check, and I removed the \s* from the regex in new().

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.250; previous revision: 1.249
done