[Admin][Payment] Sorting payment metods by position #6745

tuka217 · 2016-11-16T11:55:14Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Related tickets	-
License	MIT

GSadee · 2016-11-16T14:14:46Z

features/payment/managing_payment_methods/sorting_payment_methods_by_position.feature

+
+    @ui
+    Scenario: Payment methods are sorted by position in ascending order by default
+        When I am browsing payment methods


When I browse payment methods
or
Given I am browsing payment methods
in my opinion

GSadee · 2016-11-16T14:15:53Z

features/payment/managing_payment_methods/sorting_payment_methods_by_position.feature

+
+    @ui
+    Scenario: Payment method added at no position is added as the last one
+        Given the store also allows paying with "Credit Card"


I guess that also is not needed

GSadee · 2016-11-16T14:16:44Z

src/Sylius/Behat/Context/Setup/PaymentContext.php

-     * @Given the store allows paying :paymentMethodName
-     * @Given the store allows paying with :paymentMethodName
+     * @Given the store (also) allows paying :paymentMethodName
+     * @Given the store (also) allows paying with :paymentMethodName


You could merge this line with above by adding (with)

Is the variation without "with" correct?

Probably not, but it is old step. I can fix it here if it won't cause to many problems.

@pamil While it doesn't sound right for credit card or paypal; for offline (which we use quite a lot) using with would be silly

GSadee · 2016-11-16T14:20:37Z

src/Sylius/Component/Payment/spec/Model/PaymentMethodSpec.php

