Merge branch 'main' into 3341-add-private-and-author-to-shelter-sync

Author: adrian-codecov

api/public/v1/serializers.py

@@ -16,3 +16,7 @@ class Meta:
             "state",
         )
         fields = read_only_fields + ("user_provided_base_sha",)
+
+
+class PullIdSerializer(serializers.Serializer):
+    pullid = serializers.IntegerField()

api/public/v1/tests/views/test_pull_viewset.py

@@ -133,3 +133,10 @@ def test_post_pull_user_provided_base(self, pulls_sync_mock):
         )
         self.assertEqual(response.status_code, 405)
         assert not pulls_sync_mock.called
+
+    def test_get_pull_no_pullid_provided(self):
+        self.client.credentials(HTTP_AUTHORIZATION="Token " + self.repo.upload_token)
+        response = self.client.get("/api/github/codecov/testRepoName/pulls/abc")
+        self.assertEqual(response.status_code, 400)
+        content = json.loads(response.content.decode())
+        self.assertEqual(content["pullid"], ["A valid integer is required."])

api/public/v1/views.py

@@ -1,17 +1,17 @@
 import logging
 
-from django.db.models import OuterRef, Subquery
+from django.db.models import OuterRef, QuerySet, Subquery
 from django.shortcuts import get_object_or_404
 from django_filters.rest_framework import DjangoFilterBackend
 from rest_framework import filters, mixins, viewsets
 
 from api.shared.mixins import RepoPropertyMixin
 from codecov_auth.authentication.repo_auth import RepositoryLegacyTokenAuthentication
-from core.models import Commit
+from core.models import Commit, Pull
 from services.task import TaskService
 
 from .permissions import PullUpdatePermission
-from .serializers import PullSerializer
+from .serializers import PullIdSerializer, PullSerializer
 
 log = logging.getLogger(__name__)
 
