Add endpoint for getting item properties #53

foglcz · 2017-12-04T17:22:24Z

Depends on matej's deploy of RAD-720

Depends on #46 - before merging, change base of PR

Implements GET /item-properties

OndraM

Could you also add some exmaple usage to readme? (But keep it short - as this is just an "auxiliary" endpoint used mainly for debugging, and not a main feature :) ).

Also something is missing code coverage according to coveralls, could you have a look, please?

OndraM · 2017-12-05T10:39:28Z

src/RequestBuilder/ItemPropertiesModifierRequestBuilder.php

@@ -7,10 +7,8 @@
 use Lmc\Matej\Model\Command\ItemPropertySetup;
 use Lmc\Matej\Model\Request;

-class ItemPropertiesSetupRequestBuilder extends AbstractRequestBuilder
+class ItemPropertiesModifierRequestBuilder extends ItemPropertiesGetterRequestBuilder


My preference is to keep this name as is - its meaning is already IMHO clear and distinctive from the GET version, and this adds no extra value - maybe even is confusing, because you actually cannot modify (like change data type) item properties once they are created in the database. So the the ItemPropertiesSetupRequestBuilder is for me ok. What do you think?

OndraM · 2017-12-05T10:40:01Z

src/RequestBuilder/ItemPropertiesGetterRequestBuilder.php

+use Fig\Http\Message\RequestMethodInterface;
+use Lmc\Matej\Model\Request;
+
+class ItemPropertiesGetterRequestBuilder extends AbstractRequestBuilder


Its a builder, which build request to get item properties (Its not building "getter request"), so why not just ItemPropertiesGetRequestBuilder? (But only if the other one is kept as ItemPropertiesSetupRequestBuilder)

OndraM · 2017-12-05T10:42:32Z

tests/IntegrationTests/ItemPropertiesSetupTest.php

