Skip to content

Add code to handle user with transfer fees. Refs #16867#46

Open
caosborne89 wants to merge 4 commits intomasterfrom
story/16867_transfer_fee
Open

Add code to handle user with transfer fees. Refs #16867#46
caosborne89 wants to merge 4 commits intomasterfrom
story/16867_transfer_fee

Conversation

@caosborne89
Copy link
Collaborator

@caosborne89 caosborne89 commented Mar 18, 2026

Note

Low Risk
Low risk: adds a read-only Alma API call and simple XML parsing to drive new conditional messaging in the fees page, without changing payment or transaction flows.

Overview
Adds support for transferred/"EXPORTED" Alma fees by making an additional GET /users/{id}/fees?status=EXPORTED request, parsing the response via new AlmaUserData::hasTransferFees(), and passing has_transfer_fees into the index view.

Updates index.html.twig to show explanatory links/messages when transferred fees exist, including a dedicated empty-state when there are no payable fees but transferred fees are present.

Written by Cursor Bugbot for commit 420a5fd. This will update automatically on new commits. Configure here.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for all 3 issues found in the latest run.

  • ✅ Fixed: Redundant status check after already-filtered API response
    • Simplified hasTransferFees to return whether any fee nodes exist because the API query already filters to EXPORTED fees.
  • ✅ Fixed: Unhandled transfer fee API failure breaks entire page
    • Wrapped the transfer-fee API call in a try/catch for GuzzleException and defaulted hasTransferFees to false on failure so the page still renders.
  • ✅ Fixed: Inconsistent transfer fee messaging and destination links
    • Updated the active-fees transfer message to match the no-active-fees branch by directing users consistently to the bursar account in UAAccess.

Create PR

Or push these changes by commenting:

@cursor push 31e5642680
Preview (31e5642680)
diff --git a/src/Controller/ListFeesController.php b/src/Controller/ListFeesController.php
--- a/src/Controller/ListFeesController.php
+++ b/src/Controller/ListFeesController.php
@@ -6,6 +6,7 @@
 use App\Service\AlmaApi;
 use App\Service\AlmaUserData;
 use Doctrine\Persistence\ManagerRegistry;
+use GuzzleHttp\Exception\GuzzleException;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\Routing\Annotation\Route;
 
@@ -40,7 +41,11 @@
         foreach ($userFees as $userFee) {
             $totalDue += $userFee['balance'];
         }
-        $hasTransferFees = $this->userData->hasTransferFees($this->api->getUserTransferFees($user->getUserIdentifier()));
+        try {
+            $hasTransferFees = $this->userData->hasTransferFees($this->api->getUserTransferFees($user->getUserIdentifier()));
+        } catch (GuzzleException $e) {
+            $hasTransferFees = false;
+        }
 
         return $this->render('views/index.html.twig', [
             'full_name' => $user->getFullName(),

diff --git a/src/Service/AlmaUserData.php b/src/Service/AlmaUserData.php
--- a/src/Service/AlmaUserData.php
+++ b/src/Service/AlmaUserData.php
@@ -46,13 +46,7 @@
     public function hasTransferFees(Response $response)
     {
         $sxml = new SimpleXMLElement($response->getBody());
-        // If any fee has the status of "EXPORTED", then it is a transfer fee and we return true, otherwise we return false
-        foreach ($sxml->fee as $indv_fee) {
-            if ((string)$indv_fee->status === 'EXPORTED') {
-                return true;
-            }
-        }
-        return false;
+        return count($sxml->fee) > 0;
     }
 
     /**

diff --git a/templates/views/index.html.twig b/templates/views/index.html.twig
--- a/templates/views/index.html.twig
+++ b/templates/views/index.html.twig
@@ -37,7 +37,7 @@
 
         {% if user_fees %}
             {% if has_transfer_fees %}
-            <p>Some of your library fees have been moved to your student account. You can <a href="https://arizona-ua.primo.exlibrisgroup.com/discovery/account?vid=01UA_INST:01UA&section=fines" target="_blank">view them here</a>.</p>
+            <p>Some of your library fees have been transferred to your <a href="https://uaccess.arizona.edu/" target="_blank">bursar's account</a>.</p>
             {% endif %}
 
         <div class="charges__total-amount-due">Your total amount due is:

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

}
}
return false;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant status check after already-filtered API response

Low Severity

hasTransferFees iterates through fees checking if each has status === 'EXPORTED', but the response it receives is already filtered to only EXPORTED fees by getUserTransferFees (which passes 'status' => 'EXPORTED' to the API). The loop could simply check whether any fee exists in the response rather than re-checking the status, making the intent clearer and removing unnecessary complexity.

Additional Locations (1)
Fix in Cursor Fix in Web

foreach ($userFees as $userFee) {
$totalDue += $userFee['balance'];
}
$hasTransferFees = $this->userData->hasTransferFees($this->api->getUserTransferFees($user->getUserIdentifier()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unhandled transfer fee API failure breaks entire page

Medium Severity

The getUserTransferFees call can throw a GuzzleException (since executeApiRequest re-throws exceptions), and there's no try-catch in the controller. If this purely informational API call fails, the entire page crashes — preventing users from viewing or paying their active fees even when getUserFees succeeded. A failure in this secondary feature now breaks the primary payment flow.

Fix in Cursor Fix in Web

{% if user_fees %}
{% if has_transfer_fees %}
<p>Some of your library fees have been moved to your student account. You can <a href="https://arizona-ua.primo.exlibrisgroup.com/discovery/account?vid=01UA_INST:01UA&section=fines" target="_blank">view them here</a>.</p>
{% endif %}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent transfer fee messaging and destination links

Medium Severity

The same transfer fees (EXPORTED status) are described differently and linked to different destinations depending on whether the user also has active fees. When active fees exist, the message says fees moved to "student account" and links to Primo/ExLibris. When no active fees exist, it says fees went to "bursar's account" and links to UAAccess. The same fee type is being directed to two different systems, which will confuse users.

Additional Locations (1)
Fix in Cursor Fix in Web

@cursor
Copy link

cursor bot commented Mar 23, 2026

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants