[RFC] Address book #229

jseparovic1 · 2017-10-26T14:43:21Z

lchrusciel · 2017-10-26T21:50:38Z

Each of class misses empty line at the end of the file. Also, there are a few redundant comments. Did you think about using an Address VO from model namespace?

lchrusciel · 2017-10-27T09:51:25Z

Also, @jseparovic1 can you share your thoughts here or in Slack PM what are your experiences with working with ShopApiPlugin? Was it easy to add these endpoints?

antonioperic · 2017-10-27T12:36:11Z

@lchrusciel we spoke with @pjedrzejewski and agree to not implement PATCH for updating as PUT is enough. Just to have in mind for reviewing code

lchrusciel · 2017-10-31T08:28:22Z

@antonioperic @jseparovic1 please ping me, when it will be ready for review. Or should I check it right now?

jseparovic1 · 2017-10-31T09:47:13Z

@lchrusciel i need to finish unit tests, but you could check it right now, and let me know if there is something to change.

lchrusciel · 2017-11-02T19:55:27Z

@jseparovic1 would it be a problem for you to split this PR into smaller ones?

lchrusciel · 2017-11-02T19:56:53Z

spec/Command/UpdateAddressSpec.php

+use Sylius\ShopApiPlugin\Command\UpdateAddress;
+use Sylius\ShopApiPlugin\Model\Address;
+
+class UpdateAddressSpec extends ObjectBehavior


some of the specs are missing a final keyword

lchrusciel · 2017-11-02T20:00:12Z

spec/Handler/CreateAddressHandlerSpec.php

+        ProvinceInterface $province,
+        FactoryInterface $addressFactory
+    ) {
+        $tokenStorage->getToken()->willReturn($userToken);


I think it would be better to fetch the token inside a controller instead of the handler. It would decouple our logic from authorization mechanism, what is crucial if one would like to use an oauth for example. Or other open id.

I agree, i will refactor this. So i should fetch token inside a controller and pass it to related command and then in handler just get user from the token ?

I think we could even get a user in the controller and just add its email here. Security is a part of infrastructure at the end. And even if I didn't introduce proper namespace separation(yet ;)) we could try to follow that rule, I suppose. What's your opinion?

Okay so i will get user from token , and pass its email to a command. The in the handler i will resolve user from email. This sounds nice to me and i agree that getting user from token in handler is really bad idea because of tightly coupling. I have few more situations with getting current user, so I want to be sure it is done in proper way.

lchrusciel · 2017-11-02T20:03:10Z

src/Command/UpdateAddress.php

+    private $address;
+
+    /**
+     * CreateAddress constructor.


these comments are redundant. There are at least 6 more to delete.

lchrusciel · 2017-11-02T20:03:41Z

src/Command/UpdateAddress.php

@@ -0,0 +1,105 @@
+<?php
+
+declare(strict_types=1);


I have to refactor the plugin to introduce it in the whole codebase... Just a side note

src/Controller/AddressBook/ShowAddressBookAction.php

src/Resources/config/services/actions/address_book.xml

src/Resources/translations/validators.en.yml

src/Resources/config/validation/SetDefaultAddressRequest.xml

tests/Controller/AddressBookCreateAddressApiTest.php

tests/Responses/Expected/address_book/show_address_book_response.json

jseparovic1 · 2017-11-06T11:37:54Z

@lchrusciel I refactored this like we agreed. Unfortunately I don't have time right now for splitting up this into smaller PR.

lchrusciel

It is good to merge, but rebase is needed.

lchrusciel · 2017-11-09T22:22:39Z

src/Model/Address.php

+     *
+     * @return Address
+     */
+    public static function createFromRequest(Request $request)


Missing return type

lchrusciel · 2017-11-14T13:27:00Z

The tests are failing... :(

pjedrzejewski · 2017-11-27T22:27:47Z

@jseparovic1 Hi! Do you guys plan to update this PR? :)

jseparovic1 · 2017-11-28T12:42:23Z

@pjedrzejewski Hi, i plan to update it in upcoming weeks. Since we have tight project deadline I decide to customize this feature for our needs and finish PR later :)

