Conversation
ae06f5a to
5b6e2d3
Compare
5b6e2d3 to
0d72122
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses three issues related to the Kaui admin UI:
Changes:
- Fixed BCD display to show subscription-level BCD when available, falling back to account-level BCD (Issue #460)
- Added support for '/aviate' mount point in breadcrumb navigation (Issue #579)
- Improved "Add Currency" functionality by setting product name and adding client-side validation (Issue #410)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/helpers/kaui/account_helper_test.rb | Adds comprehensive unit tests for the new effective_bcd helper method |
| app/helpers/kaui/account_helper.rb | Implements effective_bcd helper to prioritize subscription BCD over account BCD |
| app/controllers/kaui/accounts_controller.rb | Fetches bundles data needed for calculating effective BCD |
| app/views/kaui/accounts/_billing_info.html.erb | Updates BCD display to use effective_bcd helper |
| app/views/kaui/accounts/_billing_details.html.erb | Updates BCD display to use effective_bcd helper |
| app/views/kaui/subscriptions/_subscriptions_table.html.erb | Prevents showing "Add add-on" menu item on ADD_ON subscriptions |
| app/views/kaui/components/breadcrumb/_breadcrumb.html.erb | Adds support for '/aviate' mount point in breadcrumb navigation |
| app/views/kaui/admin_tenants/new_plan_currency.html.erb | Adds HTML5 required attribute to amount field for client-side validation |
| app/controllers/kaui/admin_tenants_controller.rb | Auto-populates product name when adding currency to existing plan and comments out duplicate plan validation |
| app/models/kaui/catalog.rb | Refactors array indexing using Ruby idiom ([-1] instead of [length-1]) |
| app/controllers/kaui/subscriptions_controller.rb | Refactors array indexing using Ruby idiom ([-1] instead of [size-1]) |
Comments suppressed due to low confidence (1)
app/controllers/kaui/admin_tenants_controller.rb:222
- The redirect in the
unless is_plan_id_foundblock is missing areturnstatement. This means that when the plan is not found, the code continues execution after the redirect, which can lead to the form still being rendered and accessed with a nilproduct, potentially causing errors. Addand returnafter the redirect to ensure the method exits immediately.
unless is_plan_id_found
flash[:error] = "Plan id #{plan_id} was not found."
redirect_to admin_tenant_path(@tenant[:id])
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elsif @all_plans.include?(@simple_plan.plan_id) | ||
| flash.now[:error] = "Error while creating plan: plan #{@simple_plan.plan_id} already exists" | ||
| valid = false | ||
| # flash.now[:error] = "Error while creating plan: plan #{@simple_plan.plan_id} already exists" | ||
| # valid = false |
There was a problem hiding this comment.
The commented-out validation that checks if a plan already exists was intentionally disabled to fix Issue #410. However, this change needs reconsideration. When adding a currency to an existing plan, the plan ID will already exist in @all_plans, which would have incorrectly triggered this validation. The proper fix should distinguish between creating a new plan and adding a currency to an existing plan. Consider checking if there's a product_name parameter to determine the operation type, or add a separate flag to identify "add currency" operations.
| # Get the effective BCD for an account | ||
| # If there's an active subscription with its own BCD, return that | ||
| # Otherwise return the account-level BCD | ||
| def effective_bcd(account, bundles = nil) | ||
| # If bundles are provided, check for subscription BCD | ||
| if bundles.present? | ||
| bundles.each do |bundle| | ||
| next unless bundle.subscriptions.present? | ||
|
|
||
| bundle.subscriptions.each do |subscription| | ||
| # Skip cancelled subscriptions | ||
| next if subscription.state == 'CANCELLED' | ||
| # If subscription has a BCD set, return it (subscription BCD takes precedence) | ||
| return subscription.bill_cycle_day_local if subscription.bill_cycle_day_local.present? && subscription.bill_cycle_day_local.positive? | ||
| end |
There was a problem hiding this comment.
The effective_bcd helper only skips CANCELLED subscriptions, but subscriptions can also be in BLOCKED (paused) state. Based on the codebase patterns, BLOCKED subscriptions should likely still have their BCD displayed since they are temporarily paused and will resume. The current implementation correctly returns the subscription BCD for BLOCKED subscriptions, which is appropriate behavior. However, consider adding a test case for BLOCKED state to document this behavior explicitly.
Related issues: