-1

I have following method in a model named CashTransaction.

def is_refundable?
    self.amount > self.total_refunded_amount
end

def total_refunded_amount
   self.refunds.sum(:amount)
end

Now I need to extract all the records which satisfy the above function i.e records which return true.

I got that working by using following statement:

CashTransaction.all.map { |x| x if x.is_refundable? }

But the result is an Array. I am looking for ActiveRecord_Relation object as I need to perform join on the result.

I feel I am missing something here as it doesn't look that difficult. Anyways, it got me stuck. Constructive suggestions would be great.

Note: Just amount is a CashTransaction column.

EDIT

Following SQL does the job. If I can change that to ORM, it will still do the job.

SELECT `cash_transactions`.* FROM `cash_transactions` INNER JOIN `refunds` ON `refunds`.`cash_transaction_id` = `cash_transactions`.`id` WHERE (cash_transactions.amount > (SELECT SUM(`amount`) FROM `refunds` WHERE refunds.cash_transaction_id = cash_transactions.id GROUP BY `cash_transaction_id`));

Sharing Progress

I managed to get it work by following ORM:

CashTransaction
  .joins(:refunds)
  .group('cash_transactions.id')
  .having('cash_transactions.amount > sum(refunds.amount)')

But what I was actually looking was something like:

CashTransaction.joins(:refunds).where(is_refundable? : true)

where is_refundable? being a model function. Initially I thought setting is_refundable? as attr_accesor would work. But I was wrong.

Just a thought, can the problem be fixed in an elegant way using Arel.

abhinavmsra
  • 666
  • 6
  • 21

4 Answers4

3

There are two options.

1) Finish, what you have started (which is extremely inefficient when it comes to bigger amount of data, since it all is taken into the memory before processing):

CashTransaction.all.map(&:is_refundable?) # is the same to what you've written, but shorter.

SO get the ids:

ids = CashTransaction.all.map(&:is_refundable?).map(&:id)

ANd now, to get ActiveRecord Relation:

CashTransaction.where(id: ids) # will return a relation

2) Move the calculation to SQL:

CashTransaction.where('amount > total_refunded_amount')

Second option is in every possible way faster and efficient.

When you deal with database, try to process it on the database level, with smallest Ruby involvement possible.

EDIT

According to edited question here is how you would achieve the desired result:

CashTransaction.joins(:refunds).where('amount > SUM(refunds.amount)')

EDIT #2

As to your updates in question - I don't really understand, why you have latched onto is_refundable? as an instance method, which could be used in query, which is basically not possible in AR, but..

My suggestion is to create a scope is_refundable:

scope :is_refundable, -> { CashTransaction
  .joins(:refunds)
  .group('cash_transactions.id')
  .having('cash_transactions.amount > sum(refunds.amount)')
}

Now it is available in as short notation as

CashTransaction.is_refundable

which is shorter and more clear than aimed

CashTransaction.where('is_refundable = ?', true)
Andrey Deineko
  • 51,333
  • 10
  • 112
  • 145
  • I edited the question. `total_refunded_amount` is not a table column. – abhinavmsra Dec 03 '15 at 07:31
  • @abhinavmsra if `total_refunded_amount` is not the actual column in the database, then you can't do it using SQL. See my answer please. – K M Rakibul Islam Dec 03 '15 at 07:32
  • @abhinavmsra so what do you expect me to do now? How can I provide you with any more info? I've shown how to do it our way. If you want to see the SQL solution, show me the `total_refunded_amount` method, may be the calculations are possible in SQL. – Andrey Deineko Dec 03 '15 at 07:34
  • @AndreyDeineko I was actually looking for sth. like `CashTransaction.where('is_refundable = ?', true)` if possible of cource – abhinavmsra Dec 03 '15 at 08:28
1

You can do it this way:

cash_transactions = CashTransaction.all.map { |x| x if x.is_refundable? } # Array
CashTransaction.where(id: cash_transactions.map(&:id)) # ActiveRecord_Relation

But, this is an in-efficient way of doing it as the other answerers also mentioned.

You can do it using SQL if amount and total_refunded_amount are the columns of the cash_transactions table in the database which will be much more efficient and performant:

CashTransaction.where('amount > total_refunded_amount')

But, if amount or total_refunded_amount are not the actual columns in the database, then you can't do it this way. Then, I guess you have do it the other way which is in-efficient than using raw SQL.

K M Rakibul Islam
  • 33,760
  • 12
  • 89
  • 110
0

I think you should pre-compute is_refundable result (in a new column) when a CashTransaction and his refunds (supposed has_many ?) are updated by using callbacks :

class CashTransaction
  before_save :update_is_refundable
  def update_is_refundable
    is_refundable = amount > total_refunded_amount
  end
  def total_refunded_amount
     self.refunds.sum(:amount)
  end
end

class Refund
  belongs_to :cash_transaction
  after_save :update_cash_transaction_is_refundable
  def update_cash_transaction_is_refundable
    cash_transaction.update_is_refundable
    cash_transaction.save!
  end
end

Note : The above code must certainly be optimized to prevent some queries

They you can query is_refundable column :

CashTransaction.where(is_refundable: true)
fabien-michel
  • 1,873
  • 19
  • 38
0

I think it's not bad to do this on two queries instead of a join table, something like this

def refundable
  where('amount < ?', total_refunded_amount)
end

This will do a single sum query then use the sum in the second query, when the tables grow larger you might find that this is faster than doing a join in the database.

Mohammad AbuShady
  • 40,884
  • 11
  • 78
  • 89