lchrusciel · 2017-11-29T08:24:02Z

@jseparovic1 are you adjusting forked code or used a custom implementation? I'm just wondering how it will work in such a case.

Also, sorry for long feedback loops.

jseparovic1 · 2017-11-29T17:53:50Z

@lchrusciel forked code remains the same, i created new classes under my project namespace. It should work like a charm :)

lchrusciel · 2018-02-14T07:47:51Z

I would love to merge it, but we need tests to pass to do it. :(

jseparovic1 · 2018-02-27T00:25:25Z

@lchrusciel I installed php 7.2 and found an error so text are now fixed. PR should be ready now.

lchrusciel

Thank you very much for all your work! I hope you enjoyed working with the plugin. Most of my comments are just some small changes but for example, better validation message is pretty important. Will you find some time to fix the PR?

src/Command/CreateAddress.php

lchrusciel · 2018-03-09T21:38:35Z

src/Command/RemoveAddress.php

+
+namespace Sylius\ShopApiPlugin\Command;
+
+class RemoveAddress


missing final

src/Controller/AddressBook/CreateAddressAction.php

lchrusciel · 2018-03-09T21:43:49Z

src/Controller/AddressBook/RemoveAddressAction.php

+        }
+
+        /** @var ShopUserInterface $shopUser */
+        $shopUser = $this->tokenStorage->getToken()->getUser();


Assert::isInstanceOf($user, ShopUserInterface::class);

src/Controller/AddressBook/SetDefaultAddressAction.php

src/Resources/config/routing.yml

tests/Controller/AddressBookCreateAddressApiTest.php

tests/Controller/AddressBookRemoveAddressApiTest.php

tests/DataFixtures/ORM/address.yml

tests/DataFixtures/ORM/customer.yml

mamazu · 2018-08-16T12:17:28Z

This would be a great feature to have.

…ddress endpoint

…ng handler

…parameters order

pamil · 2018-08-20T13:12:12Z

Woah, I hope I got rebasing right 🎉

pamil · 2018-08-20T13:45:25Z

Thank you, Jurica!

mamazu · 2018-08-21T09:55:52Z

Add documentation :D

antonioperic mentioned this pull request Oct 26, 2017

As a Customer, I want manage my address book (CRUD + default setting) #26

Closed

pjedrzejewski added the New Feature label Oct 27, 2017

jseparovic1 changed the title ~~[WIP] Address book~~ [RFC] Address book Nov 2, 2017

lchrusciel reviewed Nov 2, 2017

View reviewed changes

lchrusciel approved these changes Nov 9, 2017

View reviewed changes

lchrusciel reviewed Mar 9, 2018

View reviewed changes

lchrusciel added this to the v1.1.0 milestone Mar 26, 2018

mamazu mentioned this pull request Aug 20, 2018

Adding an Address book endpoin in the API #285

Closed

pamil closed this Aug 20, 2018

pamil reopened this Aug 20, 2018

Jurica Šeparović added 5 commits August 20, 2018 15:07

[Address-book] Implement create address-book

9f50150

[Address-book] Removed unused valdiation messagess

3892cac

[Address-book] Changed province name to provinceCode

2481ea6

[Address-book] Add default address field to show action, and remove a…

d16ed25

…ddress endpoint

Removed mistake

b8a6dca

jseparovic1 added 20 commits August 20, 2018 15:10

[Address-book] Remove address request test

e3b109a

[Address-book] Remove show address request test

13930a7

[Address-book] Move user from SetDefaultAddressRequest to correspondi…

1f21f68

…ng handler

[Address-book] apply ecs fixes

8f1aa83

[Address-book] add phpspec

45a1202

