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

Create Gmv Command PoC #1

Merged
merged 16 commits into from
Jun 27, 2024
Merged

Create Gmv Command PoC #1

merged 16 commits into from
Jun 27, 2024

Conversation

mpysiak
Copy link
Member

@mpysiak mpysiak commented Jun 18, 2024

No description provided.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.lock Outdated Show resolved Hide resolved
config/services.xml Outdated Show resolved Hide resolved
src/Command/GmvCommand.php Outdated Show resolved Hide resolved
src/Provider/GmvProvider.php Show resolved Hide resolved
src/Parser/DateParser.php Outdated Show resolved Hide resolved
symfony.lock Outdated Show resolved Hide resolved
@mpysiak mpysiak force-pushed the create-gmv-command branch 4 times, most recently from 2bd7e36 to 3b276ae Compare June 19, 2024 06:53
@mpysiak mpysiak force-pushed the create-gmv-command branch 3 times, most recently from ee28d53 to 2ab6dd6 Compare June 25, 2024 10:40
@mpysiak mpysiak force-pushed the create-gmv-command branch 2 times, most recently from 51e438d to 0d37ebf Compare June 25, 2024 10:56
@mpysiak mpysiak force-pushed the create-gmv-command branch 3 times, most recently from 28fdb46 to 59d587d Compare June 25, 2024 11:13
.github/workflows/build.yaml Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
$now = new \DateTime();

return (clone $now)
->modify('first day of -12 months');
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have configurable parameters for the default start date and default end date.

src/Parser/DateParser.php Outdated Show resolved Hide resolved
src/Provider/GmvProvider.php Outdated Show resolved Hide resolved
phpunit.xml.dist Outdated Show resolved Hide resolved
<argument type="service" id="sylius_gmv.validator.input_parameters" />
<argument type="service" id="sylius_gmv.parser.date" />
<argument type="service" id="sylius_gmv.provider.gmv" />
<tag name="console.command" command="sylius:gmv:calculate" />
Copy link
Member

Choose a reason for hiding this comment

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

Is this tag needed as there is AsCommand attribute configured?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remove this tag. All private services are removed from container, and it creates problems in tests

src/Command/GmvCommand.php Outdated Show resolved Hide resolved
src/Parser/DateParser.php Outdated Show resolved Hide resolved
Comment on lines 18 to 24
public function parseStartOfMonth(string $date): \DateTime;

public function parseEndOfMonth(string $date): \DateTime;

public function getDefaultStartDate(): \DateTime;

public function getDefaultEndDate(): \DateTime;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function parseStartOfMonth(string $date): \DateTime;
public function parseEndOfMonth(string $date): \DateTime;
public function getDefaultStartDate(): \DateTime;
public function getDefaultEndDate(): \DateTime;
public function parseStartOfMonth(string $date): \DateTimeInterface;
public function parseEndOfMonth(string $date): \DateTimeInterface;
public function getDefaultStartDate(): \DateTimeInterface;
public function getDefaultEndDate(): \DateTimeInterface;

Copy link
Member Author

Choose a reason for hiding this comment

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

Using DateTimeInterface triggers php error during mocking this service

Screenshot 2024-06-26 at 09 32 38

src/Validator/InputParametersValidator.php Outdated Show resolved Hide resolved
tests/Application/config/config.yaml Show resolved Hide resolved
tests/Application/config/config.yaml Outdated Show resolved Hide resolved
tests/Functional/GmvProviderTest.php Outdated Show resolved Hide resolved
Copy link

@NoResponseMate NoResponseMate left a comment

Choose a reason for hiding this comment

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

Also, maybe add a .gitattributes file so we can minimise the weight of this package

src/Command/GmvCommand.php Outdated Show resolved Hide resolved
src/Parser/DateParser.php Outdated Show resolved Hide resolved
src/Provider/GmvProvider.php Outdated Show resolved Hide resolved
src/Provider/GmvProvider.php Outdated Show resolved Hide resolved
var/data/.gitkeep Outdated Show resolved Hide resolved
config/services.xml Outdated Show resolved Hide resolved
src/Provider/DefaultDateProvider.php Outdated Show resolved Hide resolved
src/Provider/DefaultDateProvider.php Outdated Show resolved Hide resolved
src/Provider/GmvProvider.php Outdated Show resolved Hide resolved
expensive_sw_mug_item_unit4:
__construct: [ "@expensive_sw_mug_item_2" ]

Sylius\Component\Core\Model\Adjustment:
Copy link
Member

Choose a reason for hiding this comment

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

To test both, excluded and included taxes, configure also some not neutral tax adjustments 🖖🏻

Copy link
Member

Choose a reason for hiding this comment

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

And what about promotions? They do not affect the calculation because we sum up the total items in the order, am I correct?

tests/DataFixtures/sales_with_shipping_and_taxes.yaml Outdated Show resolved Hide resolved
@mpysiak mpysiak merged commit 44a6553 into main Jun 27, 2024
18 checks passed
@mpysiak mpysiak deleted the create-gmv-command branch June 27, 2024 06:54
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.

5 participants