Closed Bug 104677 Opened 22 years ago Closed 22 years ago

vote field for products which allow multiple votes defaults to size="5" and doesnt include a maxlength="" attribute

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

Other
Other
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: caillon, Assigned: caillon)

Details

Attachments

(1 file, 2 obsolete files)

Summary says it all.  I have a patch.
Fwiw, this is applied on http://landfill.tequilarista.org/bz34488/

Review please :)
Status: NEW → ASSIGNED
Keywords: patch, review
Severity: normal → minor
Target Milestone: --- → Bugzilla 2.16
Attachment #53456 - Flags: review-
I suggest we do the following to the patch:

That min stuff should use a int-min routine.  If we don't have one in Perl, add
one to globals.pl.

The log stuff should either be changed to use standard Perl facilities (I think
conciseness is more important than performance here), or made reusable and added
to globals.pl.
Keywords: patch, review
caillon: this patch is needs-work - are you going to fix it? :-)

Gerv
Attached patch Proposed patch v2.0 (obsolete) — Splinter Review
Yeah I had this done over the weekend and couldn't get to diff it since cvs was
down.  Forgot about it.  Here's the new patch.
Attachment #53456 - Attachment is obsolete: true
Open for review again.

Added the min/max routine like Matty suggested.  Also, log10 is not a built-in
function, so i made my own quasi log10 function.  It's really a log10 + 1
function, but since it returns the digits, I just called it digits.

And for kicks, I applied this patch on http://landfill.tequilarista.org/bz104677/
Keywords: patch, review
Comment on attachment 57698 [details] [diff] [review]
Proposed patch v2.0

Is this patch really worth doing? What _bug_ does it fix? Is this really a
problem?

>+sub digits {
>+    my $number = $_[0];
>+    my $maxlength = 0;
>+    for ($maxlength; $number > 1; $maxlength++) {
>+        $number = int($number / 10);
>+    }
>+    return $maxlength;
>+}

Better done by taking the length of the variable - this converts it to a
string. Do you even need a subroutine?

Gerv
Attachment #57698 - Flags: review-
Attached patch Patch v3Splinter Review
Gerv, it is not a huge bug but it is one nonetheless.  We shouldn't take more
user input than we can use.  Currently, on products which allow multiple votes,
a user can enter an arbitrarily large number.  For example, if we only allow 5
votes per bug, the user is able to still enter (ridiculous example:)
35108356468435409840684638445140684351498946510684384973 into the field. 
Regardless of what sanity checks on the input we have, it is still sloppy to
allow it to be entered in the first place.

We know the maximum number of votes a product can have, and thus should allow
only as many characters as there are digits.  if it's only allowed 1 vote, it's
a checkbox, and this doesn't come into play.

Should have used length to begin with.	Wasn't thinking.  This updated patch
should be ready if that was the only problem with it.
Attachment #57698 - Attachment is obsolete: true
Comment on attachment 57720 [details] [diff] [review]
Patch v3

r=gerv.

Gerv
Attachment #57720 - Flags: review+
Yes you was.  You said length "wasn't cool". =)
yeah well, it's not ;-)  why use built in functions when you can make your own?  ;-P

heh i meant to switch it but forgot.
Attachment #57720 - Flags: review+
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.129; previous revision: 1.128
done
Checking in showvotes.cgi;
/cvsroot/mozilla/webtools/bugzilla/showvotes.cgi,v  <--  showvotes.cgi
new revision: 1.10; previous revision: 1.9
done
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.