Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrote Mage_Reports_Model_Resource_Review_Product_Collection/Mage_Reports_Model_Resource_Order_Collection queries for a correct use of Zend_Db_Expr #2864

Merged
merged 3 commits into from
Dec 29, 2022

Conversation

fballiano
Copy link
Contributor

This PR fixes #2237 which is triggered because of #2857 (comment)

'app/code/core/Mage/Reports/Model/Resource/Review/Product/Collection.php' wasn't written super well, when 'sprintf' was applied on Zend_Db_Expr objects (returned by '$helper->prepareColumn) you're actually removing the Zend_Db_Expr` and converting it to a normal string (which is wrong).

@github-actions github-actions bot added the Component: Reports Relates to Mage_Reports label Dec 29, 2022
addison74
addison74 previously approved these changes Dec 29, 2022
@addison74
Copy link
Contributor

By the way, this is the only file in which I found the $helper->prepareColumn() method used.

Regarding the usage of the sprintf function, changed in this PR

 'avg_rating' => sprintf('%s/%s', $sumPercentField, $countRatingId)

I found references in a few files.

@fballiano
Copy link
Contributor Author

I found something usable (since there are hundreds of occurrencies it's impossible to manually check them all) in the tax reports (reports are where it's more common to aggregate data) and tax calculations, but I couldn't trigger any error.

@elidrissidev
Copy link
Member

I found 2 other occurrences in the Reports module if you wanna include them in this PR:

'profit' => "SUM($baseSubtotalInvoiced) "
. "+ SUM({$baseDiscountRefunded}) - SUM({$baseSubtotalRefunded}) "
. "- SUM({$baseDiscountInvoiced}) - SUM({$baseTotalInvocedCost})"
]);
} else {
$this->getSelect()->columns([
'subtotal' => 'SUM(main_table.base_subtotal * main_table.base_to_global_rate)',
'tax' => 'SUM(main_table.base_tax_amount * main_table.base_to_global_rate)',
'shipping' => 'SUM(main_table.base_shipping_amount * main_table.base_to_global_rate)',
'discount' => 'SUM(main_table.base_discount_amount * main_table.base_to_global_rate)',
'total' => 'SUM(main_table.base_grand_total * main_table.base_to_global_rate)',
'invoiced' => 'SUM(main_table.base_total_paid * main_table.base_to_global_rate)',
'refunded' => 'SUM(main_table.base_total_refunded * main_table.base_to_global_rate)',
'profit' => "SUM({$baseSubtotalInvoiced} * main_table.base_to_global_rate) "
. "+ SUM({$baseDiscountRefunded} * main_table.base_to_global_rate) "
. "- SUM({$baseSubtotalRefunded} * main_table.base_to_global_rate) "
. "- SUM({$baseDiscountInvoiced} * main_table.base_to_global_rate) "
. "- SUM({$baseTotalInvocedCost} * main_table.base_to_global_rate)"

@github-actions github-actions bot added the Mage.php Relates to app/Mage.php label Dec 29, 2022
@fballiano
Copy link
Contributor Author

@elidrissidev thanks, I pushed a fix for that class too. But I couldn't trigger the error (or that collection) in the "orders" report

@fballiano fballiano changed the title Rewrote Mage_Reports_Model_Resource_Review_Product_Collection query for a correct use of Zend_Db_Expr Rewrote Mage_Reports_Model_Resource_Review_Product_Collection/Mage_Reports_Model_Resource_Order_Collection queries for a correct use of Zend_Db_Expr Dec 29, 2022
app/Mage.php Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Mage.php Relates to app/Mage.php label Dec 29, 2022
@elidrissidev
Copy link
Member

But I couldn't trigger the error (or that collection) in the "orders" report

Same, I couldn't see where that method is called. But after trying that expression in a script it indicates the issue should be happening in that part too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Reports Relates to Mage_Reports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reports - Products Reviews Error Page in OM 20.14
4 participants