-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[UI] History - fix CJ group amounts (alternative) #11839
Conversation
@@ -40,7 +40,7 @@ public partial class TransactionModel : ReactiveObject | |||
|
|||
public Money? Fee { get; init; } | |||
|
|||
public Money Amount => Math.Abs(IncomingAmount ?? OutgoingAmount ?? Money.Zero); | |||
public Money Amount => IncomingAmount ?? -(OutgoingAmount ?? Money.Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the amount should be either negative or positive, depending if it was an outgoing or incoming transaction.
Previously it was always positive which was misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYM? What is this amount field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yahiheb the TransactionModel.Amount
property is a unification of the IncomingAmount
and OutgoingAmount
(as shown above) and is used in several places in the UI logic that creates the transaction tree (TransactionTreeBuilder
). In the future if these columns are unified in the UI the Amount
value can be displayed in the UI as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo this is not the correct solution.
Check my review here #11838 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK.
This PR fixes a bug currently on master.
fixes #11818
closes #11838