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

[Admin] Disable toggling of base currency #6354

Conversation

NoResponseMate
Copy link
Contributor

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

Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

We need to check if we can use form type extension instead.

{
parent::buildForm($builder, $options);

$builder->addEventListener(FormEvents::PRE_SET_DATA, function(FormEvent $event) {
Copy link
Contributor

@pamil pamil Oct 7, 2016

Choose a reason for hiding this comment

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

function(...) -> function (...) 🎉

/**
* @author Jan Góralski <jan.goralski@lakion.com>
*/
class CannotDisableCurrency extends Constraint
Copy link
Contributor

Choose a reason for hiding this comment

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

final?

/**
* @author Jan Góralski <jan.goralski@lakion.com>
*/
class CannotDisableCurrencyValidator extends ConstraintValidator
Copy link
Contributor

Choose a reason for hiding this comment

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

final?

/**
* {@inheritdoc}
*/
public function validate($currency, Constraint $constraint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing assertion that $currency is instance of CurrencyInterface.

/**
* {@inheritdoc}
*/
public function validate($currency, Constraint $constraint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing assertion that $constraint is instance of CannotDisableCurrency.

if ($currency->getCode() !== $this->baseCurrency) {
return;
}
if ($currency->isEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing blank line above.

/**
* @author Jan Góralski <jan.goralski@lakion.com>
*/
class CurrencyType extends BaseCurrencyType
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw. I guess we can achieve the same result by using form type extensions instead of custom type. It will be also a lot cleaner.

@NoResponseMate NoResponseMate force-pushed the admin/disable-toggling-base-currency branch from c31889c to 9712adb Compare October 7, 2016 15:36
Scenario:
Given the store operates on a single channel
And it uses the "USD" currency by default
And I want to edit this currency
Copy link
Contributor

Choose a reason for hiding this comment

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

When?

@NoResponseMate NoResponseMate force-pushed the admin/disable-toggling-base-currency branch from 4fc11a0 to c3ab499 Compare October 10, 2016 18:31
@pjedrzejewski pjedrzejewski merged commit b4a39ef into Sylius:master Oct 11, 2016
@pjedrzejewski
Copy link
Member

Thanks Jasiek!

@NoResponseMate NoResponseMate deleted the admin/disable-toggling-base-currency branch October 24, 2016 13:10
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

4 participants