[Admin] Editing countries #4442
Conversation
Arminek
commented
Mar 9, 2016
Arminek
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | #4374 |
| License | MIT |
Arminek
force-pushed
the
editing-countries
branch
2 times, most recently
from
3d8c019
to
fda22c7
Compare
| { | ||
| $rows = $this->tableManipulator->getRowsWithFields($this->getElement('table'), $parameters); | ||
|
|
||
| if (!empty($rows)) { |
There was a problem hiding this comment.
return !empty($rows); ?
Arminek
force-pushed
the
editing-countries
branch
from
fda22c7
to
38097e5
Compare
michalmarcinkowski
added
the
UX
| @todo | ||
| Scenario: Disabling country | ||
| Given the store has "France" country enabled | ||
| And I want to disable country |
There was a problem hiding this comment.
Wouldn't it be better with I want to edit country "France" or something alike?
That way you'd have one @Given method instead of 2 doing the exact same thing.
There was a problem hiding this comment.
I think toggling is so specific function, that it is worth to mention the beginning state of entity. I vote for 2 different steps.
There was a problem hiding this comment.
I want to disable country is a step that is more suitable for toggling on the index page.
There was a problem hiding this comment.
Anyway, I want to edit this country
Arminek
force-pushed
the
editing-countries
branch
3 times, most recently
from
7fe10f8
to
a44ea94
Compare
| { | ||
| $this->sharedStorage->set('country', $country); | ||
| $this->countryRepository->add($country); | ||
| } |
There was a problem hiding this comment.
Can't we somehow merge this steps or extract common logic? They're pretty similar :D
There was a problem hiding this comment.
Most likely it would work as one step with 2 tags, cause transformer is taking care of the difference here.
There was a problem hiding this comment.
I was thinking about such a solution, sounds good for me 👍
Arminek
force-pushed
the
editing-countries
branch
from
a44ea94
to
d890dc5
Compare
Arminek
force-pushed
the
editing-countries
branch
from
e91b4ea
to
9881197
Compare
| @@ -52,6 +72,30 @@ public function iWantToCreateNewCountry() | |||
| } | |||
|
|
|||
| /** | |||
| * @Given /^(country "([^"]*)") should appear in the store$/ | |||
| */ | |||
| public function countryWithNameShouldAppearInTheStore(CountryInterface $country) | |||
There was a problem hiding this comment.
Method name should reflect step definition -> countryShouldAppearInTheStore
Arminek
force-pushed
the
editing-countries
branch
2 times, most recently
from
7a38fb9
to
8d546fa
Compare
|
|
||
| @ui | ||
| Scenario: Enabling country | ||
| Given the store operates in "France" but it is disabled |
There was a problem hiding this comment.
What about putting but it is disabled in a new line? WDYT?
There was a problem hiding this comment.
👍, could be applicated to any model implementing TogglableInterface.
pjedrzejewski
added
Admin
Arminek
force-pushed
the
editing-countries
branch
2 times, most recently
from
eb8df80
to
cbe64f7
Compare
…tion, add country transformer context, add managing countries setup context
Arminek
force-pushed
the
editing-countries
branch
2 times, most recently
from
df68883
to
2258eec
Compare
Arminek
force-pushed
the
editing-countries
branch
from
2258eec
to
5b2bb04
Compare
Arminek
force-pushed
the
editing-countries
branch
from
930aaf1
to
f9eeb15
Compare
| */ | ||
| public function iShouldBeNotifiedAboutSuccessfulCreation() | ||
| { | ||
| $doesSuccessMessageAppear = $this->notificationAccessor->hasSuccessMessage(); |
There was a problem hiding this comment.
Please remove this variable, it does not add much readability, it is used only once and naming it properly seems like a challenge. Agree?
[Admin] Editing countries
|
Thanks Arek! Please apply my comments in one of your further PRs. 👍 |
| $this->thisCountryShouldBeEnabled($country); | ||
| } | ||
|
|
||
| function it_throws_not_equal_exception_if_country_has_not_proper_status(IndexPageInterface $countryIndexPage, CountryInterface $country) |
There was a problem hiding this comment.
not_equal_exception -> invalid_argument_exception
| } catch (\InvalidArgumentException $exception) { | ||
| return false; | ||
| } catch (ElementNotFoundException $exception) { | ||
| return false; |
There was a problem hiding this comment.
Current logic is not totally correct, we shouldn't silence these exceptions. Example:
if (!$this->isCountryDisabled($country)) {
// here I assume that the country is enabled, but it doesn't have to be since there could be exception
}
