Extend the functionality #1

justd88 · 2018-10-07T14:39:00Z

If the order is virtual then create the invoice automatically and set the order status to complete
Add frontend comment before redirect to the OTP. ( The admin can leave a note for the users about the redirect etc. )

Add Order status to complete if the order is virtual.

DavidBelicza

It's a good feature. Thanks for the contribution.

Before merge there are things should be changed first:

There are double empty lines, missing empty lines, not well alignments, PSR should be followed
missing dock blocks
If you feel this PR is too big, it could be split to two parts (invoicing and information text on checkout)
I added other comments to the PR inline.

DavidBelicza · 2018-10-08T09:22:21Z

Helper/Config.php

+    /**
+     * @return string
+     */
+    public function getFrontendComment()


Return type declaration is missing from here:

public function getFrontendComment(): string

DavidBelicza · 2018-10-08T09:36:28Z

Model/Order/StatusUpdater.php

+    /**
+     * @param Order $order
+     */
+    private function createInvoice(Order $order)


I think the createInvoice method shouldn't be part of this StatusUpdater. StatusUpdater is responsible only for updating statuses and states of order, this class is already too big.

The order related things should be in Order namespace and Invoice related thing outside of Order namespace, for example: \Youama\OTP\Model\Invoice\StatusUpdater. The invoice updater could be injected to order updater through the constructor.

Fixed, Sorry I was very busy... but now back on track :)

No need to sorry, there are no deadlines :)

DavidBelicza · 2018-10-08T09:41:43Z

etc/adminhtml/system.xml

@@ -87,6 +87,12 @@
                    <label>Shop Comment</label>
                    <comment>Short text about the webshop or order on the OTP payment user interface.</comment>
                </field>
+                <field id="frontend_comment" translate="label" type="textarea" sortOrder="90" showInDefault="1"


This should be defined in config.xml too because of when admin does not click on save button in module settings area after module installation, there will be no default value. Config.xml contains default values.

DavidBelicza · 2018-10-08T09:44:11Z

composer.json

@@ -2,7 +2,7 @@
    "name": "youama/module-otp-2",
    "description": "OTP Payment integration for Magento 2",
    "type": "magento2-module",
-    "version": "1.0.5",
+    "version": "1.0.6",


It could be 1.1.0, because of this is a new feature. I would also increase the setup version to make consistent the module version number to the composer package version number.

DavidBelicza · 2018-10-08T09:47:40Z

Model/Order/StatusUpdater.php

     */
    public function __construct(
        OrderRepositoryInterface $orderRepository,
        OrderFinderInterface $orderFinder,
        OrderStatusHistoryInterfaceFactory $orderStatusHistoryInterfaceFactory,
        OrderCommentSender $orderCommentSender,
-        Config $configHelper
+        Config $configHelper,
+        \Magento\Sales\Model\Service\InvoiceService $invoiceService,


FQN should be used, because of this module use FQN almost everywhere, except those parts where Magento's magic functionalities require the full import path.

Instead of this:

\Magento\Sales\Model\Service\InvoiceService $invoiceService,

Use this:

InvoiceService $invoiceService,

DavidBelicza · 2018-10-08T10:04:54Z

Model/Order/StatusUpdater.php

+
+
+            /** Create Invoice if order was virtual */
+            if ($order->getIsVirtual()) {


There should be a config checking here to check if admin approved the invoicing in the store. Many of shop owners don't want to create invoice inside Magento for different reasons.

DavidBelicza · 2018-10-08T10:05:39Z

Model/Order/StatusUpdater.php

    /**
     * Put order to processing, notify customer about it.
     *
     * @param string $orderIncrementId
-     * @param int    $transactionId


Dockblock param list should be aligned to each other.

justd88 · 2018-10-19T06:34:57Z

Hi David! I made some changes. Can you have a look pls and let me know if anything wrong?
Thank you!

DavidBelicza

I've just found 3 things. I think it can be merged after you modify these.

DavidBelicza · 2018-10-23T08:35:11Z

Model/Invoice/StatusUpdater.php

+    /**
+     * @var InvoiceService
+     */
+    protected $invoiceService;


These fields don't have to be protected, they can be private according to SOLID principles. Same in Order's StatusUpdater.

DavidBelicza · 2018-10-23T08:42:57Z

Model/Invoice/StatusUpdater.php

+
+            $order->addStatusHistory($history);
+            $order->setStatus(Order::STATE_COMPLETE);
+            $this->orderRepository->save($order);


At this point this is a second order saving in the success flow. This method createInvoice has called at the end of the Order's StatusUpdater::success where the order already has been saved.

Is there any reason why is get two saves? If order save is required after invoice saving then I think this createInvoice could be easily before the order saving.

That is a good point. shall I pass the order as a pointer?

I think it's not necessary, the $order already points back because objects are reference types.

DavidBelicza · 2018-10-23T08:45:22Z

Model/Invoice/StatusUpdater.php

+/**
+ * Youama_OTP
+ *
+ * @author  David Belicza <[email protected]>


In any file what you created, you can add your own name, real one or nickname and your own email address if you want, instead of mine because you are the author of those files. :) (The other parts like license should be the same as it was before.)

You can append your name to the other files what you just modified, it is valid in PHPDoc:

@author David Belicza <[email protected]> @author Other Author <[email protected]>

im-dnsosso added 4 commits October 7, 2018 15:16

Add Gitignore

8bda132

Add Create invoice if the order is Virtual.

e3faa85

Add Order status to complete if the order is virtual.

Add Storefront comment before the redirect

63df63f

Increase Version Number

db7b2c1

DavidBelicza self-requested a review October 8, 2018 09:18

DavidBelicza requested changes Oct 8, 2018

View reviewed changes

DavidBelicza added the fix required label Oct 8, 2018

im-dnsosso added 10 commits October 12, 2018 08:18

Fix return type declaration

817f1f2

Add System config option for auto invoicing

6b71837

Add Required dep. and update the version number

743aa83

Refactor invoice creation

7a03f67

Fix typo

4faf5a1

Add vendor dir for .gitignore

57b92b8

Change the order of the fields in admin

2aa2796

Fix update the setup version number

b7c5bfa

Increase setup version number

574ef2a

Fix: frontend comment not displaying

a785c1c

upgrade setup version

95a7035

DavidBelicza requested changes Oct 23, 2018

View reviewed changes

im-dnsosso added 3 commits October 31, 2018 14:37

Fix: SOLID principles

86d642f

Update Authors

764a94a

Fix: language can be set on store view level

5c43d06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the functionality #1

Extend the functionality #1

justd88 commented Oct 7, 2018

DavidBelicza left a comment

DavidBelicza Oct 8, 2018

DavidBelicza Oct 8, 2018

justd88 Oct 31, 2018

DavidBelicza Nov 4, 2018

DavidBelicza Oct 8, 2018

DavidBelicza Oct 8, 2018

DavidBelicza Oct 8, 2018

DavidBelicza Oct 8, 2018

DavidBelicza Oct 8, 2018

justd88 commented Oct 19, 2018

DavidBelicza left a comment

DavidBelicza Oct 23, 2018

justd88 Oct 31, 2018

DavidBelicza Oct 23, 2018

justd88 Oct 31, 2018

DavidBelicza Nov 4, 2018

DavidBelicza Oct 23, 2018



		/** Create Invoice if order was virtual */
		if ($order->getIsVirtual()) {

Extend the functionality #1

Are you sure you want to change the base?

Extend the functionality #1

Conversation

justd88 commented Oct 7, 2018

DavidBelicza 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

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

justd88 commented Oct 19, 2018

DavidBelicza 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment