support part_pkg option input validation, check bytecounts and allow commas (closes 1863)

5 files changed, 44 insertions, 6 deletions

diff --git a/FS/FS/UI/bytecount.pm b/FS/FS/UI/bytecount.pm
index 38aa1dfd6..d278dbecc 100644
--- a/FS/FS/UI/bytecount.pm
+++ b/FS/FS/UI/bytecount.pm
@@ -42,20 +42,21 @@ sub bytecount_unexact {
 
 Accepts a number (digits and a decimal point) possibly followed by k, m, g, or
 t (and an optional 'b') in either case.  Returns a pure number representing
-the input or the input itself if unparsable.
+the input or the input itself if unparsable.  Discards commas as noise.
 
 =cut
 
 sub parse_bytecount {
   my $bc = shift;
   return $bc if (($bc =~ tr/.//) > 1);
-  $bc =~ /^\s*([\d.]*)\s*([kKmMgGtT]?)[bB]?\s*$/ or return $bc;
+  $bc =~ /^\s*([,\d.]*)\s*([kKmMgGtT]?)[bB]?\s*$/ or return $bc;
   my $base = $1;
+  $base =~ tr/,//d;
   return $bc unless length $base;
   my $exponent = index ' kmgt', lc($2);
   return $bc if ($exponent < 0 && $2);
   $exponent = 0 if ($exponent < 0);
-  return $base * 1024 ** $exponent;
+  return int($base * 1024 ** $exponent);  #bytecounts are integer values
 }
 
 =item display_bytecount AMOUNT
diff --git a/FS/FS/part_pkg/flat.pm b/FS/FS/part_pkg/flat.pm
index 471f216ba..f5ccd0119 100644
--- a/FS/FS/part_pkg/flat.pm
+++ b/FS/FS/part_pkg/flat.pm
@@ -26,40 +26,49 @@ use FS::part_pkg;
                     },
     'seconds'       => { 'name' => 'Time limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                        },
     'upbytes'       => { 'name' => 'Upload limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                          'format' => \&FS::UI::bytecount::display_bytecount,
                          'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'downbytes'     => { 'name' => 'Download limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                          'format' => \&FS::UI::bytecount::display_bytecount,
                          'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'totalbytes'    => { 'name' => 'Transfer limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                          'format' => \&FS::UI::bytecount::display_bytecount,
                          'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_amount'       => { 'name' => 'Cost of recharge for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*(\.\d{2})?$/ },
                        },
     'recharge_seconds'      => { 'name' => 'Recharge time for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                        },
     'recharge_upbytes'      => { 'name' => 'Recharge upload for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                          'format' => \&FS::UI::bytecount::display_bytecount,
                          'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_downbytes'    => { 'name' => 'Recharge download for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                          'format' => \&FS::UI::bytecount::display_bytecount,
                          'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_totalbytes'   => { 'name' => 'Recharge transfer for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                          'format' => \&FS::UI::bytecount::display_bytecount,
                          'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
diff --git a/FS/FS/part_pkg/prorate.pm b/FS/FS/part_pkg/prorate.pm
index 6b7c61532..967b1eb18 100644
--- a/FS/FS/part_pkg/prorate.pm
+++ b/FS/FS/part_pkg/prorate.pm
@@ -26,38 +26,47 @@ use FS::part_pkg::flat;
 					    },
     'seconds'       => { 'name' => 'Time limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                        },
     'upbytes'       => { 'name' => 'Upload limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'downbytes'     => { 'name' => 'Download limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'totalbytes'    => { 'name' => 'Transfer limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_amount'       => { 'name' => 'Cost of recharge for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*(\.\d{2})?$/ },
                        },
     'recharge_seconds'      => { 'name' => 'Recharge time for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                        },
     'recharge_upbytes'      => { 'name' => 'Recharge upload for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_downbytes'    => { 'name' => 'Recharge download for this package',                         'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_totalbytes'   => { 'name' => 'Recharge transfer for this package',                         'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
diff --git a/FS/FS/part_pkg/subscription.pm b/FS/FS/part_pkg/subscription.pm
index 7f4dbcab9..3e5a41669 100644
--- a/FS/FS/part_pkg/subscription.pm
+++ b/FS/FS/part_pkg/subscription.pm
@@ -22,38 +22,47 @@ use FS::part_pkg::flat;
                     },
     'seconds'       => { 'name' => 'Time limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                        },
     'upbytes'       => { 'name' => 'Upload limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'downbytes'     => { 'name' => 'Download limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'totalbytes'    => { 'name' => 'Transfer limit for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_amount'       => { 'name' => 'Cost of recharge for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*(\.\d{2})?$/ },
                        },
     'recharge_seconds'      => { 'name' => 'Recharge time for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
                        },
     'recharge_upbytes'      => { 'name' => 'Recharge upload for this package',
                          'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_downbytes'    => { 'name' => 'Recharge download for this package',                         'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
     'recharge_totalbytes'   => { 'name' => 'Recharge transfer for this package',                         'default' => '',
+                         'check' => sub { shift =~ /^\d*$/ },
 			 'format' => \&FS::UI::bytecount::display_bytecount,
 			 'parse' => \&FS::UI::bytecount::parse_bytecount,
                        },
diff --git a/httemplate/edit/process/part_pkg.cgi b/httemplate/edit/process/part_pkg.cgi
index 5fc59c14d..fd7b17567 100755
--- a/httemplate/edit/process/part_pkg.cgi
+++ b/httemplate/edit/process/part_pkg.cgi
@@ -10,12 +10,19 @@
 %my $href = $plans{$cgi->param('plan')}->{'fields'};
 %
 %#fixup plandata
+%my $error;
 %my $plandata = $cgi->param('plandata');
 %my @plandata = split(',', $plandata);
 %$cgi->param('plandata', 
 %  join('', map { my $parser = sub { shift };
 %                 $parser = $href->{$_}{parse} if exists($href->{$_}{parse});
-%                 "$_=". join(', ', &$parser($cgi->param($_))). "\n"
+%                 my $value = join(', ', &$parser($cgi->param($_)));
+%                 my $check = $href->{$_}{check};
+%                 if ( $check && ! &$check($value) ) {
+%                   $value = join(', ', $cgi->param($_));
+%                   $error ||= "Illegal ". ($href->{$_}{name}||$_). ": $value";
+%                 }
+%                 "$_=$value\n";
 %               } @plandata )
 %);
 %
@@ -39,9 +46,12 @@
 %              map { $_->svcpart }
 %              qsearch('part_svc', {} );
 %
-%my $error;
 %my $custnum = '';
-%if ( $cgi->param('taxclass') eq '(select)' ) {
+%if ( $error ) {
+%
+% # fall through
+%
+%} elsif ( $cgi->param('taxclass') eq '(select)' ) {
 %
 %  $error = 'Must select a tax class';
 %