@@ -16,6 +16,8 @@
 use Sylius\Component\Payment\Model\PaymentMethodInterface;

 /**
+ * @mixin PaymentMethod


We should not add @mixin docbloc

Arminek · 2016-11-16T14:37:07Z

src/Sylius/Bundle/PaymentBundle/Form/Type/PaymentMethodType.php

+            ->add('position', IntegerType::class, [
+                'required' => false,
+                'label' => 'sylius.form.shipping_method.position',
+            ])
            ->add('enabled', 'checkbox', [


Imo checkbox should be changed into CheckboxType::class.

Not a part of this PR, I've already done that in #6738.

pamil · 2016-11-16T15:21:19Z

features/payment/managing_payment_methods/sorting_payment_methods_by_position.feature

+        And I am logged in as an administrator
+
+    @ui
+    Scenario: Payment methods are sorted by position in ascending order by default


Actually it does not assert that methods are sorted by position, because they can be as well sorted by id and this scenario would pass. We need to shuffle the order of creating payment methods.

pamil · 2016-11-16T15:23:59Z

src/Sylius/Behat/Context/Setup/PaymentContext.php

-     * @Given the store allows paying :paymentMethodName
-     * @Given the store allows paying with :paymentMethodName
+     * @Given the store (also) allows paying :paymentMethodName
+     * @Given the store (also) allows paying with :paymentMethodName


Is the variation without "with" correct?

pamil · 2016-11-16T15:25:05Z

src/Sylius/Bundle/PaymentBundle/Form/Type/PaymentMethodType.php

+            ->add('position', IntegerType::class, [
+                'required' => false,
+                'label' => 'sylius.form.shipping_method.position',
+            ])
            ->add('enabled', 'checkbox', [


Not a part of this PR, I've already done that in #6738.

pamil · 2016-11-16T15:25:46Z

src/Sylius/Bundle/PaymentBundle/spec/Form/Type/PaymentMethodTypeSpec.php

-
+    function it_adds_add_code_event_subscriber(FormBuilder $builder) 
+    {
+        $builder->add(Argument::any(), Argument::cetera())->willReturn($builder);


It does not check anything, deleting the whole spec would be the better option 🗡

💯 👍 for deleting

If you say, that I can kill it then I will 🔫

Zales0123 · 2016-11-17T08:49:16Z

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

+        );
+    }
+
+


Redundant blank line.

lchrusciel · 2016-11-17T09:16:59Z

src/Sylius/Behat/Context/Setup/PaymentContext.php

@@ -83,20 +83,20 @@ public function __construct(
    }

    /**
-     * @Given the store allows paying :paymentMethodName
-     * @Given the store allows paying with :paymentMethodName
+     * @Given the store (also) allows paying (with) :paymentMethodName


After this change few steps could be broken. It is because also keyword is optional but spaces around it are not. So only two valid sentences are store (two spaces here) allows and store also allows, but store allows will be broken. (same for with)

Or I can be wrong and it will work. Nvm 💨

If I am right, when the definition of step in docblock is not a regexp, behat is quite clever and interprets this correctly.

My bad! Sorry ;)

That's not true, Turnip will allow for the following patterns:

store allows store also allows

Unfortunately, it will also allow for:

store allows storeallows storealso allows store alsoallows storealsoallows

See https://github.com/Behat/Behat/blob/v3.2.2/src/Behat/Behat/Definition/Pattern/Policy/TurnipPatternPolicy.php#L161

So meaningful regexp :D

Nevertheless, at the end we should have store (also )allows

I knew, there were a reason for this change 🎱

…r implementation [Behat] Add scenario about viewing sorted payment method during checkout and it's implementation

[AdminBundle] Add sorting by position, allow admin to declare payment method position

pjedrzejewski · 2016-11-23T08:18:21Z

Thank you Ania, nice work! :)

…sition [Admin][Payment] Sorting payment metods by position

pjedrzejewski added the New Feature label Nov 16, 2016

tuka217 force-pushed the sort-payment-method-by-position branch 3 times, most recently from 8a1ad30 to 464b9d4 Compare November 16, 2016 14:20

GSadee requested changes Nov 16, 2016

View reviewed changes

tuka217 force-pushed the sort-payment-method-by-position branch from 464b9d4 to cd078dc Compare November 16, 2016 14:23

Arminek reviewed Nov 16, 2016

View reviewed changes

pamil suggested changes Nov 16, 2016

View reviewed changes

tuka217 force-pushed the sort-payment-method-by-position branch 2 times, most recently from d1f0a04 to 248d3c4 Compare November 17, 2016 08:51

Zales0123 reviewed Nov 17, 2016

View reviewed changes

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

);

}

Copy link

Member

Zales0123 Nov 17, 2016

Choose a reason for hiding this comment

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

Redundant blank line.

pamil approved these changes Nov 17, 2016

View reviewed changes

lchrusciel requested changes Nov 17, 2016

View reviewed changes

tuka217 force-pushed the sort-payment-method-by-position branch 5 times, most recently from 99d189b to 0e022c6 Compare November 21, 2016 14:08

tuka217 added 5 commits November 22, 2016 09:59

[Behat] Add scenarios for sorting payment metods by position and thei…

c492802

…r implementation [Behat] Add scenario about viewing sorted payment method during checkout and it's implementation

[PaymentBundle][Payment] Add position field to payment method

2388e66

[AdminBundle] Add sorting by position, allow admin to declare payment method position

[Documentation] Add info about position to paymentMethod's description

27d54dc

Migration

a10d558

Fix after review

a067654

tuka217 force-pushed the sort-payment-method-by-position branch from 0e022c6 to a067654 Compare November 22, 2016 09:00

pjedrzejewski merged commit a6d0231 into Sylius:master Nov 23, 2016

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

Merge pull request Sylius#6745 from tuka217/sort-payment-method-by-po…

427e7f4

…sition [Admin][Payment] Sorting payment metods by position

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

Merge pull request Sylius#6745 from tuka217/sort-payment-method-by-po…

9bbd4d1

…sition [Admin][Payment] Sorting payment metods by position

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Admin][Payment] Sorting payment metods by position #6745

[Admin][Payment] Sorting payment metods by position #6745

tuka217 commented Nov 16, 2016

GSadee Nov 16, 2016

GSadee Nov 16, 2016

GSadee Nov 16, 2016

pamil Nov 16, 2016

tuka217 Nov 17, 2016

NoResponseMate Nov 17, 2016

tuka217 Nov 17, 2016

GSadee Nov 16, 2016

Arminek Nov 16, 2016

pamil Nov 16, 2016

pamil Nov 16, 2016

pamil Nov 16, 2016

pamil Nov 16, 2016

pamil Nov 16, 2016

NoResponseMate Nov 17, 2016

tuka217 Nov 17, 2016

Zales0123 Nov 17, 2016

lchrusciel Nov 17, 2016 •

edited

Loading

lchrusciel Nov 17, 2016

GSadee Nov 17, 2016

lchrusciel Nov 17, 2016

pamil Nov 17, 2016

lchrusciel Nov 17, 2016

pjedrzejewski commented Nov 23, 2016

[Admin][Payment] Sorting payment metods by position #6745

[Admin][Payment] Sorting payment metods by position #6745

Conversation

tuka217 commented Nov 16, 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

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

Choose a reason for hiding this comment

lchrusciel Nov 17, 2016 • edited Loading

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

pjedrzejewski commented Nov 23, 2016

lchrusciel Nov 17, 2016 •

edited

Loading