@@ -30,8 +30,11 @@ class PullViewSet(
     authentication_classes = [RepositoryLegacyTokenAuthentication]
     permission_classes = [PullUpdatePermission]
 
-    def get_object(self):
-        pullid = self.kwargs.get("pk")
+    def get_object(self) -> Pull:
+        serializer = PullIdSerializer(data={"pullid": self.kwargs.get("pk")})
+        serializer.is_valid(raise_exception=True)
+        pullid = serializer.validated_data["pullid"]
+
         if self.request.method == "PUT":
             # Note: We create a new pull if needed to make sure that they can be updated
             # with a base before the upload has finished processing.
@@ -41,7 +44,7 @@ def get_object(self):
             return obj
         return get_object_or_404(self.get_queryset(), pullid=pullid)
 
-    def get_queryset(self):
+    def get_queryset(self) -> QuerySet:
         return self.repo.pull_requests.annotate(
             ci_passed=Subquery(
                 Commit.objects.filter(
@@ -50,7 +53,7 @@ def get_queryset(self):
             )
         )
 
-    def perform_update(self, serializer):
+    def perform_update(self, serializer: PullSerializer) -> Pull:
         result = super().perform_update(serializer)
         TaskService().pulls_sync(repoid=self.repo.repoid, pullid=self.kwargs.get("pk"))
         return result

api/public/v2/compare/serializers.py

@@ -16,10 +16,11 @@ class ComparisonSerializer(BaseComparisonSerializer):
 
     def get_files(self, comparison: Comparison) -> List[dict]:
         data = []
-        for filename in comparison.head_report.files:
-            file = comparison.get_file_comparison(filename, bypass_max_diff=True)
-            if self._should_include_file(file):
-                data.append(FileComparisonSerializer(file).data)
+        if comparison.head_report is not None:
+            for filename in comparison.head_report.files:
+                file = comparison.get_file_comparison(filename, bypass_max_diff=True)
+                if self._should_include_file(file):
+                    data.append(FileComparisonSerializer(file).data)
         return data
 
 

billing/tests/test_views.py

@@ -1643,6 +1643,132 @@ class MockPaymentMethod:
             "sub_123", default_payment_method=payment_method_retrieve_mock.return_value
         )
 
+    @patch("logging.Logger.error")
+    @patch("services.billing.stripe.PaymentMethod.attach")
+    @patch("services.billing.stripe.Customer.modify")
+    @patch("services.billing.stripe.Subscription.modify")
+    @patch("services.billing.stripe.PaymentMethod.retrieve")
+    def test_check_and_handle_delayed_notification_payment_methods_no_subscription(
+        self,
+        payment_method_retrieve_mock,
+        subscription_modify_mock,
+        customer_modify_mock,
+        payment_method_attach_mock,
+        log_error_mock,
+    ):
+        class MockPaymentMethod:
+            type = "us_bank_account"
+            us_bank_account = {}
+            id = "pm_123"
+
+        payment_method_retrieve_mock.return_value = MockPaymentMethod()
+
+        self.owner.stripe_subscription_id = None
+        self.owner.stripe_customer_id = "cus_123"
+        self.owner.save()
+
+        handler = StripeWebhookHandler()
+        handler._check_and_handle_delayed_notification_payment_methods(
+            "cus_123", "pm_123"
+        )
+
+        payment_method_retrieve_mock.assert_called_once_with("pm_123")
+        payment_method_attach_mock.assert_not_called()
+        customer_modify_mock.assert_not_called()
+        subscription_modify_mock.assert_not_called()
+
+        log_error_mock.assert_called_once_with(
+            "No owners found with that customer_id, something went wrong",
+            extra=dict(customer_id="cus_123"),
+        )
+
+    @patch("logging.Logger.error")
+    @patch("services.billing.stripe.PaymentMethod.attach")
+    @patch("services.billing.stripe.Customer.modify")
+    @patch("services.billing.stripe.Subscription.modify")
+    @patch("services.billing.stripe.PaymentMethod.retrieve")
+    def test_check_and_handle_delayed_notification_payment_methods_no_customer(
+        self,
+        payment_method_retrieve_mock,
+        subscription_modify_mock,
+        customer_modify_mock,
+        payment_method_attach_mock,
+        log_error_mock,
+    ):
+        class MockPaymentMethod:
+            type = "us_bank_account"
+            us_bank_account = {}
+            id = "pm_123"
+
+        payment_method_retrieve_mock.return_value = MockPaymentMethod()
+
+        handler = StripeWebhookHandler()
+        handler._check_and_handle_delayed_notification_payment_methods(
+            "cus_1", "pm_123"
+        )
+
+        payment_method_retrieve_mock.assert_called_once_with("pm_123")
+        payment_method_attach_mock.assert_not_called()
+        customer_modify_mock.assert_not_called()
+        subscription_modify_mock.assert_not_called()
+
+        log_error_mock.assert_called_once_with(
+            "No owners found with that customer_id, something went wrong",
+            extra=dict(customer_id="cus_1"),
+        )
+
+    @patch("services.billing.stripe.PaymentMethod.attach")
+    @patch("services.billing.stripe.Customer.modify")
+    @patch("services.billing.stripe.Subscription.modify")
+    @patch("services.billing.stripe.PaymentMethod.retrieve")
+    def test_check_and_handle_delayed_notification_payment_methods_multiple_subscriptions(
+        self,
+        payment_method_retrieve_mock,
+        subscription_modify_mock,
+        customer_modify_mock,
+        payment_method_attach_mock,
+    ):
+        class MockPaymentMethod:
+            type = "us_bank_account"
+            us_bank_account = {}
+            id = "pm_123"
+
+        payment_method_retrieve_mock.return_value = MockPaymentMethod()
+
+        self.owner.stripe_subscription_id = "sub_123"
+        self.owner.stripe_customer_id = "cus_123"
+        self.owner.save()
+
+        OwnerFactory(stripe_subscription_id="sub_124", stripe_customer_id="cus_123")
+
+        handler = StripeWebhookHandler()
+        handler._check_and_handle_delayed_notification_payment_methods(
+            "cus_123", "pm_123"
+        )
+
+        payment_method_retrieve_mock.assert_called_once_with("pm_123")
+        payment_method_attach_mock.assert_called_once_with(
+            payment_method_retrieve_mock.return_value, customer="cus_123"
+        )
+        customer_modify_mock.assert_called_once_with(
+            "cus_123",
+            invoice_settings={
+                "default_payment_method": payment_method_retrieve_mock.return_value
+            },
+        )
+        subscription_modify_mock.assert_has_calls(
+            [
+                call(
+                    "sub_123",
+                    default_payment_method=payment_method_retrieve_mock.return_value,
+                ),
+                call(
+                    "sub_124",
+                    default_payment_method=payment_method_retrieve_mock.return_value,
+                ),
+            ]
+        )
+
     @patch(
         "billing.views.StripeWebhookHandler._check_and_handle_delayed_notification_payment_methods"
     )

billing/views.py

@@ -539,7 +539,6 @@ def _check_and_handle_delayed_notification_payment_methods(
         When verification succeeds, this attaches the payment method to the customer and sets
         it as the default payment method for both the customer and subscription.
         """
-        owner = Owner.objects.get(stripe_customer_id=customer_id)
         payment_method = stripe.PaymentMethod.retrieve(payment_method_id)
 
         is_us_bank_account = payment_method.type == "us_bank_account" and hasattr(
@@ -548,19 +547,34 @@ def _check_and_handle_delayed_notification_payment_methods(
 
         should_set_as_default = is_us_bank_account
 
+        # attach the payment method + set as default on the invoice and subscription
         if should_set_as_default:
-            # attach the payment method + set as default on the invoice and subscription
-            stripe.PaymentMethod.attach(
-                payment_method, customer=owner.stripe_customer_id
-            )
-            stripe.Customer.modify(
-                owner.stripe_customer_id,
-                invoice_settings={"default_payment_method": payment_method},
-            )
-            stripe.Subscription.modify(
-                owner.stripe_subscription_id, default_payment_method=payment_method
+            # retrieve the number of owners to update
+            owners = Owner.objects.filter(
+                stripe_customer_id=customer_id, stripe_subscription_id__isnull=False
             )
 
+            if owners.exists():
+                # Even if multiple results are returned, these two stripe calls are
+                # just for a single customer
+                stripe.PaymentMethod.attach(payment_method, customer=customer_id)
+                stripe.Customer.modify(
+                    customer_id,
+                    invoice_settings={"default_payment_method": payment_method},
+                )
+
+                # But this one is for each subscription an owner may have
+                for owner in owners:
+                    stripe.Subscription.modify(
+                        owner.stripe_subscription_id,
+                        default_payment_method=payment_method,
+                    )
+            else:
+                log.error(
+                    "No owners found with that customer_id, something went wrong",
+                    extra=dict(customer_id=customer_id),
+                )
+
     def payment_intent_succeeded(self, payment_intent: stripe.PaymentIntent) -> None:
         """
         Stripe payment_intent.succeeded webhook event is emitted when a

graphql_api/types/plan/plan.py

@@ -30,7 +30,7 @@ def resolve_trial_end_date(plan_service: PlanService, info) -> Optional[datetime
 
 @plan_bindable.field("trialStatus")
 def resolve_trial_status(plan_service: PlanService, info) -> TrialStatus:
-    if plan_service.trial_status is None:
+    if not plan_service.trial_status:
         return TrialStatus.NOT_STARTED
     return TrialStatus(plan_service.trial_status)