[Admin] Adding countries #4395

Arminek · 2016-03-04T14:46:30Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	#4374
License	MIT

pjedrzejewski · 2016-03-04T15:41:07Z

etc/behat/suites.yml

@@ -34,6 +34,7 @@ imports:
    - suites/ui_cart.yml
    - suites/ui_channel.yml
    - suites/ui_checkout.yml
+    - suites/ui_localization.yml


I think more appropriate suite name would be addressing, where we will put:

country management

zones

Any better ideas? :)

lchrusciel · 2016-03-10T10:32:10Z

etc/behat/suites/ui_addressing.yml

+        ui_addressing:
+            contexts_as_services:
+                - sylius.behat.context.hook.doctrine_orm
+                - sylius.behat.context.transform.lexical


We used to split each type of context with blank line.

lchrusciel · 2016-03-10T10:45:12Z

src/Sylius/Behat/Context/Ui/Admin/ManagingCountryContext.php

+     * @param IndexPageInterface $adminCountryIndexPage
+     * @param CreatePageInterface $adminCountryCreatePage
+     */
+    public function __construct(SharedStorageInterface $sharedStorage, IndexPageInterface $adminCountryIndexPage, CreatePageInterface $adminCountryCreatePage)


This line is too long

lchrusciel · 2016-03-10T11:13:35Z

src/Sylius/Behat/Page/Admin/Crud/IndexPage.php