+        array $properties
+    ): ItemPropertiesModifierRequestBuilder {
+        foreach ($properties as $name => $definition) {
+            $property = forward_static_call([Command\ItemPropertySetup::class, $definition[0]], $name);


ItemPropertySetup::$definition[0]($name) :)?

That wouldn't work as PHP would think that $definition is a static array on ItemPropertySetup class.

Would have to be ItemPropertySetup::{$definition[0]}($name) which I found pretty ugly :|

OndraM · 2017-12-05T10:43:54Z

tests/IntegrationTests/ItemPropertiesSetupTest.php

+    }
+
+    /**
+     * @return array[string, array[string, mixed]]


I'm not sure there is any common PHP standard of defining internal array structure (and PHPStorm don't understand this, no?) , so we may just stick with standardized array[]...

It's true that phpstorm doesn't understand this notation, but PHPStan does :)

Is it documented somewhere in PHPStan? I couldn't find it...

OndraM · 2017-12-05T10:49:16Z

src/RequestBuilder/ItemPropertiesModifierRequestBuilder.php

@@ -7,10 +7,8 @@
 use Lmc\Matej\Model\Command\ItemPropertySetup;
 use Lmc\Matej\Model\Request;

-class ItemPropertiesSetupRequestBuilder extends AbstractRequestBuilder
+class ItemPropertiesModifierRequestBuilder extends ItemPropertiesGetterRequestBuilder


If the inheritance is there just because of the endpoint path, I'd not introduce it at all and rather have the endpoint path copied on two places. "Setup request builder" is from my POV not specific version of "Get request builder". It would only make sense IMHO to have AbstractItemPropertiesRequestBuilder with two childs, but would be a bit overengineered :).

OndraM · 2017-12-07T10:26:48Z

src/RequestBuilder/ItemPropertiesGetRequestBuilder.php

+use Fig\Http\Message\RequestMethodInterface;
+use Lmc\Matej\Model\Request;
+
+class ItemPropertiesGetRequestBuilder extends AbstractRequestBuilder


Could you please add also simple unit test for this builder? (See other unit tests for builders).

OndraM · 2017-12-07T13:11:16Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 <!-- There is always Unreleased section on the top. Subsections (Added, Changed, Fixed, Removed) should be added as needed. -->

 ## Unreleased
+- Endpoint `$matej->getItemProperties()` that returns all defined item properties


### Added - Endpoint to get all defined item properties in the Matej database (`$matej->request()->getItemProperties()`).

OndraM · 2017-12-11T09:57:47Z

src/RequestBuilder/ItemPropertiesSetupRequestBuilder.php

@@ -7,10 +7,8 @@
 use Lmc\Matej\Model\Command\ItemPropertySetup;
 use Lmc\Matej\Model\Request;

-class ItemPropertiesSetupRequestBuilder extends AbstractRequestBuilder
+class ItemPropertiesSetupRequestBuilder extends ItemPropertiesGetRequestBuilder


What do you think about this? (The comment got "outdated" by github after the rebase...)

If the inheritance is there just because of the endpoint path, I'd not introduce it at all and rather have the endpoint path copied on two places. "Setup request builder" is from my POV not specific version of "Get request builder". It would only make sense IMHO to have AbstractItemPropertiesRequestBuilder with two childs, but would be a bit overengineered :).

This is a very good point; I was thinking from the perspective of REST endpoints - and not from the perspective of RequestBuilders.

Will update code in a sec.

OndraM

If you could please rebase the PR (to get rid of the fixup commits) and we can merge it then 🎉

foglcz requested a review from OndraM December 4, 2017 17:22

OndraM reviewed Dec 5, 2017

View reviewed changes

foglcz force-pushed the feature/get-item-properties-setup branch from 28b6d5a to 96c7edd Compare December 5, 2017 15:37

foglcz changed the base branch from master to feature/integration-tests December 5, 2017 15:38

OndraM requested changes Dec 7, 2017

View reviewed changes

OndraM force-pushed the feature/integration-tests branch from 02ca722 to 02cf89f Compare December 7, 2017 13:09

OndraM reviewed Dec 7, 2017

View reviewed changes

OndraM changed the base branch from feature/integration-tests to master December 7, 2017 13:14

OndraM changed the title ~~Add endpoint for getting item properties (do not merge)~~ Add endpoint for getting item properties Dec 7, 2017

OndraM force-pushed the feature/get-item-properties-setup branch from e5aa9f9 to 6c15fb2 Compare December 7, 2017 13:16

OndraM reviewed Dec 11, 2017

View reviewed changes

OndraM approved these changes Dec 11, 2017

View reviewed changes

OndraM requested changes Dec 11, 2017

View reviewed changes

Add endpoint for getting item properties (RAD-720)

d9db672

foglcz force-pushed the feature/get-item-properties-setup branch from 2504f2e to d9db672 Compare December 11, 2017 13:48

OndraM approved these changes Dec 11, 2017

View reviewed changes

OndraM merged commit 4e4b3af into master Dec 11, 2017

foglcz deleted the feature/get-item-properties-setup branch December 11, 2017 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoint for getting item properties #53

Add endpoint for getting item properties #53

foglcz commented Dec 4, 2017 •

edited

OndraM left a comment

OndraM Dec 5, 2017

OndraM Dec 5, 2017

OndraM Dec 5, 2017

foglcz Dec 11, 2017 •

edited

OndraM Dec 5, 2017

foglcz Dec 11, 2017

OndraM Dec 11, 2017 •

edited

OndraM Dec 5, 2017

OndraM Dec 7, 2017

OndraM Dec 7, 2017

OndraM Dec 11, 2017 •

edited

foglcz Dec 11, 2017

OndraM left a comment •

edited

Add endpoint for getting item properties #53

Add endpoint for getting item properties #53

Conversation

foglcz commented Dec 4, 2017 • edited

OndraM left a 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

foglcz Dec 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraM Dec 11, 2017 • edited

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

OndraM Dec 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndraM left a comment • edited

Choose a reason for hiding this comment

foglcz commented Dec 4, 2017 •

edited

foglcz Dec 11, 2017 •

edited

OndraM Dec 11, 2017 •

edited

OndraM Dec 11, 2017 •

edited

OndraM left a comment •

edited