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

[Currency] Introduce immutable code for Currency #3675

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

tuka217
Copy link
Contributor

@tuka217 tuka217 commented Dec 4, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets -
License MIT
Doc PR -

@tuka217 tuka217 force-pushed the immutable-code-currency branch from 6212d54 to bd7a20b Compare December 4, 2015 10:32

Scenario: Cannot update currency code
When I am editing currency with code "USD"
Then The code field should be disabled
Copy link
Member

Choose a reason for hiding this comment

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

Lowercase the.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? In all steps after 'Then' we starts sentence with capital letter.

Copy link
Member

Choose a reason for hiding this comment

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

If yes, we should not :) We start with capital "I" and it is correct, due to english grammar rules. However, other words should be wrote down with lowercase (as i.e. Given store has default configuration).
Moreover, in more abstract way of thinking, we always start sentence with capital letter, because keyword is part of the sentence! 😄 Making behat scenarios more natural means also thinking about them as natural language, I suppose :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@tuka217 tuka217 force-pushed the immutable-code-currency branch 4 times, most recently from b5c76fb to b1f5a92 Compare December 4, 2015 12:43
public function __construct($type = 'text')
{
$this->type = $type;
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line after this.

@tuka217 tuka217 force-pushed the immutable-code-currency branch 3 times, most recently from fec0b82 to 0da7656 Compare December 9, 2015 09:51
@tuka217 tuka217 changed the title [WIP][Currency] Introduce immutable code for Currency [Currency] Introduce immutable code for Currency Dec 9, 2015
$this->addSql('ALTER TABLE sylius_channel DROP FOREIGN KEY FK_16C8119EECD792C0');
$this->addSql('DROP INDEX IDX_16C8119E743BF776 ON sylius_channel');
$this->addSql('DROP INDEX IDX_16C8119EECD792C0 ON sylius_channel');
$this->addSql('ALTER TABLE sylius_channel DROP default_locale_id, DROP default_currency_id');
Copy link
Contributor

Choose a reason for hiding this comment

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

The migration is incorrect and should be removed.

[CurrencyBundle] Add AddCodeFormSubscriber to CurrencyType
[ResourceBundle] Modify AddCodeFormSubscriber to make possible passing type of code field
[Currency] Introduce code for Currency
@tuka217 tuka217 force-pushed the immutable-code-currency branch from 0da7656 to 3ce46f7 Compare December 9, 2015 10:37
pjedrzejewski pushed a commit that referenced this pull request Dec 9, 2015
[Currency] Introduce immutable code for Currency
@pjedrzejewski pjedrzejewski merged commit e559c86 into Sylius:master Dec 9, 2015
@pjedrzejewski
Copy link
Member

Thank you Ania!

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