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

Fix Customer Group Type and RuleChecker #6374

Merged
merged 2 commits into from
Oct 11, 2016
Merged

Fix Customer Group Type and RuleChecker #6374

merged 2 commits into from
Oct 11, 2016

Conversation

tchapi
Copy link
Contributor

@tchapi tchapi commented Oct 10, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related tickets
License MIT

Fix the CustomerGroupType and RuleChecker

@@ -39,7 +39,11 @@ public function isEligible(PromotionSubjectInterface $subject, array $configurat
return false;
}

if ($configuration['groups'] === $customer->getGroup()->getId()) {
if (is_null($customer->getGroup())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the following convention for checking for null value: if (null === $customer->getGroup()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, changed

return false;
}

if ($configuration['group'] === $customer->getGroup()->getId()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use code, same as we do in TaxonRuleChecker (can be done in a separate PR).

@pjedrzejewski pjedrzejewski merged commit 93ab216 into Sylius:master Oct 11, 2016
@tchapi tchapi deleted the fix-customer-group-rule-checker branch October 11, 2016 08:37
@pjedrzejewski
Copy link
Member

Thanks @tchapi! Regarding using the code, we can handle that in a separate PR or when covering this functionality with scenarios.

@tchapi
Copy link
Contributor Author

tchapi commented Oct 11, 2016

Yes, that's what I thought too because it's not really related to this PR in particular

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…le-checker

Fix Customer Group Type and RuleChecker
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