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

[ApiBundle][ProductVariant] Unification of channelCode in channelPricing #15705

Conversation

Wojdylak
Copy link
Member

@Wojdylak Wojdylak commented Jan 5, 2024

Q A
Branch? 1.13
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets N/A
License MIT

@Wojdylak Wojdylak requested a review from a team as a code owner January 5, 2024 10:16
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Jan 5, 2024
Copy link

github-actions bot commented Jan 5, 2024

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

tests/Api/JsonApiTestCase.php Outdated Show resolved Hide resolved
@@ -35,7 +43,7 @@
</constraint>
</property>
<property name="channelCode">
<constraint name="NotBlank">
<constraint name="NotNull">
Copy link
Contributor

Choose a reason for hiding this comment

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

Guessing we're missing a test what happens with a configuration like

['channelPricings' => ['' => []]];

just to be sure it won't blow up

Copy link
Member Author

Choose a reason for hiding this comment

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

There is ExistingChannelCodeValidator

if ($value === null) {
    return;
}

if ($this->channelRepository->findOneByCode($value) === null) {
    $this->context->buildViolation($constraint->message)
        ->setParameter('{{ channelCode }}', $value)
        ->addViolation()
    ;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

So in case when it's an empty string we're unnecessarily calling the database.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, you're right. I'll revert to using NotBlank, and in the ExistingChannelCodeValidator, I'll add a return statement for empty value.

@@ -36,7 +44,7 @@
</property>
<property name="channelCode">
<constraint name="NotBlank">
<option name="message">sylius.channel_pricing.channel_code.not_null</option>
<option name="message">sylius.channel_pricing.channel_code.not_blank</option>
Copy link
Member

Choose a reason for hiding this comment

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

Should this be mentioned in UPGRADE? If someone overrode this particular translation, it won't work anymore for them

Copy link
Member

Choose a reason for hiding this comment

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

From what I see, there's no such translation key as not_null.

@jakubtobiasz jakubtobiasz merged commit 55950de into Sylius:1.13 Jan 9, 2024
25 checks passed
@jakubtobiasz
Copy link
Member

Thank you, @Wojdylak!

@Wojdylak Wojdylak deleted the SYL-3278-api-adjust-managing-channel-pricings-similarly-to-translations branch February 21, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants