Forum: Ruby on Rails Would someone like to tell me why this code will not solve m

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
Bruce B. (Guest)
on 2006-01-03 06:12
(Received via mailing list)
I am building an invoicing system but cannot use the auto_increment
field to determine the invoice number (because they are running 3
different companies off the one system.  I need to find the last
invoice number from any given company and then add 1 to it to get the
next invoice number.  BUT, there is a unique case on the very first
invoice produced because there is no earlier invoice number.  My
solution is as follows (and does not work)

This is in the 'create' controller


     if Invoice.find(:all)
       @temp = Invoice.find_by_sql("select max(inv_number) as inv_number
                               from invoices where company_id = params
[:company_id")
       @invoice.inv_number = @temp[0].inv_number +1
     else
       @invoice.inv_number=1
     end



The error I am getting is

NoMethodError in Invoices#create

You have a nil object when you didn't expect it!
You might have expected an instance of Array.
The error occured while evaluating nil.+
Kevin O. (Guest)
on 2006-01-03 06:25
bruce balmer wrote:
>
> if Invoice.find(:all)
> @temp = Invoice.find_by_sql("select max(inv_number) as inv_number
>                            from invoices where company_id
>                            = params[:company_id")

This line might be part of the problem.  the "params[:company_id]" is
being inserted literally into your sql string.  You need to do something
like

... = #{params[:company_id]}" to insert it.

WARNING... this is highly unsafe.  A user could manually pass all sorts
of badness through the company_id param and totally hose you.  Please
find some other (secure) way to do this.  The ['company_id = ?',
params[:company_id]] approach is better, but I'm not sure how to do this
with the find_by_sql function.

By the way,  you could do something like..

Invoice.find(:first, :condition=>["company_id = ?",
params[:company_id]], :order=>"inv_number DESC")

This should give you the record with the highest invoice number.

_Kevin
Marcel Molina Jr. (Guest)
on 2006-01-03 06:48
(Received via mailing list)
On Mon, Jan 02, 2006 at 09:10:47PM -0700, Bruce B. wrote:
>    This is in the 'create' controller

Invoice.find(:all) will always be boolean true. If there are no
Invoices,
then find(:all) will return an empty array ([]), which is always boolean
true.

A way of expressing your intentions is:

  unless Invoice.count.zero?

>        if Invoice.find(:all)

  params[:company_id] is not being interpolated. It's being passed along
as a
literal value. String interpolation in Ruby is done with #{}. For
example:

  foo = 'Marcel'
  "My name is #{foo}"
  => "My name is Marcel"

The following find_by_sql, makes you vulnerable to SQL injection. Rather
than
using find_by_sql, you could use find, passing the :select and the
:conditions options. Something like:

  Invoice.find(:first, :select => 'MAX(inv_number) as inv_number',
:conditions => ['company_id = ?', params[:company_id]).inv_number.to_i

But there is no need to do this MAX business. You can instead use
ActiveRecord's increment or increment! or increment_counter method:

http://api.rubyonrails.com/classes/ActiveRecord/Ba...
http://api.rubyonrails.com/classes/ActiveRecord/Ba...

Increment allows you to avoid the entire conditional.

>          @temp = Invoice.find_by_sql("select max(inv_number) as inv_number
>                                  from invoices where company_id =
>    params[:company_id")
>          @invoice.inv_number = @temp[0].inv_number +1
>        else
>          @invoice.inv_number=1
>        end

marcel
Jarkko L. (Guest)
on 2006-01-03 11:27
(Received via mailing list)
On 3.1.2006, at 6.55, Marcel Molina Jr. wrote:
>
> But there is no need to do this MAX business. You can instead use
> ActiveRecord's increment or increment! or increment_counter method:
>
> http://api.rubyonrails.com/classes/ActiveRecord/Ba...
> http://api.rubyonrails.com/classes/ActiveRecord/Ba...
>
> Increment allows you to avoid the entire conditional.

Also keep in mind that selecting max() from a table is NOT a
transaction safe method. If you have two users inserting an invoice
concurrently, they might end up with the same numbe. That's why many
database management systems have sequences [1].

//jarkko

[1] http://www.postgresql.org/docs/8.1/interactive/sql-
createsequence.html
Bruce B. (Guest)
on 2006-01-04 05:16
(Received via mailing list)
Marcel:

Thanks very much.  That was awesome!

bruce
This topic is locked and can not be replied to.