-
Notifications
You must be signed in to change notification settings - Fork 14
PaymentProviderMethod is now able to use an Amount, a Percentage and a combination of both. #30
base: master
Are you sure you want to change the base?
Conversation
…ge field at PaymentProviderMethod.
…tion for amountType.
//{ | ||
// return Convert.ToInt32(OrderTotalWithoutPaymentInCents * PaymentProviderOrderPercentage / 10000); | ||
//} | ||
//return PaymentProviderAmount; |
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.
don't leave out commented code.
Removed out commented code. Some indents fixed.
@kappy Thanks for noticing, fixed that. |
I will let @mnwsmit comment on this, because the code is not formatted properly. This project uses tabs instead of spaces. |
Functionally this is quite a big PR. I currently don't have time to review it, sorry |
Fine for me. Built this functionality mainly for my own application. I think, however, that its an improvement to your software. |
@@ -44,7 +44,7 @@ public class Order : IConvertibleToOrderInfo | |||
public DateTime? FulfillDate; | |||
public PaymentInfo PaymentInfo; | |||
public int PaymentProviderPrice; | |||
public int? PaymentProviderOrderPercentage; | |||
public decimal? PaymentProviderOrderPercentage; |
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.
Do you really need to use percentages with more than 2 decimal places? If not, please keep using the same "philosophy" of having a percentage represented in an int
* 100
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 think we should move on to converting that to a decimal
but we don't know the code that is already relying on PaymentProviderOrderPercentage being a int
* 100
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.
No I don't need more places, but the end user should be able to enter the value as 2.80% or 0.028 instead of 280 (which is confusing).
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 hope not 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.
I don't care how to store/process the percentage, as long as the field is clear for end-users.
No description provided.