feat(dashspend): add discount values to Merchant lists and details #1298
Conversation
|
We are waiting on some things to be resolved before continuing with this PR. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces multiple updates across modules. In common utilities, function signatures and logic have been revised to use fractional discount values instead of percentages. New drawable resources for button backgrounds and icons have been added. The ExploreDash feature has seen changes in the Merchant model and discount display logic, with variable renaming and layout enhancements. In the wallet module, notifications components have been adjusted, the contacts update flow has been refactored (with a new transaction filter option for gift cards), and the database version has been downgraded. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PGF as PurchaseGiftCardFragment
participant VM as CTXSpendViewModel
participant GU as GenericUtils
participant Confirm as PurchaseGiftCardConfirmDialog
User->>PGF: Initiate gift card purchase
PGF->>VM: getDiscountedAmount(fullAmount, savingsFraction)
VM->>VM: Compute discounted amount using savingsFraction
VM-->>PGF: Return discounted amount
PGF->>GU: formatPercent(savingsFraction)
GU-->>PGF: Return formatted discount text
PGF->>Confirm: Open dialog with discount info
sequenceDiagram
participant UI
participant VM as MainViewModel
participant Processor as TransactionProcessor
UI->>VM: Provide transactions list (txs)
VM->>Processor: Filter self-transactions and build contacts map
Processor-->>VM: Return contacts map
VM->>UI: Update UI with contacts and metadata
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsFragment.kt (2)
271-273:⚠️ Potential issueFix credit balance warning logic error.
There's a logic error in the credit balance warning checks:
- Line 273 incorrectly reuses
isBalanceWarning()forisEmpty- This makes
isEmptyidentical toshouldWarn, which is not the intended behaviorApply this diff to fix the logic:
- val shouldWarn = enough.isBalanceWarning() - val isEmpty = enough.isBalanceWarning() + val shouldWarn = enough.isBalanceWarning() + val isEmpty = enough.isEmpty()
264-266: 💡 Verification agent🧩 Analysis chain
Verify the impact of unimplemented ignore request functionality.
The
onIgnoreRequestfunction is referenced in the adapter but not implemented. This could lead to unexpected behavior when users try to ignore contact requests.Let's check if this function is used in the UI:
🏁 Script executed:
#!/bin/bash # Search for references to ignore request functionality in layouts and adapters rg -A 5 "ignore.*request|request.*ignore" --glob "*.xml" --glob "*Adapter.kt"Length of output: 27896
Unimplemented ignore request handler impacts user experience
The UI layouts clearly expect an action when users tap the ignore button (e.g. via the "ignore_contact_request" resource across multiple layouts), but the
onIgnoreRequestcallback inNotificationsFragment.ktremains empty. This could lead to confusing or unresponsive interactions for users.
- Issue: The callback is referenced (likely during adapter initialization) without a proper implementation.
- Recommendation: Either implement the necessary logic (or at least provide a temporary notification such as a toast indicating that this feature isn't available yet) or adjust the adapter to disable the ignore action until a proper implementation is provided.
🧹 Nitpick comments (8)
wallet/res/layout/fragment_notifications.xml (2)
69-71: RecyclerView Identifier and Sizing Update:
The RecyclerView's ID change fromcontacts_rvtonotifications_rvaligns with the updated purpose. Changing the layout width tomatch_parentand height towrap_contentshould be evaluated in the context of your design—ensure that this does not adversely impact scrolling behavior if the list grows longer.
79-89: New Empty State TextView Addition:
The new TextView with IDno_notifications_labelenhances user experience by displaying a message when there are no notifications. Confirm that the string resource@string/notifications_none_newis defined and review if additional accessibility attributes (like content descriptions) might be beneficial.common/src/main/res/drawable-v21/dark_gray_button_background.xml (1)
12-33: State Selector and Layer-list Configuration
The<selector>element is well implemented for handling state-dependent backgrounds. The disabled state is explicitly defined with<item android:state_enabled="false">, and the fallback (enabled) state uses a<layer-list>containing a<shape>. However, note that both states specify the same solid color (@color/dash_black_0.7). Please confirm that the lack of visual differentiation between enabled and disabled states is intentional per design specifications.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/adapters/MerchantsAtmsResultAdapter.kt (2)
116-126: Consider using epsilon comparison for floating-point values.While the logic is correct, comparing floating-point numbers directly with
0.00might lead to precision issues. Consider using an epsilon-based comparison for more robust handling of floating-point values.- if (merchant.savingsAsDouble != 0.00) { + val epsilon = 0.000001 // Adjust based on your precision needs + if (merchant.savingsAsDouble > epsilon) {
116-126: Consider using Kotlin's safe call operator.The null check and discount visibility logic can be simplified using Kotlin's safe call operator.
- if (merchant != null) { - if (merchant.savingsAsDouble != 0.00) { - binding.discountValue.isVisible = true - binding.discountValue.text = binding.root.context.getString( - R.string.explore_discount, - merchant.savingsPercentageAsDouble - ) - } else { - binding.discountValue.isVisible = false - } - } + merchant?.let { + binding.discountValue.isVisible = it.savingsAsDouble != 0.00 + if (it.savingsAsDouble != 0.00) { + binding.discountValue.text = binding.root.context.getString( + R.string.explore_discount, + it.savingsPercentageAsDouble + ) + } + }features/exploredash/src/main/res/layout/merchant_row.xml (1)
64-73: Addition of Discount Value TextView
A newTextView(discount_value) has been introduced to display discount information. Consider adding a default attribute, for example:android:visibility="gone"This ensures that the view remains hidden when no discount is applicable, preventing any unwanted blank space or layout shifts.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (2)
165-165: Consider renaming the property for better clarity.The property name
savingsAsDoubledoesn't clearly indicate whether it returns a fraction (0.0-1.0) or percentage (0-100). Consider renaming it tosavingsFractionorsavingsAsFractionto make the expected value range more explicit.
167-167: Consider renaming the constant for better clarity.The constant name
DEFAULT_DISCOUNT_AS_DOUBLEdoesn't clearly indicate whether it represents a fraction (0.0-1.0) or percentage (0-100). Consider renaming it toDEFAULT_DISCOUNT_FRACTIONto make the expected value range more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
common/src/main/java/org/dash/wallet/common/util/GenericUtils.kt(1 hunks)common/src/main/java/org/dash/wallet/common/util/MonetaryExt.kt(1 hunks)common/src/main/res/drawable-v21/dark_gray_button_background.xml(1 hunks)common/src/main/res/drawable/triangle_black_70.xml(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/model/Merchant.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/adapters/MerchantsAtmsResultAdapter.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/ItemDetails.kt(1 hunks)features/exploredash/src/main/res/layout/item_details_view.xml(2 hunks)features/exploredash/src/main/res/layout/merchant_row.xml(2 hunks)features/exploredash/src/main/res/values/strings-explore-dash.xml(2 hunks)wallet/res/layout/fragment_notifications.xml(2 hunks)wallet/src/de/schildbach/wallet/database/AppDatabase.kt(1 hunks)wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsFragment.kt(4 hunks)wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt(2 hunks)wallet/src/de/schildbach/wallet/ui/main/TransactionsFilterDialog.kt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- common/src/main/res/drawable/triangle_black_70.xml
- wallet/src/de/schildbach/wallet/database/AppDatabase.kt
👮 Files not reviewed due to content moderation or server errors (4)
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/model/Merchant.kt
- common/src/main/java/org/dash/wallet/common/util/MonetaryExt.kt
- common/src/main/java/org/dash/wallet/common/util/GenericUtils.kt
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/adapters/MerchantsAtmsResultAdapter.kt
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (34)
wallet/res/layout/fragment_notifications.xml (1)
7-8: Context and Orientation Update:
The introduction ofandroid:orientation="vertical"ensures the layout’s children are stacked vertically, and the addition oftools:context="de.schildbach.wallet.ui.dashpay.NotificationsFragment"provides proper design-time context. Please verify that the specified fragment path matches your project structure for correct preview behavior.common/src/main/res/drawable-v21/dark_gray_button_background.xml (2)
1-3: XML Header and Root Element Validity
The XML declaration and the opening<ripple>tag—with the correct namespace and color attribute—are properly structured.
5-10: Proper Mask Definition
The<item android:id="@android:id/mask">block correctly defines a mask using a<shape>with a rectangular form, solid color, and rounded corners (6dp), which is ideal for creating the intended ripple effect.wallet/src/de/schildbach/wallet/ui/main/TransactionsFilterDialog.kt (2)
76-80: LGTM! The gift card filter UI implementation is consistent.The implementation follows the established pattern of other filter options, using the same
IconSelectMode.Noneand a similar structure.
41-41:❓ Verification inconclusive
LGTM! The new filter option enhances transaction categorization.
The addition of
TxFilterType.GIFT_CARDto the options list is well-structured and maintains consistency with existing filter types.Let's verify the existence of the required resources:
🏁 Script executed:
#!/bin/bash # Description: Verify the existence of required string and drawable resources # Check for the string resource rg -A 1 'name="explore_filter_gift_cards"' 'res/values*/strings.xml' # Check for the drawable resource fd 'ic_filter_gift_card.xml' 'res/drawable*'Length of output: 280
Resource Verification Required: Confirm the Gift Card Assets
The addition of
TxFilterType.GIFT_CARDis implemented consistently with the other filter types. However, the automated checks for the required resources didn’t produce any output. Please verify manually that:
- The string resource
explore_filter_gift_cardsis defined (for example, inwallet/res/values/strings.xml).- The drawable resource
ic_filter_gift_card.xmlexists in the appropriate drawable folder (such aswallet/res/drawable).Once these assets are confirmed or added, the implementation is good to go.
wallet/src/de/schildbach/wallet/ui/dashpay/NotificationsFragment.kt (3)
24-24: LGTM! Import changes improve code organization.The import changes enhance type safety with dedicated notification item classes and follow Kotlin best practices.
Also applies to: 32-36, 52-52
108-109: LGTM! UI component naming is more descriptive.The RecyclerView reference rename from
contactsRvtonotificationsRvbetter reflects its purpose in handling notifications.
195-195: LGTM! Empty state handling improves user experience.The empty state visibility toggle using
isVisibleprovides clear feedback when there are no notifications.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/explore/model/Merchant.kt (9)
53-53: LGTM! Clear documentation of basis points.The comment clearly explains that the value is in basis points where 1 = 0.001%, which helps prevent confusion about the scale of the values.
58-59: LGTM! Improved property naming.The property rename from
savingsPercentageAsDoubletosavingsAsDoublebetter reflects that this is a raw fraction (where 0.01 = 1%) rather than a display-ready percentage.
61-63: LGTM! Clear separation of concerns.The new
savingsPercentageAsDoubleproperty provides a display-ready percentage value (where 1.00 = 1%), making it easier to use in UI components without additional conversion.
53-53: LGTM! Clear documentation of basis points.The comment clearly explains that savings are stored in basis points where 1 = 0.001%.
58-59: LGTM! Improved property naming.The property rename from
savingsPercentageAsDoubletosavingsAsDoublebetter reflects that this is a fractional value (e.g., 0.01 for 1%).
61-63: LGTM! Clear separation of display and calculation values.The new
savingsPercentageAsDoubleproperty provides a display-ready percentage value, making it easier to show discounts in the UI.
53-53: LGTM! Clear documentation of basis points representation.The comment effectively clarifies that the
savingsPercentageuses basis points where 1 = 0.001%, which is a standard financial notation.
58-59: LGTM! Accurate fraction conversion.The property correctly converts basis points to a decimal fraction (e.g., 1000 basis points → 0.10 or 10%).
61-63: LGTM! Clear percentage representation.The property appropriately converts the fraction to a percentage for display purposes (e.g., 0.10 → 10.00).
common/src/main/java/org/dash/wallet/common/util/MonetaryExt.kt (3)
88-89: LGTM! Clearer discount calculation.The function now uses a more intuitive fractional representation (where 0.01 = 1%) and a simpler calculation formula. This aligns well with the broader changes in discount representation across the codebase.
88-89: LGTM! Simplified discount calculation using fractions.The change from percentage to fraction simplifies the calculation and aligns with the codebase-wide shift to fractional representation of discounts.
88-89: LGTM! Correct discount calculation using fraction.The function has been properly updated to use fractional representation, with the calculation
value * (1.0 - fraction)correctly applying the discount (e.g., 0.10 for a 10% discount).common/src/main/java/org/dash/wallet/common/util/GenericUtils.kt (3)
97-102: LGTM! Consistent fraction handling.The function has been updated to:
- Use clearer parameter naming (
fractioninstead ofpercent).- Document the expected input format (0.01 = 1.00%).
- Directly use the fraction value without additional conversion.
This aligns well with the codebase-wide standardization of discount representation.
97-102: LGTM! Consistent use of fractional values.The changes to
formatPercentalign with the codebase-wide shift to fractional representation:
- Documentation clearly states that 0.01 represents 1.00%
- Implementation directly uses the fraction value
96-102: LGTM! Clear documentation and correct percentage formatting.The function has been properly updated to:
- Accept fractions (e.g., 0.01 for 1.00%) instead of percentages.
- Use the built-in formatter which automatically handles the conversion to percentage display.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (1)
70-83: LGTM! Consistent representation of discount valuesThe changes correctly implement the transition from percentage to fraction representation for merchant savings, maintaining consistency with the broader codebase changes.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (2)
188-192: LGTM! Consistent parameter namingThe method signature change from
savingsPercentagetosavingsFractionmaintains consistency with the new fraction-based representation of discounts.
78-79: LGTM! Correct discount calculationThe balance calculation correctly uses the savings fraction and properly accounts for transaction fees.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/ItemDetails.kt (1)
241-251: LGTM! Clean UI state managementThe code properly manages the visibility of discount-related UI elements based on the merchant's savings value, using the appropriate percentage format for display.
wallet/src/de/schildbach/wallet/ui/main/MainViewModel.kt (1)
662-683: LGTM! Improved efficiency and clarityThe refactored method includes several improvements:
- Early return for empty transaction lists
- More descriptive method name
- Better null safety with proper use of safe call operators
- Efficient contact mapping with proper filtering
features/exploredash/src/main/res/layout/merchant_row.xml (2)
44-49: Updated Title TextView Margin and Constraint
The end margin for the title has been reduced from 16dp to 3dp, and its end constraint now points to@id/discount_value. This change appears to align with the new discount display design.
57-62: Adjusted Subtitle TextView Layout
The subtitle TextView’s end margin has been similarly reduced and its constraint updated to reference@id/discount_value. This maintains consistency in the layout adjustments.features/exploredash/src/main/res/values/strings-explore-dash.xml (2)
49-49: Addition of Discount Format String
The new string resourceexplore_discountwith the value~%.0f%%has been added. Please ensure that the inclusion of the tilde (~) is intentional to meet the design requirements. The use oftranslatable="false"is appropriate if this format should remain unchanged across locales.
66-66: Addition of Discount Save String
The stringexplore_pay_with_dash_saveis correctly defined with a two-decimal-place format. It should effectively display the discount as intended.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (1)
178-181:✅ Verification successful
Verify the behavior of
formatPercentwith fraction values.The method name
formatPercentsuggests it expects a percentage value (0-100), but it's being passed a fraction (0.0-1.0). Ensure thatformatPercentcorrectly handles fraction values and formats them as expected.Run the following script to check the implementation of
formatPercent:
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of formatPercent to ensure it handles fractions correctly # Search for the formatPercent function implementation ast-grep --pattern $'fun formatPercent($_) { $$$ }'Length of output: 54
Verify the behavior of
formatPercentfor handling fraction valuesThe call to
GenericUtils.formatPercent(savingsFraction)passes a fraction (0.0–1.0) whereas the method name implies it may expect a percentage (0–100). Please run the above script and manually verify that the implementation offormatPercenteither:
- Converts the fraction to a percentage internally (e.g., multiplies by 100), or
- Is called with a pre-converted percentage value.
If the function does not perform the conversion, update the code to either multiply
savingsFractionby 100 before passing it or adjust the implementation offormatPercentaccordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash # Searching for the definition of formatPercent in Kotlin files to verify its implementation rg --type kotlin "fun formatPercent" .Length of output: 280
Double-check the conversion logic in
GenericUtils.formatPercentIt appears that the function signature—
fun formatPercent(fraction: Double): String?—suggests it accepts a fraction (0.0–1.0). However, the method name implies working with a percentage value (0–100). Please verify whether the implementation:
- Multiplies the fraction by 100 internally before formatting, or
- Expects the caller to supply a percentage value.
If the function does not perform the conversion, update its usage or adjust the implementation accordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash # Display snippet of the implementation of formatPercent to check its conversion logic. rg --type kotlin -A 15 "fun formatPercent" ./common/src/main/java/org/dash/wallet/common/util/GenericUtils.ktLength of output: 388
Implementation Verified:
formatPercentcorrectly processes fraction valuesAfter checking the implementation in
GenericUtils.kt, we can confirm thatformatPercentinternally converts a fraction (e.g., 0.01) to its percentage representation (e.g., 1.00%). No further changes are required for this usage.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/adapters/MerchantsAtmsResultAdapter.kt (1)
116-126: LGTM! Clear and safe discount display logic.The implementation:
- Safely checks for null merchant
- Correctly uses
savingsAsDoublefor the visibility check- Properly formats the discount using
savingsPercentageAsDouble- Uses string resources for localization
| <ImageView | ||
| android:id="@+id/discount_stem" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:src="@drawable/triangle_black_70" | ||
| app:layout_constraintEnd_toStartOf="@+id/discount_value" | ||
| app:layout_constraintBottom_toTopOf="@id/pay_btn" | ||
| /> | ||
| <TextView | ||
| android:id="@+id/discount_value" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| tools:text="Save 3.75%" | ||
| android:textColor="@color/white" | ||
| android:background="@drawable/dark_gray_button_background" | ||
| android:paddingHorizontal="5dp" | ||
| android:paddingVertical="1dp" | ||
| app:layout_constraintTop_toTopOf="@+id/pay_btn" | ||
| app:layout_constraintBottom_toTopOf="@+id/pay_btn" | ||
| app:layout_constraintEnd_toEndOf="@id/pay_btn" | ||
| android:layout_marginEnd="30dp" | ||
| /> |
There was a problem hiding this comment.
Review of Discount Components Constraints
The newly added discount components (discount_stem and discount_value) are meant to visually represent discounts. However, note that the discount_value TextView has its top constrained to the top of @+id/pay_btn and its bottom constrained to the top of @+id/pay_btn. This effectively forces its vertical size to zero or may result in ambiguous positioning.
- Please verify whether the intended behavior was to anchor
discount_valueabove the pay button (or possibly between the pay button and thediscount_stem). - Consider revising the vertical constraints—for example, constraining the bottom of
discount_valueto the top ofdiscount_stem(or vice-versa) to properly separate these elements. - Additionally, setting a default
android:visibility="gone"on both discount components will help ensure they only appear when a discount is present.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Refactor
Style