Updated refund processing methods

- Attempts to approve/deny previously-approved/denied refunds return True
- Attempts to approve/deny refunds in an incorrect state are logged as warnings instead of debug
- Removed unnecessary mocking in test_success_approve_payment_only

ECOM-6975

ecommerce/extensions/api/v2/tests/views/test_refunds.py

 import json
 
 import ddt
-from django.core.urlresolvers import reverse
 import httpretty
 import mock
+from django.core.urlresolvers import reverse
 from oscar.core.loading import get_model
 from oscar.test import factories
 from rest_framework import status
@@ -11,7 +11,7 @@ from rest_framework import status
 from ecommerce.extensions.api.serializers import RefundSerializer
 from ecommerce.extensions.api.tests.test_authentication import AccessTokenMixin
 from ecommerce.extensions.api.v2.tests.views import JSON_CONTENT_TYPE
-from ecommerce.extensions.refund.status import REFUND_LINE
+from ecommerce.extensions.refund.status import REFUND
 from ecommerce.extensions.refund.tests.factories import RefundLineFactory, RefundFactory
 from ecommerce.extensions.refund.tests.mixins import RefundTestMixin
 from ecommerce.tests.mixins import JwtMixin, ThrottlingMixin
@@ -211,25 +211,21 @@ class RefundProcessViewTests(ThrottlingMixin, TestCase):
             self.assertEqual(response.status_code, 200)
             self.assertEqual(response.data, RefundSerializer(self.refund).data)
 
+    @mock.patch('ecommerce.extensions.refund.models.Refund._revoke_lines')
     @mock.patch('ecommerce.extensions.refund.models.Refund._issue_credit')
-    def test_success_approve_payment_only(self, mock_issue_credit):
-        """ If the action succeeds, the view should return HTTP 200 and the serialized Refund. """
-        def mock_fulfillment(refund_obj):
-            for refund_line in refund_obj.lines.all():
-                refund_line.set_status(REFUND_LINE.COMPLETE)
-
+    def test_success_approve_payment_only(self, mock_issue_credit, mock_revoke_lines):
+        """ Verify the endpoint supports approving the refund, and issuing credit without revoking fulfillment. """
         mock_issue_credit.return_value = None
 