+    }
+
+    /**
+     * @return string


{@inheritdoc}

pamil · 2016-03-10T15:20:46Z

src/Sylius/Behat/Page/Admin/Crud/IndexPageInterface.php

+    /**
+     * @return bool
+     *
+     * @throws ElementNotFoundException


If we catch that exception, it wouldn't be needed

michalmarcinkowski · 2016-03-11T09:10:18Z

src/Sylius/Behat/Page/Admin/Crud/IndexPage.php

+        try {
+            $rows = $this->tableManipulator->getRowsWithFields($this->getElement('table'), $parameters);
+
+            return 0 !== count($rows);


return 1 === count($rows) to validate that only one resource was found.

michalmarcinkowski · 2016-03-11T09:19:19Z

👍

pjedrzejewski · 2016-03-11T09:36:49Z

etc/behat/services.xml

@@ -23,6 +23,9 @@
        <parameter key="sylius.behat.table_manipulator.class">Sylius\Behat\TableManipulator</parameter>
        <parameter key="sylius.behat.page.class">Sylius\Behat\Page\Page</parameter>
        <parameter key="sylius.behat.symfony_page.class">Sylius\Behat\Page\SymfonyPage</parameter>
+        <parameter key="sylius.behat.page.admin.crud.index.class">Sylius\Behat\Page\Admin\Crud\IndexPage</parameter>
+        <parameter key="sylius.behat.page.admin.crud.create.class">Sylius\Behat\Page\Admin\Crud\CreatePage</parameter>
+        <parameter key="sylius.behat.page.admin.crud.edit.class">Sylius\Behat\Page\Admin\Crud\EditPage</parameter>


Maybe in code we should be consistent and use Update instead of Edit. UI (buttons, etc.) is a different story, but in Sylius codebase everything is "update".

UpdatePage ?

For me EditPage sounds more appropriate than UpdatePage. The action is update, but the page is where we edit (want to change) and something is updated after we update it (save edited changes).

To be honest I found EditPage much more appropriate, but in terms of CRUD. On the other hand Resource action is named Update, so for now I would stay with update.

Yes, but update de facto has no page, it only updates data on object. I know action in ResourceController is named updateAction, but in this case I think we should based on what we're using this page for. For editing object, update follows edit.

For now we use update as a name of action and routes suffix for both displaying form and updating resource. As @pjedrzejewski mention, in a whole codebase, we use update to express everything around update. For me is more important to be consistent with the codebase rather than on feeling what is more appropriate. Action is with an update keyword, route is with an update word, so page should also be with update keyword.

Ok, I just feel update more than action, edit more than having access to update possibility. But I'm 100% sure consistency is the greatest aim we should follow for now.

For the reasons outlined by @lchrusciel I say we go with UpdatePage for now, to be consistent. I think the whole discussion is caused by completely different problem: lack of separation of edit and update in ResourceController, but that's not something we can look at right now.

…y context and custom Create page for country resource. Add grid and routing configuration in AdminBundle

Arminek · 2016-03-11T13:36:28Z

One thing we need to discuss, this protected getter on resource name and i think it is ready to go.

[Admin] Adding countries

pjedrzejewski · 2016-03-11T15:32:35Z

Thanks Arek! Nice work! 👍

[Admin] Adding countries

Arminek changed the title ~~Adding country feature~~ [Admin] Adding country feature Mar 4, 2016

Arminek changed the title ~~[Admin] Adding country feature~~ [Admin] Adding scenarios for country feature Mar 4, 2016

Arminek changed the title ~~[Admin] Adding scenarios for country feature~~ [Admin] Adding scenarios for managing country Mar 4, 2016

pjedrzejewski reviewed Mar 4, 2016
View reviewed changes

Arminek force-pushed the managing_countries branch 8 times, most recently from 0f9e5bc to 74ba0bf Compare March 9, 2016 12:46

Arminek changed the title ~~[Admin] Adding scenarios for managing country~~ [Admin] Adding country Mar 9, 2016

Arminek changed the title ~~[Admin] Adding country~~ [Admin] Adding countries Mar 9, 2016

Arminek force-pushed the managing_countries branch 2 times, most recently from 525f3fa to 4e876ee Compare March 10, 2016 10:16

lchrusciel reviewed Mar 10, 2016
View reviewed changes

Arminek force-pushed the managing_countries branch 2 times, most recently from b2f410d to 33ccb6b Compare March 10, 2016 10:37

lchrusciel reviewed Mar 10, 2016
View reviewed changes

Arminek force-pushed the managing_countries branch from 33ccb6b to 98ba52e Compare March 10, 2016 11:09

lchrusciel reviewed Mar 10, 2016
View reviewed changes

src/Sylius/Behat/Page/Admin/Crud/IndexPage.php

}

/**

* @return string

Copy link

Member

lchrusciel Mar 10, 2016

Choose a reason for hiding this comment

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

{@inheritdoc}

pamil reviewed Mar 10, 2016
View reviewed changes

Arminek force-pushed the managing_countries branch from e264853 to 955b9aa Compare March 10, 2016 23:38

michalmarcinkowski reviewed Mar 11, 2016
View reviewed changes

Arminek force-pushed the managing_countries branch from 955b9aa to d7756f4 Compare March 11, 2016 09:35

pjedrzejewski reviewed Mar 11, 2016
View reviewed changes

Arminek force-pushed the managing_countries branch 3 times, most recently from 7fa2850 to 711e76b Compare March 11, 2016 13:13

Arminek added 3 commits March 11, 2016 14:21

Adding country feature

3a3969d

Introduce admin CRUD pages, Index page and Create page. Create countr…

c36f70f

…y context and custom Create page for country resource. Add grid and routing configuration in AdminBundle

Create edit page

c97b8bd

Arminek force-pushed the managing_countries branch from 711e76b to 5de44a9 Compare March 11, 2016 13:33

Review fixes

d296fe2

Arminek force-pushed the managing_countries branch from 5de44a9 to d296fe2 Compare March 11, 2016 13:56

pjedrzejewski pushed a commit that referenced this pull request Mar 11, 2016

Merge pull request #4395 from Arminek/managing_countries

894fa80

[Admin] Adding countries

pjedrzejewski merged commit 894fa80 into Sylius:master Mar 11, 2016

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

Merge pull request Sylius#4395 from Arminek/managing_countries

cef3591

[Admin] Adding countries

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

Merge pull request Sylius#4395 from Arminek/managing_countries

5856332

[Admin] Adding countries

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Admin] Adding countries #4395

[Admin] Adding countries #4395

Arminek commented Mar 4, 2016

pjedrzejewski Mar 4, 2016

lchrusciel Mar 10, 2016

pjedrzejewski Mar 10, 2016

lchrusciel Mar 10, 2016

lchrusciel Mar 10, 2016

pamil Mar 10, 2016

michalmarcinkowski Mar 11, 2016

michalmarcinkowski commented Mar 11, 2016

pjedrzejewski Mar 11, 2016

Arminek Mar 11, 2016

michalmarcinkowski Mar 11, 2016

lchrusciel Mar 11, 2016

Zales0123 Mar 11, 2016

lchrusciel Mar 11, 2016

Zales0123 Mar 11, 2016

pjedrzejewski Mar 11, 2016

Arminek commented Mar 11, 2016

pjedrzejewski commented Mar 11, 2016

[Admin] Adding countries #4395

[Admin] Adding countries #4395

Conversation

Arminek commented Mar 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arminek commented Mar 11, 2016

pjedrzejewski commented Mar 11, 2016