fix: unsupported operand type for remaining balance #262
Conversation
sameenfatima78
force-pushed
the
sameen/fix-unsupported-operand-type
branch
from
0b170ba
to
c699f56
Compare
sameenfatima78
requested review from
iloveagent57,
adamstankiewicz and
johnnagro
sameenfatima78
force-pushed
the
sameen/fix-unsupported-operand-type
branch
from
c699f56
to
9326113
Compare
sameenfatima78
force-pushed
the
sameen/fix-unsupported-operand-type
branch
from
9326113
to
0a3e7d0
Compare
|
@adamstankiewicz Thanks for the suggestion, I have made the changes. Can you please review again? Also, do you think it would make sense to move the "credit_available" method to the parent class as their definitions in both child classes are identical? |
|
@adamstankiewicz Reminder to please take a look into this on priority. This change is required by this PR that is already merged and deployed on stage but I am waiting to deploy it on prod. |
There was a problem hiding this comment.
Also, do you think it would make sense to move the "credit_available" method to the parent class as their definitions in both child classes are identical?
Hm, good question. Based on the other review comment about the credit_available methods, I think there might be an opportunity to abstract some common/generic checks used across both policy types' credit_available method (e.g., did the policy exceed its configured spend_limit?), but I might recommend keeping the per-learner checks separate as one's "remaining balance" deals with enrollment counts and the other's "remaining balance" deals with spend (money), which are fundamentally two different constructs that might have divergent behavior.
| if self.remaining_balance_per_user(lms_user_id) > 0: | ||
| return True | ||
| return False | ||
| remaining_balance_per_user = self.remaining_balance_per_user(lms_user_id) or 0 |
There was a problem hiding this comment.
Similar to handling None versus 0 differently in the remaining_balance_per_user methods, the None or 0 should likely be handled as semantically different here as well.
If self.remaining_balance_per_user returns None because there is no self.per_learner_enrollment_limit set, I don't believe it's semantically correct to fallback to say the user has a remaining balance of 0, as there is no per-learner limit (this method would return False currently, even when there is no limit set).
Related, are there additional checks that are relevant to determine whether these policies are "available" beyond only checking against the per-learner limits? Would cases such as the following be handled by these credit_available methods?
- User hasn't exceeded their individual per-learner limit, but the policy has already exceeded its configured
spend_limit(not redeemable). - User hasn't exceeded their individual per-learner limit, but the associated subsidy has no remaining balance.
- User hasn't exceeded their individual per-learner limit, but the policy is no longer active.
- User hasn't exceeded their individual per-learner limit, but the associated subsidy is expired.
These are some of the additional checks that the can_redeem methods perform that might be worth referring to. Just verifying that the credits_available API is taking into account all factors that could deem a policy available or unavailable.
There was a problem hiding this comment.
Similar to handling None versus 0 differently in the remaining_balance_per_user methods, the None or 0 should likely be handled as semantically different here as well.
If self.remaining_balance_per_user returns None because there is no self.per_learner_enrollment_limit set, I don't believe it's semantically correct to fallback to say the user has a remaining balance of 0, as there is no per-learner limit (this method would return False currently, even when there is no limit set).
@adamstankiewicz Agreed. I have handled that part in the new changes.
Related, are there additional checks that are relevant to determine whether these policies are "available" beyond only checking against the per-learner limits? Would cases such as the following be handled by these credit_available methods?
User hasn't exceeded their individual per-learner limit, but the policy has already exceeded its configured spend_limit (not redeemable).
User hasn't exceeded their individual per-learner limit, but the associated subsidy has no remaining balance.
User hasn't exceeded their individual per-learner limit, but the policy is no longer active.
User hasn't exceeded their individual per-learner limit, but the associated subsidy is expired.
These are some of the additional checks that the can_redeem methods perform that might be worth referring to. Just verifying that the credits_available API is taking into account all factors that could deem a policy available or unavailable.
@adamstankiewicz I believe the existing code doesn't check any of these conditions. Would it be okay if I create a follow up ticket/PR to address this feedback as all of these checks would require adding unit tests..
There was a problem hiding this comment.
@adamstankiewicz I believe the existing code doesn't check any of these conditions. Would it be okay if I create a follow up ticket/PR to address this feedback as all of these checks would require adding unit tests..
Yep, totally. Just wanted to call out that the current logic may not be fully taking into account all factors.
remove int from transaction count incorporated feedback incorporated feedback round two incorporated feedback round 3
sameenfatima78
force-pushed
the
sameen/fix-unsupported-operand-type
branch
from
0a3e7d0
to
6dc80aa
Compare
|
@adamstankiewicz Gentle reminder to review this PR and if all looks good, please merge it. |
|
@adamstankiewicz Thanks for the approval. Can you also please merge it? I don't seem to have the right access. |
Merged! Edit: That is odd about the lack of merge permissions on your end... |
|
@adamstankiewicz Yeah, I believe it's the same for everyone in Markhors. 😞 |
This change handles the scenario when
spent_amountcomes to beNone.