Feature/421 43 refund to store credit #55

rsisco · 2017-03-15T16:37:03Z

Add code to create two new AvaTax DB tables
Add code to populate tables on queue processing
Remove code that is now unused
Add code to migrate data from old columns to new tables
Refactor code to load data from new tables into ext attributes
Add code to remove unused AvaTax columns from core tables
Refactor code so new install will create new tables instead of adding
columns to core tables

- Add code to create two new AvaTax DB tables - Add code to populate tables on queue processing

- Remove code that is now unused - Add code to migrate data from old columns to new tables - Refactor code to load data from new tables into ext attributes

- Add doc blocks - Revise code to remove unused methods - Add upgrade data script

- Remove code for Refund plugin - Add code to remove unused AvaTax columns from core tables - Refactor code so new install will create new tables instead of adding columns to core tables

mttjohnson · 2017-04-12T23:09:52Z

Model/ResourceModel/CreditMemo.php

+namespace ClassyLlama\AvaTax\Model\ResourceModel;
+
+class CreditMemo extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb
+{


It appears that the database field names are used in a few places, and I would suggest setting a constant for each of the field names on the resource and reference those instead of having several strings typed out in the code directly representing the field names. This will allow for more strongly typed code, and the ability to find any references where those field names are used throughout the code. It also aids in any event that the field names need to be changed in the future as they are only typed out in one location.

You can find an example of this to this on \ClassyLlama\AvaTax\Model\ResourceModel\Queue

/**#@+ * Field Names */ const QUEUE_STATUS_FIELD_NAME = 'queue_status'; const UPDATED_AT_FIELD_NAME = 'updated_at'; const CREATED_AT_FIELD_NAME = 'created_at'; /**#@-*/

mttjohnson · 2017-04-12T23:11:25Z

Model/ResourceModel/Invoice.php

+
+class Invoice extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb
+{
+    protected function _construct()


Database field names could be defined as constants and referenced instead of statically typed strings throughout the code.

An example of this to this on: \ClassyLlama\AvaTax\Model\ResourceModel\Queue

mttjohnson · 2017-04-12T23:16:34Z

Model/CreditMemo.php

+
+namespace ClassyLlama\AvaTax\Model;
+
+class CreditMemo


Where magic getters and setters are used on your model you should include references to them in the DocBlock section of the class definition so that the IDE knows about them and can help catch any typo/validation issues. It also allows the IDE to provide auto complete support when using the class.

Example \ClassyLlama\AvaTax\Model\Queue:

/** * Queue * * @method string getCreatedAt() getCreatedAt() * @method string getUpdatedAt() getUpdatedAt() * @method int getStoreId() getStoreId()

mttjohnson · 2017-04-12T23:17:11Z

Model/Invoice.php

+ */
+
+namespace ClassyLlama\AvaTax\Model;
+


Where magic getters and setters are used on your model you should include references to them in the DocBlock section of the class definition so that the IDE knows about them and can help catch any typo/validation issues. It also allows the IDE to provide auto complete support when using the class.

Example \ClassyLlama\AvaTax\Model\Queue:

/** * Queue * * @method string getCreatedAt() getCreatedAt() * @method string getUpdatedAt() getUpdatedAt() * @method int getStoreId() getStoreId()

mttjohnson · 2017-04-12T23:36:53Z

Plugin/Sales/Model/Spi/CreditmemoResource.php

-            $baseAvataxTaxAmount = $entity->getData('base_avatax_tax_amount');
+            // Get the AvaTax record
+            /** @var CreditMemo $avataxRecord */
+            $avataxRecord = $this->avataxCreditMemo->loadByParentId($entity->getId());


I think you can call load() on the model with an additional parameter to specify the field you want to load by, and thus eliminate the loadByParentId() methods on both your model and resourceModel classes.

$this->avataxCreditMemo->load($entity->getId(), AvaTaxCreditMemoResource::PARENT_ID_FIELD_NAME);

Reference: \Magento\Framework\Model\ResourceModel\Db\AbstractDb::load

mttjohnson · 2017-04-12T23:38:15Z

Plugin/Sales/Model/Spi/InvoiceResource.php

-            $baseAvataxTaxAmount = $entity->getData('base_avatax_tax_amount');
+            // Get the AvaTax record
+            /** @var Invoice $invoice */
+            $avataxRecord = $this->avataxInvoice->loadByParentId($entity->getId());


Eliminate the loadByParentId() method on your model and resourceModel by using the Abstract load() method with a field name parameter as indicated on the CreditMemo comment.

Reference: \Magento\Framework\Model\ResourceModel\Db\AbstractDb::load

mttjohnson · 2017-04-12T23:43:37Z

We should review the use of extension attributes for Magento\Sales\Api\Data\CreditmemoInterface in etc/extension_attributes.xml

I'm wondering how this works when used with a SearchResult on a list of multiple CreditMemo results. I'm not sure we have an existing use case for this, but it is something to be aware of at least.

There is some support for extension attributes that utilize a reference table (reference_table, reference_field, join_on_field) http://devdocs.magento.com/guides/v2.0/extension-dev-guide/attributes.html.

mttjohnson · 2017-04-12T23:58:50Z

Plugin/Sales/Model/Spi/CreditmemoResource.php

+            /** @var CreditMemo $avataxRecord */
+            $avataxRecord = $this->avataxCreditMemo->loadByParentId($entity->getId());
+            $avataxIsUnbalanced = $avataxRecord->getData('is_unbalanced');
+            $baseAvataxTaxAmount = $avataxRecord->getData('base_avatax_tax_amount');


If the magic getters and setters are defined on the class you shouldn't need to make calls to getData() with the direct field name. You could just call $avataxRecord->getBaseAvataxTaxAmount()

mttjohnson · 2017-04-13T00:31:00Z

Model/Queue/Processing.php

+    {
+        $avaTaxRecord = $this->createAvataxEntity($entity);
+        // Load existing AvaTax entry for this entity, if exists
+        $avaTaxRecord->loadByParentId($entity->getId());


Another place where the loadByParentId() method could be eliminated in favor of load($entity->getId(), 'parent_id')`

mttjohnson · 2017-04-13T00:32:39Z

Model/Queue/Processing.php

-                    ]
-                );
-                $entityExtension->setAvataxIsUnbalanced($processSalesResponse->getIsUnbalanced());
+        if ($avaTaxRecord->getData('parent_id')) {


If the magic getters are specified in the docblock on the class you could call this as $avaTaxRecord->getParentId()

- Revise code to utilize magic getters and setters - Revise code to utilize constants for column names - Remove unnecessary methods

Removed backslash in fully qualified use references

mttjohnson

I committed a minor refactor to Processing.php and corrected a couple fully qualified use statements.

If you can run a quick test with the latest commit checked out to confirm that it still loads an already existing record to update when saving the AvataxRecord, this will be good to merge.

mttjohnson · 2017-04-13T20:04:05Z

You will probably also want to resolve branch conflicts before your final test.

…und-to-store-credit # Conflicts: # CHANGELOG.md

- Revise version number in appropriate locations

…instead of “NULL” values

…ith “0” instead of “NULL” values" - this change is actually not needed, since each invoice should always record values, even if it’s not imbalanced This reverts commit 8fe14e8.

erikhansen

Overall, this looks great. I tested locally and confirmed that the following works:

Upgrading existing install moves data into new tables
New invoices created get appropriately logged to the new tables

erikhansen · 2017-04-17T17:32:23Z

Model/Queue/Processing.php

-        if ($entityExtension == null) {
-            $entityExtension = $this->getEntityExtensionInterface($entity);
-        }
+    )


The parenthesis and curly brace should be on the same line in this situation:

) {

erikhansen · 2017-04-17T18:15:45Z

Setup/UpgradeData.php

+                            'avatax_is_unbalanced',
+                            'base_avatax_tax_amount'
+                        ])
+                    ->where('(base_avatax_tax_amount IS NOT NULL AND base_avatax_tax_amount <> 0) OR (avatax_is_unbalanced IS NOT NULL AND avatax_is_unbalanced <> 0)');


I tested this locally and found that the avatax_is_unbalanced and base_avatax_tax_amount columns were sometimes "0" instead of "NULL":

As a result of this, the entries in the avatax_sales_invoice and avatax_sales_creditmemo tables were getting entries inserted into it like this:

In order to prevent this, I changed this conditional in this commit:

8fe14e8

I tested this change and now only the correct values are getting inserted:

Actually, never mind. I reverted that commit. I forgot that those columns got populated regardless of whether the invoice/credit memo was unbalanced.

erikhansen · 2017-04-17T20:35:00Z

CHANGELOG.md

+* Add code to create new database tables dedicated to storing AvaTax data
+* Add code to migrate existing data from AvaTax columns on sales_invoice and sales_creditmemo tables to new tables
+* Refactor code to store AvaTax data in new tables instead of attaching to entities
+* Previous versions of this extension added two fields to the native Magento invoice and credit memo tables. When this extension


I added this to the readme to provide more information about this release

erikhansen and others added 10 commits June 30, 2016 21:58

Merge branch 'release/0.3.0'

efaf541

Merge branch 'release/0.3.1'

0f86a78

Merge branch 'release/0.3.2'

7fdf689

Merge branch 'release/0.3.3'

a4c97f5

Merge branch 'release/0.3.4'

1ab4a2e

#421_43 - Refund to store credit

62d9dae

- Add code to create two new AvaTax DB tables - Add code to populate tables on queue processing

#421_43 - Refund to store credit credit

6c46ab5

- Remove code that is now unused - Add code to migrate data from old columns to new tables - Refactor code to load data from new tables into ext attributes

#421_43 - Refund to store credit

ebd6f98

- Add doc blocks - Revise code to remove unused methods - Add upgrade data script

#421_43 - Update changelog

73c2670

#421_43 - Refund to store credit

7379e58

- Remove code for Refund plugin - Add code to remove unused AvaTax columns from core tables - Refactor code so new install will create new tables instead of adding columns to core tables

rsisco requested a review from erikhansen March 15, 2017 16:37

mttjohnson reviewed Apr 12, 2017

View reviewed changes

mttjohnson reviewed Apr 13, 2017

View reviewed changes

Richard Sisco and others added 3 commits April 13, 2017 10:45

#421_43 - Refund to store credit

45f6d88

- Revise code to utilize magic getters and setters - Revise code to utilize constants for column names - Remove unnecessary methods

#421_43 - Correct doc block

64204b1

Refactored call to load AvataxRecord utilizing constant for field name

e43df7e

Removed backslash in fully qualified use references

mttjohnson approved these changes Apr 13, 2017

View reviewed changes

Richard Sisco and others added 4 commits April 13, 2017 16:13

Merge remote-tracking branch 'origin/develop' into feature/421_43-ref…

f65df5a

…und-to-store-credit # Conflicts: # CHANGELOG.md

#421_43 - Refund to store credit

858d76e

- Revise version number in appropriate locations

#421_43 - Change upgrade data script to account for columns with “0” …

8fe14e8

…instead of “NULL” values

Revert "#421_43 - Change upgrade data script to account for columns w…

411619b

…ith “0” instead of “NULL” values" - this change is actually not needed, since each invoice should always record values, even if it’s not imbalanced This reverts commit 8fe14e8.

Add clarification to the readme

ca3b691

erikhansen approved these changes Apr 17, 2017

View reviewed changes

erikhansen merged commit 4e77d0b into develop Apr 17, 2017

erikhansen mentioned this pull request Apr 17, 2017

Error when refunding to store credit #47

Closed

LordZardeck deleted the feature/421_43-refund-to-store-credit branch September 12, 2018 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/421 43 refund to store credit #55

Feature/421 43 refund to store credit #55

rsisco commented Mar 15, 2017

mttjohnson Apr 12, 2017

mttjohnson Apr 12, 2017

mttjohnson Apr 12, 2017

mttjohnson Apr 12, 2017

mttjohnson Apr 12, 2017

mttjohnson Apr 12, 2017

mttjohnson commented Apr 12, 2017 •

edited

Loading

mttjohnson Apr 12, 2017 •

edited

Loading

mttjohnson Apr 13, 2017

mttjohnson Apr 13, 2017

mttjohnson left a comment

mttjohnson commented Apr 13, 2017

erikhansen left a comment

erikhansen Apr 17, 2017

erikhansen Apr 17, 2017

erikhansen Apr 17, 2017

erikhansen Apr 17, 2017

		*/

		namespace ClassyLlama\AvaTax\Model;

Feature/421 43 refund to store credit #55

Feature/421 43 refund to store credit #55

Conversation

rsisco commented Mar 15, 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

mttjohnson commented Apr 12, 2017 • edited Loading

mttjohnson Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mttjohnson left a comment

Choose a reason for hiding this comment

mttjohnson commented Apr 13, 2017

erikhansen 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

mttjohnson commented Apr 12, 2017 •

edited

Loading

mttjohnson Apr 12, 2017 •

edited

Loading