Add Plan Service by killianherbunot · Pull Request #4 · OpenClassrooms/FrontDesk

killianherbunot · 2016-10-19T15:55:59Z

No description provided.

arnaud-23 · 2016-10-20T10:22:41Z

src/Gateways/PlanGateway.php

+     *
+     * @return integer
+     */
+    public function recuperate($personId);


rename to 'findAllByPersonId'

arnaud-23 · 2016-10-20T10:23:11Z

src/Gateways/PlanGateway.php

+    /**
+     * @param integer $personId
+     *
+     * @return integer


return Plan[]

arnaud-23 · 2016-10-20T10:25:56Z

src/Services/Impl/PlanServiceImpl.php

+    /**
+     * {@inheritdoc}
+     */
+    public function pickUp($personId)


arnaud-23 · 2016-10-20T10:26:57Z

src/Services/PlanService.php

+     *
+     * @return integer
+     */
+    public function pickUp($personId);


arnaud-23 · 2016-10-20T10:27:06Z

src/Services/PlanService.php

+    /**
+     * @param $personId
+     *
+     * @return integer


return Plan[]

arnaud-23 · 2016-10-20T10:30:41Z

src/Repository/PlanRepository.php

+        $jsonResult = $this->apiClient->get(self::RESOURCE_NAME.'/'.$personId.'/plans');
+        $result = json_decode($jsonResult, true);
+
+        return $result;


build array of Plan object and put the data inside and then return the array of plan

arnaud-23 · 2016-10-20T10:35:25Z

tests/Doubles/Client/Impl/ApiClientMock.php

 {
+    /**
+     * @var int
+     */


the default value in a mock must be null because you should manage what id should be in the unit test
then you should put self::$id = null inside the controller so that each test construct separatly the mock and reset id to null

arnaud-23 · 2016-10-20T10:37:14Z

tests/Doubles/Gateways/PlanGatewayMock.php

+    /**
+     * @var int
+     */
+    public static $id = 1222;


same as before (ApiClientMock)

arnaud-23 · 2016-10-20T10:38:24Z

tests/Doubles/Gateways/PlanGatewayMock.php

+    /**
+     * {@inheritdoc}
+     */
+    public function recuperate($planId)


I think the signature attribute is personId

arnaud-23 · 2016-10-20T10:40:21Z

tests/Doubles/guzzlehttp/guzzle/src/ClientMock.php