-        with mock.patch("ecommerce.extensions.fulfillment.api.revoke_fulfillment_for_refund",
-                        side_effect=mock_fulfillment):
-            with mock.patch("ecommerce.extensions.refund.models.logger") as patched_log:
-                response = self.put('approve_payment_only')
-                self.assertEqual(response.status_code, 200)
-                refund = Refund.objects.get(id=self.refund.id)
-                self.assertEqual(response.data['status'], refund.status)
-                self.assertEqual(response.data['status'], "Complete")
-                patched_log.info.assert_called_with(
-                    "Skipping the revocation step for refund [%d].", self.refund.id)
+        with mock.patch('ecommerce.extensions.refund.models.logger') as patched_log:
+            response = self.put('approve_payment_only')
+            self.assertEqual(response.status_code, 200)
+
+        self.refund.refresh_from_db()
+        self.assertEqual(response.data['status'], self.refund.status)
+        self.assertEqual(response.data['status'], 'Complete')
+        patched_log.info.assert_called_with('Skipping the revocation step for refund [%d].', self.refund.id)
+        self.assertFalse(mock_revoke_lines.called)
 
     @ddt.data(
         ('approve', 'approve'),
@@ -243,3 +239,16 @@ class RefundProcessViewTests(ThrottlingMixin, TestCase):
             response = self.put(decision)
             self.assertEqual(response.status_code, 500)
             self.assertEqual(response.data, RefundSerializer(self.refund).data)
+
+    @ddt.data(
+        ('approve', REFUND.COMPLETE),
+        ('approve_payment_only', REFUND.COMPLETE),
+        ('deny', REFUND.DENIED)
+    )
+    @ddt.unpack
+    def test_subsequent_approval(self, action, _status):
+        """ Verify the endpoint supports reprocessing a previously-processed refund. """
+        self.refund.status = _status
+        self.refund.save()
+        response = self.put(action)
+        self.assertEqual(response.status_code, 200)

ecommerce/extensions/refund/models.py

@@ -233,8 +233,11 @@ class Refund(StatusMixin, TimeStampedModel):
             self.set_status(REFUND.REVOCATION_ERROR)
 
     def approve(self, revoke_fulfillment=True, notify_purchaser=True):
-        if not self.can_approve:
-            logger.debug('Refund [%d] cannot be approved.', self.id)
+        if self.status == REFUND.COMPLETE:
+            logger.info('Refund [%d] has already been completed. No additional action is required to approve.', self.id)
+            return True
+        elif not self.can_approve:
+            logger.warning('Refund [%d] has status set to [%s] and cannot be approved.', self.id, self.status)
             return False
         elif self.status in (REFUND.OPEN, REFUND.PAYMENT_REFUND_ERROR):
             try:
@@ -251,7 +254,7 @@ class Refund(StatusMixin, TimeStampedModel):
             self._revoke_lines()
 
         if not revoke_fulfillment and self.status == REFUND.PAYMENT_REFUNDED:
-            logger.info("Skipping the revocation step for refund [%d].", self.id)
+            logger.info('Skipping the revocation step for refund [%d].', self.id)
             # Mark the status complete as it does not involve the revocation.
             self.set_status(REFUND.COMPLETE)
             for refund_line in self.lines.all():
@@ -264,8 +267,12 @@ class Refund(StatusMixin, TimeStampedModel):
         return False
 
     def deny(self):
+        if self.status == REFUND.DENIED:
+            logger.info('Refund [%d] has already been denied. No additional action is required to deny.', self.id)
+            return True
+
         if not self.can_deny:
-            logger.debug('Refund [%d] cannot be denied.', self.id)
+            logger.warning('Refund [%d] cannot be denied. Its status is [%s].', self.id, self.status)
             return False
 
         self.set_status(REFUND.DENIED)

ecommerce/extensions/refund/tests/test_models.py

@@ -264,6 +264,11 @@ class RefundTests(RefundTestMixin, StatusTestsMixin, TestCase):
         # Verify an attempt is made to send a notification
         mock_notify.assert_called_once_with()
 
+        # Verify subsequent calls to approve an approved refund do not change the state
+        refund.refresh_from_db()
+        self.assertTrue(refund.approve())
+        self.assertEqual(refund.status, REFUND.COMPLETE)
+
     def test_approve_payment_error(self):
         """
         If payment refund fails, the Refund status should be set to Payment Refund Error, and the RefundLine
@@ -296,9 +301,9 @@ class RefundTests(RefundTestMixin, StatusTestsMixin, TestCase):
                 self.assertEqual(refund.status, REFUND.REVOCATION_ERROR)
                 self.assert_line_status(refund, REFUND_LINE.REVOCATION_ERROR)
 
-    @ddt.data(REFUND.COMPLETE, REFUND.DENIED)
-    def test_approve_wrong_state(self, status):
+    def test_approve_wrong_state(self):
         """ The method should return False if the Refund cannot be approved. """
+        status = REFUND.DENIED
         refund = self._get_instance(status=status)
         self.assertEqual(refund.status, status)
         self.assert_line_status(refund, REFUND_LINE.OPEN)
@@ -320,6 +325,11 @@ class RefundTests(RefundTestMixin, StatusTestsMixin, TestCase):
         self.assertEqual(refund.status, REFUND.DENIED)
         self.assert_line_status(refund, REFUND_LINE.DENIED)
 
+        # Verify subsequent calls to approve an approved refund do not change the state
+        refund.refresh_from_db()
+        self.assertTrue(refund.deny())
+        self.assertEqual(refund.status, REFUND.DENIED)
+
     def test_deny_with_exception(self):
         """
         If denial of a line results in an exception being raised, the exception should be logged, and the method

package.json

@@ -9,7 +9,7 @@
     "requirejs": "^2.1.18"
   },
   "devDependencies": {
-    "geckodriver": "^1.1.3",
+    "geckodriver": "^1.3.0",
     "gulp": "^3.9.0",
     "gulp-jscs": "3.0.0",
     "gulp-jshint": "1.12.0",