refactor: remove redundant free-course check in create_payment_intent

[ankushchk]

Related issues

No existing issue this is a proactive code quality fix discovered during code review.

Description

The create_payment_intent view in web/views.py contained two sequential checks for free/zero-priced courses that were redundant, the second block was unreachable dead code.

The problem

The function originally had two back-to-back guards:

# Block 1: Catches price == 0 and returns early
if course.price == 0:
    # ... approval logic ...
    return JsonResponse({"free_course": True, ...})

# ... some intermediate code ...

# Block 2: UNREACHABLE
if course.price <= 0:
    # ... duplicate approval logic ...
    return JsonResponse({"free_course": True, ...})

Why the second block was dead code:

• If course.price == 0, the first block returns immediately.
• After that, execution only continues when price != 0.
• At that point price <= 0 can only be True if price is negative which is prevented by standard model validation.
• Even if negative prices existed, the original == 0 check wouldn't have caught them, meaning the logic was split unnecessarily.

The fix

The PR merges these into a single, more robust guard:

# Merged guard: handles both zero AND any unexpected negative prices
if course.price <= 0:
    # Find the enrollment and update its status
    enrollment = get_object_or_404(Enrollment, student=request.user, course=course)
    if enrollment.status == "pending":
        enrollment.status = "approved"
        enrollment.save()
        # ... notifications ...
    return JsonResponse({"free_course": True, "message": "Enrollment approved for free course"})

1. Changed the first check from == 0 to <= 0 now also catches any unexpected negative prices (more defensive).
2. Removed the second, unreachable if course.price <= 0 block entirely (11 lines of dead code).
3. No behavioral change the logic remains identical for valid inputs while cleaning up the codebase.

Checklist

• Did you run the pre-commit?
• Did you test the change?
• [/] Added screenshots to the PR description (if applicable) - not applicable, backend-only change

Summary by CodeRabbit

• Bug Fixes
  • Fixed enrollment handling for free and negative-priced courses to prevent unnecessary payment processing interactions and streamline the approval workflow.

[github-actions]

👀 Peer Review Required

Hi @ankushchk! This pull request does not yet have a peer review.

Before this PR can be merged, please request a review from one of your peers:

• Go to the PR page and click "Reviewers" on the right sidebar.
• Select a team member or contributor to review your changes.
• Once they approve, this reminder will be automatically removed.

Thank you for contributing! 🎉

[coderabbitai]

Walkthrough

The create_payment_intent function in web/views.py was modified to use a more inclusive guard condition (price <= 0 instead of price == 0) to handle free or negative-priced courses. A redundant enrollment approval block that duplicated this logic was removed, consolidating the flow to rely on the single condition check before Stripe PaymentIntent creation.

Changes

Cohort / File(s)	Summary
Payment Intent Logic web/views.py	Broadened free-course guard from price == 0 to price <= 0 and removed redundant enrollment approval block, streamlining payment intent creation flow.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: removing redundant free-course check logic in create_payment_intent, which aligns with the PR's consolidation of duplicate guards into a single check.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

web/views.py (1)

1608-1616: ⚠️ Potential issue | 🟠 Major

Make free-course approval idempotent before sending notifications.

At Line 1609, concurrent/retried requests can both observe pending and both trigger Line 1614–Line 1615, causing duplicate side effects. Gate notifications on an atomic status transition.

🔧 Proposed fix

         enrollment = get_object_or_404(Enrollment, student=request.user, course=course)
-        if enrollment.status == "pending":
-            enrollment.status = "approved"
-            enrollment.save()
-
-            # Send notifications
-            send_enrollment_confirmation(enrollment)
-            notify_teacher_new_enrollment(enrollment)
+        with transaction.atomic():
+            status_updated = Enrollment.objects.filter(id=enrollment.id, status="pending").update(status="approved")
+
+        if status_updated:
+            enrollment.status = "approved"
+            transaction.on_commit(lambda: send_enrollment_confirmation(enrollment))
+            transaction.on_commit(lambda: notify_teacher_new_enrollment(enrollment))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@web/views.py` around lines 1608 - 1616, The enrollment approval currently
checks and sets enrollment.status == "pending" non-atomically, allowing
concurrent requests to both pass the check and send duplicate notifications;
change the logic to perform an atomic state transition and only send
notifications if the transition actually occurred (e.g., use a transactional
update or Enrollment.objects.filter(pk=enrollment.pk,
status="pending").update(status="approved") and test the affected-row count, or
wrap get_object_or_404(Enrollment, ...) in a transaction with select_for_update
and update the model), and only call send_enrollment_confirmation(enrollment)
and notify_teacher_new_enrollment(enrollment) when the atomic update succeeded;
keep references to the Enrollment instance, its status, and the notification
functions send_enrollment_confirmation and notify_teacher_new_enrollment to
locate and modify the code.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@web/views.py`:
- Around line 1608-1616: The enrollment approval currently checks and sets
enrollment.status == "pending" non-atomically, allowing concurrent requests to
both pass the check and send duplicate notifications; change the logic to
perform an atomic state transition and only send notifications if the transition
actually occurred (e.g., use a transactional update or
Enrollment.objects.filter(pk=enrollment.pk,
status="pending").update(status="approved") and test the affected-row count, or
wrap get_object_or_404(Enrollment, ...) in a transaction with select_for_update
and update the model), and only call send_enrollment_confirmation(enrollment)
and notify_teacher_new_enrollment(enrollment) when the atomic update succeeded;
keep references to the Enrollment instance, its status, and the notification
functions send_enrollment_confirmation and notify_teacher_new_enrollment to
locate and modify the code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6a16dd8c-3a60-4eda-a3cd-dcf8ec6ac9e4

📥 Commits

Reviewing files that changed from the base of the PR and between c94caf8 and fdfb509.

📒 Files selected for processing (1)

• web/views.py