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.

Sign up for GitHub

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

Jump to bottom

[Product] Implement Toggleable interface on Product #3342

Merged
merged 1 commit into from
Jan 4, 2016
Merged

[Product] Implement Toggleable interface on Product #3342

merged 1 commit into from
Jan 4, 2016

Conversation

CoderMaggie
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #3213
License MIT

stloyd
stloyd reviewed Sep 23, 2015
View reviewed changes
src/Sylius/Component/Product/spec/Model/ProductSpec.php
function it_is_toggleable()
{
$this->enable();
$this->isEnabled()->shouldReturn(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

$this->shouldBeEnabled();

tristanbes
tristanbes reviewed Sep 23, 2015
View reviewed changes
src/Sylius/Bundle/ProductBundle/Behat/ProductContext.php

return $product;
}
$product = $this->getRepository('product')->createNew();
Copy link
Contributor

Choose a reason for hiding this comment

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

you should add a blank line before 311

@CoderMaggie CoderMaggie force-pushed the feature/product-toggleable branch 5 times, most recently from 3a496a5 to b4d50c5 Compare September 24, 2015 09:20
@CoderMaggie CoderMaggie changed the title [WIP] [Product] [RFC] Implement Toggleable interface on Product [Product] Implement Toggleable interface on Product Sep 24, 2015
1XWP
1XWP reviewed Sep 24, 2015
View reviewed changes
src/Sylius/Bundle/ProductBundle/Behat/ProductContext.php
$product = $this->getRepository('product')->findOneBy(array('name' => $name));

$isEnabled = false;
if ('enabled' == $enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if('enabled' ===  $enabled)

Copy link
Contributor

Choose a reason for hiding this comment

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

if ('enabled' === $enabled) {

😃
Anyway, aren't triple === usefull for bool only ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Due to PHP dynamic typing they are always useful :) In this case not so much, but it's better to get used to them to avoid nasty bugs in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway:

$isEnabled = 'enabled' === $enabled;

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact not only this should be changed, the whole part is wrong. It should be:

if (null === $product) {
    $product = $this->getRepository('product')->createNew();
    $product->setName($name);
}

$product->setEnabled('enabled' === $enabled);

$manager = $this->getEntityManager();
$manager->persist($product);
$manager->flush();

return $product;

@pjedrzejewski pjedrzejewski added this to the v0.16.0 milestone Oct 21, 2015
@pjedrzejewski
Copy link
Member

@TheMadeleine Requires rebasing.

@CoderMaggie CoderMaggie force-pushed the feature/product-toggleable branch 2 times, most recently from 2e3e014 to 6dab33c Compare October 21, 2015 12:52
@CoderMaggie
Copy link
Member Author

@pjedrzejewski done 👍

stloyd
stloyd reviewed Oct 21, 2015
View reviewed changes
src/Sylius/Bundle/ProductBundle/Behat/ProductContext.php
@@ -296,6 +296,7 @@ public function theFollowingOptionTranslationsExist(TableNode $table)
}

/**
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Invalid rebase.

stloyd
stloyd reviewed Oct 21, 2015
View reviewed changes
src/Sylius/Bundle/ProductBundle/Behat/ProductContext.php
@@ -325,9 +326,30 @@ private function getPricingCalculatorConfiguration(array $data)
foreach ($calculatorConfiguration as $channelCode => $price) {
$channel = $channelRepository->findOneBy(array('code' => $channelCode));

$finalCalculatorConfiguration[$channel->getId()] = (int) round($price * 100);
$finalCalculatorConfiguration[$channel->getId()] = (int)round($price * 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be reverted.

michalmarcinkowski
michalmarcinkowski reviewed Dec 11, 2015
View reviewed changes
src/Sylius/Bundle/ProductBundle/Behat/ProductContext.php

if (null === $product) {
$product = $this->getRepository('product')->createNew();
$product->setName($name);
Copy link
Contributor

Choose a reason for hiding this comment

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

description as well as price is necessary also.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated :)

@pjedrzejewski pjedrzejewski mentioned this pull request Dec 11, 2015
Closed
@pjedrzejewski pjedrzejewski removed this from the v0.16.0 milestone Dec 14, 2015
michalmarcinkowski
michalmarcinkowski reviewed Dec 22, 2015
View reviewed changes
src/Sylius/Component/Product/spec/Model/ProductSpec.php
@@ -41,6 +41,11 @@ function it_implements_Sylius_product_interface()
$this->shouldImplement(ProductInterface::class);
}

function it_implements_toggleable_interface()
{
$this->shouldImplement('Sylius\Component\Resource\Model\ToggleableInterface');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ::class

@michalmarcinkowski
Copy link
Contributor

Migration file is missing.

michalmarcinkowski
michalmarcinkowski reviewed Jan 4, 2016
View reviewed changes
src/Sylius/Component/Product/spec/Model/ProductSpec.php
function it_is_toggleable()
{
$this->enable();
$this->shouldBeEnabled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's enabled by default I would swap the order (disable then enable again).

@michalmarcinkowski
Copy link
Contributor

The migration is wrong and rebase is needed.

@CoderMaggie
Copy link
Member Author

@michalmarcinkowski updated :)

michalmarcinkowski added a commit that referenced this pull request Jan 4, 2016
@michalmarcinkowski
Merge pull request #3342 from TheMadeleine/feature/product-toggleable
52600b3 
[Product] Implement Toggleable interface on Product
@michalmarcinkowski michalmarcinkowski merged commit 52600b3 into Sylius:master Jan 4, 2016
@michalmarcinkowski
Copy link
Contributor

Thank you Magda! 👍

@okwinza okwinza mentioned this pull request Feb 4, 2016
Closed
@CoderMaggie CoderMaggie deleted the feature/product-toggleable branch March 14, 2017 09:45
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
@michalmarcinkowski
Merge pull request Sylius#3342 from TheMadeleine/feature/product-togg…
ce1ca8b 
…leable

[Product] Implement Toggleable interface on Product
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@CoderMaggie @pjedrzejewski @michalmarcinkowski @stloyd @tristanbes @pamil @Zales0123 @1XWP