[Address-book] add validation labels, and change address constructor …

2e57ef3

…parameters order

[Address-book] returned old path for sql

528c8ad

[Address-book] add final to all phpunit tests

2df80ea

[Address-book] add final classes and empty lines at the end of the file

b95e3f7

[Address-book] Checking province refactored

8bdec06

[Address-book] add return type

151b5df

[Address-book] changed expected address model parameters types

e8d2294

[Address-book] Refactor create address

1a0fc9b

[Address-book] Refactor

9581b43

[Address-book] Removed test db hack

338b28d

[Address-book] Changed error messages

db1390c

[Address-book] add return type

f8bbfc0

change 'address_id to string'

7772868

Fixed phpspec

f7ab829

Apply small fixes and add better validation

b9efa6c

pamil closed this Aug 20, 2018

pamil reopened this Aug 20, 2018

pamil closed this Aug 20, 2018

pamil reopened this Aug 20, 2018

pamil approved these changes Aug 20, 2018

View reviewed changes

pamil merged commit 20a55b1 into Sylius:master Aug 20, 2018

pamil mentioned this pull request Aug 31, 2018

As a Customer, I want to update my profile #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Address book #229

[RFC] Address book #229

jseparovic1 commented Oct 26, 2017 •

edited

Loading

lchrusciel commented Oct 26, 2017

lchrusciel commented Oct 27, 2017

antonioperic commented Oct 27, 2017

lchrusciel commented Oct 31, 2017

jseparovic1 commented Oct 31, 2017

lchrusciel commented Nov 2, 2017

lchrusciel Nov 2, 2017

lchrusciel Nov 2, 2017

jseparovic1 Nov 2, 2017

lchrusciel Nov 2, 2017

jseparovic1 Nov 3, 2017

lchrusciel Nov 2, 2017

lchrusciel Nov 2, 2017

jseparovic1 commented Nov 6, 2017

lchrusciel left a comment

lchrusciel Nov 9, 2017

lchrusciel commented Nov 14, 2017

pjedrzejewski commented Nov 27, 2017

jseparovic1 commented Nov 28, 2017

lchrusciel commented Nov 29, 2017

jseparovic1 commented Nov 29, 2017

lchrusciel commented Feb 14, 2018

jseparovic1 commented Feb 27, 2018

lchrusciel left a comment

lchrusciel Mar 9, 2018

lchrusciel Mar 9, 2018

mamazu commented Aug 16, 2018

pamil commented Aug 20, 2018 •

edited

Loading

pamil commented Aug 20, 2018

mamazu commented Aug 21, 2018 •

edited

Loading

		@@ -0,0 +1,105 @@
		<?php

		declare(strict_types=1);

[RFC] Address book #229

[RFC] Address book #229

Conversation

jseparovic1 commented Oct 26, 2017 • edited Loading

lchrusciel commented Oct 26, 2017

lchrusciel commented Oct 27, 2017

antonioperic commented Oct 27, 2017

lchrusciel commented Oct 31, 2017

jseparovic1 commented Oct 31, 2017

lchrusciel commented Nov 2, 2017

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

jseparovic1 commented Nov 6, 2017

lchrusciel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchrusciel commented Nov 14, 2017

pjedrzejewski commented Nov 27, 2017

jseparovic1 commented Nov 28, 2017

lchrusciel commented Nov 29, 2017

jseparovic1 commented Nov 29, 2017

lchrusciel commented Feb 14, 2018

jseparovic1 commented Feb 27, 2018

lchrusciel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamazu commented Aug 16, 2018

pamil commented Aug 20, 2018 • edited Loading

pamil commented Aug 20, 2018

mamazu commented Aug 21, 2018 • edited Loading

jseparovic1 commented Oct 26, 2017 •

edited

Loading

pamil commented Aug 20, 2018 •

edited

Loading

mamazu commented Aug 21, 2018 •

edited

Loading