+    public function get($resource)
+    {
+        self::$resource = $resource;
+        self::$config = [];


it does not make any sense, because the config will be an empty array for each test so the test of this attribute is useless

coveralls · 2016-10-21T13:27:23Z

Coverage decreased (-0.4%) to 99.58% when pulling 2580406 on add_plan_service into 0663633 on master.

arnaud-23 · 2016-10-21T14:49:50Z

src/Models/Impl/PlanBuilderImpl.php

+    private $plan;
+
+    /**
+     * @return PlanBuilder


arnaud-23 · 2016-10-24T08:30:22Z

src/Models/Impl/PlanBuilderImpl.php

+    }
+
+    /**
+     * @return Plan


arnaud-23 · 2016-10-24T08:30:54Z

src/Models/Impl/PlanBuilderImpl.php

+    /**
+     * {@inheritdoc}
+     */
+    public function withCreatedAt(\DateTime $createdAt)


arnaud-23 · 2016-10-24T08:31:39Z

src/Models/Impl/PlanBuilderImpl.php

+    /**
+     * {@inheritdoc}
+     */
+    public function withStartDate(\DateTime $startDate)


arnaud-23 · 2016-10-24T08:35:19Z

src/Models/Impl/PlanImpl.php

+    public function jsonSerialize()
+    {
+        return [
+            [


return just this array

arnaud-23 · 2016-10-24T08:55:16Z

tests/Doubles/Client/Impl/ApiClientMock.php

    {
+        self::$id = null;
+        self::$response = null;
+        self::$params = null;


= empty array

arnaud-23 · 2016-10-24T08:59:30Z

tests/Doubles/Gateways/PlanGatewayMock.php

+
+    public function __construct()
+    {
+        self::$plans = new PlanStub1();


let the possibility to the users of the library to create their own stub. so ask an array of plan in the constructor signature :

/** * param Plan[] $plans */ public function __construct(array $plans = []) { self::$plans = $plans; }

arnaud-23 · 2016-10-24T09:00:37Z

tests/Doubles/Models/PlanStub1.php

+ */
+class PlanStub1 extends PlanImpl
+{
+    const ID = 1;


alphabetic order

arnaud-23 · 2016-10-24T09:03:04Z

tests/Doubles/guzzlehttp/guzzle/src/ClientMock.php

     */
    public static $params;

-    public function __construct()


need it to initialize the static attribute with a default value (null or empty array)

arnaud-23 · 2016-10-24T09:07:24Z

tests/Services/Impl/PlanServiceImplTest.php

+    public function getPlans_ReturnId()
+    {
+        $plan = $this->service->getPlans(PersonStub1::ID);
+        $this->assertPlan(PlanGatewayMock::$plans, $plan);


there are a problem somewhere because you compare an array of plan with a single plan. This test shouldn't be green

done in the set up

arnaud-23 · 2016-10-24T12:50:39Z

src/Models/PlanBuilder.php

+    public function withStartDate(\DateTime $startDate = null);

    /**
     * @param $type


type hint is missing everywhere

arnaud-23 · 2016-10-24T12:51:46Z

src/Repository/PlanRepository.php

     */
    private $apiClient;

+    /**


prefer interface instead of implementation

arnaud-23 · 2016-10-24T12:52:53Z

tests/Doubles/Gateways/PlanGatewayMock.php

    public static $plans;

-    public function __construct()
+    public function __construct(array $plan = [])


plans because you need several plan

arnaud-23 · 2016-10-24T12:54:08Z

tests/Doubles/Models/PlanTestCase.php

+     * @param Plan[] $expected
+     * @param Plan[] $actual
+     */
+    protected function assertPlan($expected, $actual)


not at all it was good, it was where you used it you need to change the ways

arnaud-23 · 2016-10-24T12:58:12Z

tests/Services/Impl/PlanServiceImplTest.php

    {
        $this->service = new PlanServiceImpl();
-        $this->service->setPlanGateway(new PlanGatewayMock());
+        $this->service->setPlanGateway(new PlanGatewayMock([[new PlanStub1()]]));


[PlanStub1::ID => new PlanStub1()]

arnaud-23 · 2016-10-24T13:57:42Z

tests/Services/Impl/PlanServiceImplTest.php

+
+        $count = count($plans);
+        for ($i = 0; $i < $count; ++$i) {
+            $this->assertPlan(PlanGatewayMock::$plans[$i], $plans[$i]);


cf: GetUsersTest L43

$i = 0; foreach ($plans as $plan) { $planStub = '\path\to\PlanStub'.++$i; $this->assertPlan(new $planStub(), $plan); }

arnaud-23 · 2016-10-24T13:59:09Z

tests/Services/Impl/PlanServiceImplTest.php

    {
        $this->service = new PlanServiceImpl();
-        $this->service->setPlanGateway(new PlanGatewayMock([[new PlanStub1()]]));
+        $this->service->setPlanGateway(new PlanGatewayMock([new PlanStub1()]));


you need to force the array key to handle it because api or database will never start with a key = 0
[planStub1::Id => new PlanStub1()]

coveralls · 2016-10-24T14:10:09Z

Coverage decreased (-0.4%) to 99.578% when pulling 81c127f on add_plan_service into 0663633 on master.

arnaud-23 · 2016-10-24T14:28:02Z

src/Repository/PlanRepository.php

+    {
+        $plans = [];
+        foreach ($result['plans'] as $plan) {
+            $plans[] = $planBuilder


now you planBuilder is a class property so you can call it directly here and don't have to call in method

coveralls · 2016-10-24T14:43:28Z

Coverage decreased (-0.4%) to 99.578% when pulling 469d688 on add_plan_service into 0663633 on master.

coveralls · 2016-10-24T14:59:47Z

Coverage decreased (-0.4%) to 99.578% when pulling fb9dbd9 on add_plan_service into 0663633 on master.

killianherbunot force-pushed the add_plan_service branch 3 times, most recently from 789f6b6 to faa20e0 Compare October 20, 2016 10:14

arnaud-23 suggested changes Oct 20, 2016

View reviewed changes

arnaud-23 suggested changes Oct 24, 2016

View reviewed changes

killianherbunot force-pushed the add_plan_service branch 2 times, most recently from c4a1e78 to d33e41e Compare October 24, 2016 12:37

arnaud-23 suggested changes Oct 24, 2016

View reviewed changes

arnaud-23 reviewed Oct 24, 2016

View reviewed changes

arnaud-23 approved these changes Oct 24, 2016

View reviewed changes

Add Get Plan Service + clean

fb9dbd9

killianherbunot force-pushed the add_plan_service branch from 469d688 to fb9dbd9 Compare October 24, 2016 14:57

killianherbunot merged commit f14418b into master Oct 24, 2016

killianherbunot deleted the add_plan_service branch October 26, 2016 16:27

Conversation

killianherbunot commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

coveralls commented Oct 21, 2016 •

edited

Loading

coveralls commented Oct 24, 2016 •

edited

Loading

coveralls commented Oct 24, 2016 •

edited

Loading

coveralls commented Oct 24, 2016 •

edited

Loading