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

Moved quantity calculation to its own method calculateTotalQuantity() #34

Merged
merged 4 commits into from
May 15, 2018

Conversation

shineability
Copy link
Contributor

  • Moved quantity calculation to its own method calculateTotalQuantity()
  • CS fixes

@sprankhub I removed the underscores from the protected functions (PSR-2), hope this is ok for you? Personally, I hate them, but I know it's still common practice in M1... Feel free to add them again :)

Maarten Troonbeeckx added 2 commits March 29, 2018 10:54
CS
Signed-off-by: Maarten Troonbeeckx <maarten@uni-t.be>
Signed-off-by: Maarten Troonbeeckx <maarten@uni-t.be>
@shineability
Copy link
Contributor Author

@sprankhub Failed checks don't seem to be related to the actual tests. Can you have a look?

@sprankhub
Copy link
Member

Sorry that it takes so long, but since this is a critical change and involves background processes (new release, update on Marketplace etc.), we need some more time. I definitely have it on my list and will check this PR as soon as possible.

Copy link
Member

@sprankhub sprankhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late feedback. I had a quick look at it now and have to request a few changes. We will process the PR further once they are corrected. Thanks!

const XML_PATH_IS_ENABLED = 'sales/customgento_configurabletierprices/is_enabled';
const XML_PATH_DISABLED_FOR_CATEGORY = 'sales/customgento_configurabletierprices/disabled_for_category';

// attribute code length must be less than 30 symbols!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this comment (as it is non-intuitive to use an abbreviated version of the extension name here).

@@ -41,15 +39,15 @@ public function isProductInDisabledCategory(Mage_Catalog_Model_Product $product)

public function isExtensionDisabledForProduct(Mage_Catalog_Model_Product $product)
{
// get the product attribute directly, because it may not be loaded
// Get the product attribute directly, because it may not be loaded
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not change such subtle things in a PR. It makes it harder to merge (and I prefer starting with lower case letters btw).

/**
* @var CustomGento_ConfigurableTierPrices_Helper_Data
*/
protected $helper;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to $_helper.

*
* @param Mage_Catalog_Model_Product $product
*
* @return float
*/
protected function _calcConfigProductTierPricing($product)
protected function calculateTierPrice($product)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to _calculateTierPrice.

*
* @return int
*/
protected function calculateTotalQuantity($product)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to _calculateTotalQuantity.

}

/**
* Retrieves all visible quote items from the session.
*
* @return array with instances of Mage_Sales_Model_Quote_Item
*/
protected function _getAllVisibleItems()
protected function getAllVisibleItems()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert to _getAllVisibleItems.

@shineability
Copy link
Contributor Author

@sprankhub Tnx for the feedback. Should be fixed now. Also reverted the inline comments in the Observer to start with a lower case.

@sprankhub
Copy link
Member

Thanks for the quick update! My colleague @norgeindian will further test and process this PR.

@norgeindian norgeindian merged commit 34c92fb into customgento:master May 15, 2018
@shineability
Copy link
Contributor Author

@norgeindian Tnx!

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

Successfully merging this pull request may close these issues.

None yet

3 participants