-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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] Tax category creation #4587
[Admin] Tax category creation #4587
Conversation
c36e7b3
to
e48838e
Compare
@lchrusciel Rebase needed. |
on it |
e48838e
to
1693444
Compare
and done |
7c42404
to
e18d10b
Compare
function it_throws_an_exception_if_the_message_on_a_page_is_not_related_to_creation(NotificationAccessorInterface $notificationAccessor) | ||
{ | ||
$notificationAccessor->hasSuccessMessage()->willReturn(true); | ||
$notificationAccessor->isSuccessfullyCreatedFor('tax_category')->willReturn(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not with ManagingTaxCategoryContext::RESOURCE_NAME
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which approach is better. I'm not convinced about using const in specs. I just prefer to write it by my self, especially, because I do not have this const when I am writing first lines of spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes the specs highly vulnerable, as changing introducing a constant should happen for a reason - to keep the code DRY. It's just the same situation as with typehinting all the collaborators or using @mixin
- the refactoring in most IDEs will understand it and prevent us from unnecessary, repetitive work.
As for a non-existing constant, it's nothing unusual while writing specs, as you also don't have the SUT or methods you are using on it :)
* @Then this tax category name should be :taxCategoryName | ||
*/ | ||
public function thisTaxCategoryNameShouldBe($taxCategoryName) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we always assert that page is open before using it?
fe41e5a
to
3c3ff30
Compare
{ | ||
use SpecifiesCode; | ||
use NamesIt; | ||
use DescribesItAs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be consistent:
- SpecifiesItsCode ❓
- NamesIt 🆗
- DescribesIt~~As~~
@lchrusciel nice work! 👍 |
3c3ff30
to
b4fa539
Compare
Perfect, thanks Łukasz! |
Based on #4573