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

Implement "bought together products" #7

Merged
merged 24 commits into from
Jan 30, 2024
Merged

Implement "bought together products" #7

merged 24 commits into from
Jan 30, 2024

Conversation

coldic3
Copy link
Member

@coldic3 coldic3 commented May 14, 2023

  • src code
  • config
  • handle migrations (fix The class 'Tests\CommerceWeavers\SyliusAlsoBoughtPlugin\Application\Entity\Product' was not found in the chain configured namespaces)
  • move tests
  • introduce behat tests
  • make sure ProductsBoughtTogetherTest is covered with behat scenario
  • CI
  • remove sample GreetingController
  • viewing "Bought together" product associations, Behat tests @api and @ui
  • prevent from removing "bought_together" association type, or inform admin that the association type is missing

@coldic3 coldic3 changed the title Implement "brought together products" [WIP] Implement "brought together products" May 14, 2023
@coldic3 coldic3 changed the title [WIP] Implement "brought together products" Implement "brought together products" Jul 7, 2023
@coldic3 coldic3 changed the title Implement "brought together products" [WIP] Implement "brought together products" Jul 7, 2023
@coldic3 coldic3 changed the title [WIP] Implement "brought together products" Implement "brought together products" Nov 30, 2023
@coldic3 coldic3 changed the title Implement "brought together products" Implement "bought together products" Dec 4, 2023
<argument type="service" id="doctrine.orm.entity_manager" />
<argument type="service" id="sylius.repository.product" />
<argument type="service" id="CommerceWeavers\SyliusAlsoBoughtPlugin\Provider\BoughtTogetherProductsAssociationProviderInterface" />
<argument>10</argument>
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to parametrize it.

src/Sylius/ChannelBundle/Collector/ChannelCollector.php Outdated Show resolved Hide resolved
features/synchronizing_bought_together_products.feature Outdated Show resolved Hide resolved
features/synchronizing_bought_together_products.feature Outdated Show resolved Hide resolved
migrations/Version20230630102308.php Show resolved Hide resolved
src/Mapper/BoughtTogetherProductsMapper.php Outdated Show resolved Hide resolved
src/Mapper/BoughtTogetherProductsMapper.php Outdated Show resolved Hide resolved
Comment on lines 16 to 17
foreach ($order->getItems() as $item) {
$productCode = $item->getProduct()?->getCode();
Copy link
Member

Choose a reason for hiding this comment

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

Since items are created from variants, wouldn't we potentially have a lot of wasteful processing cause $productCode can be inserted multiple times within $productCodes, but the foreach below will simply overwrite the key on each run w/ $map[$productCode] = array_diff($productCodes, [$productCode])?

If the duplicates are needed for the value it might be worth making another array with unique codes and iterate over that instead.

@@ -0,0 +1,19 @@
Copyright (c) 2023-2077 Commerce Weavers
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, what's the legality here?

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
Member

Choose a reason for hiding this comment

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

💃

);
}

public function testItProvidesEarliestPossibleDateIfThereIsNoSynchronizationLog(): void
Copy link
Member

Choose a reason for hiding this comment

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

That's just wrong, it returns current not the earliest possible

Comment on lines +29 to +31
This plugin was sponsored by [Mofakult](https://www.mofakult.ch)
and developed in cooperation with [Fusonic](https://www.fusonic.net).
Thank you for your contribution to the open-source ecosystem!
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to put Mofakult and Fusonic logos somewhere here 💃

<service id="CommerceWeavers\SyliusAlsoBoughtPlugin\Cli\CreateBoughtTogetherProductAssociationTypeCommand">
<argument type="service" id="Symfony\Component\Messenger\MessageBusInterface" />
<argument type="service" id="sylius.repository.product_association_type" />
<tag name="console.command" />
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, that maybe we can move forward with autowiring and autoconfiguration for our plugins 🤔 but could be done separately

And the store has a product association type "Bought together" with a code "bought_together"
When I create a bought together product association type by running command
Then I should be informed that the bought together product association type already exists

Copy link
Member

Choose a reason for hiding this comment

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

Extra blank line 🚀

@cli
Scenario: Running setup bought together command when there is no bought together product association type
Given the store operates on a single channel in "United States"
When I create a bought together product association type by running command
Copy link
Member

Choose a reason for hiding this comment

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

by running command is an implementation details, should be removed

Feature: Creating bought together product association type
In order to setup plugin
As a Developer
I want to run a command that will create bought together product association type
Copy link
Member

Choose a reason for hiding this comment

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

I want to create bought together product association type? To focus on business value not on the details

Comment on lines +32 to +33
'commerce_weavers_sylius_also_bought_plugin',
'doctrine/orm',
Copy link
Member

Choose a reason for hiding this comment

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

Let's extract those values to constants

Comment on lines +13 to +16
private Uuid $id,
private \DateTimeImmutable $date,
private int $numberOfOrders,
private array $affectedProducts,
Copy link
Member

Choose a reason for hiding this comment

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

Could be readonly

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it cannot be, because we're supporting still PHP 8.0 💃 We could bump to 8.1 at least


namespace CommerceWeavers\SyliusAlsoBoughtPlugin\Exception;

class BoughtTogetherAssociationTypeNotFoundException extends \RuntimeException
Copy link
Member

Choose a reason for hiding this comment

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

Could be final

Comment on lines +20 to +22
if (null === $lastSynchronization) {
return (new \DateTimeImmutable())->setTimestamp(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree 👍

Comment on lines +4 to +5


Copy link
Member

Choose a reason for hiding this comment

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

Doubled blank line 💃

@Zales0123 Zales0123 merged commit 6615d59 into CommerceWeavers:main Jan 30, 2024
24 checks passed
@Zales0123
Copy link
Member

Thank you, Kevin, amazing work! 🎉

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.

3 participants