Skip to content
This repository has been archived by the owner on Sep 26, 2018. It is now read-only.

Network balance limit (Client dependent) #38

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

shivkumarsah
Copy link
Contributor

Please review network/subscriber credit limit implementation which include the below features:

  • Cloud admin and admin have rights to set the highest denomination bracket (Reload limit) correspond to which subscriber can do top up.
  • Cloud admin and admin have rights to set the maximum balance limit a subscriber can have. Retailer cannot transfer more than the set balance limit amount to any subscriber.
  • Subscribers can not have the account balance greater than the set balance limit.
  • Cloud admin and admin have rights to set the limit on maximum number of consecutive unsuccessful transactions.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes

Copy link
Contributor

@kkroo kkroo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some small changes.

"""Crispy form to set Network balance limit and transaction.
set min_value =0.01 so that it will not accept 0 value"""

limit = forms.CharField(required=False, label="Maximum Balance Limit",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More clear name like max_balalance?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf


limit = forms.CharField(required=False, label="Maximum Balance Limit",
max_length=10)
transaction = forms.CharField(required=False, max_length=3,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_transfer_attempts or max_unsuccessful_transfers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf

@@ -978,6 +978,9 @@ class Network(models.Model):
# Network environments let you specify things like "prod", "test", "dev",
# etc so they can be filtered out of alerts. For internal use.
environment = models.TextField(default="default")
# Added for Network Balance Limit
max_account_limit = models.BigIntegerField(default=10000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should mention balance max_balance maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf

@@ -978,6 +978,9 @@ class Network(models.Model):
# Network environments let you specify things like "prod", "test", "dev",
# etc so they can be filtered out of alerts. For internal use.
environment = models.TextField(default="default")
# Added for Network Balance Limit
max_account_limit = models.BigIntegerField(default=10000)
max_failure_transaction = models.IntegerField(default=3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this specific to all transactions or transfers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes this network max balance limit applied to network.If any transaction happen for subscriber of same network this is applied on them

{% block js %}

<script type="text/javascript"
src="https://ajax.aspnetcdn.com/ajax/jquery.validate/1.11.1/jquery.validate.min.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check your jquery version matches the rest of the site. Try to use the same CDN if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit # 1367cfd
Its part of PR #39

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep changes local to the PR. This will result in merge conflicts

@@ -560,6 +560,9 @@ def post(self, request, imsi=None):
CURRENCIES[currency]).amount_raw
if abs(amount) > 2147483647:
raise ValueError(error_text)
if sub.balance + amount > network.max_account_limit:
error_text = 'Error : Crossed Credit Limit.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print out the max limit in the error message? Also check your units. I am pretty sure sub.balance is actually in millicents/pesos depending on the currency selected. Testing this in USD is a good idea. Also can we write tests for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now while checking cross limitation ,there also take out network_max_balance based on currency.written unit test case.
comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf.
attached console output for unit test:

unittest_for_network_max_balance

CURRENCIES[currency]).amount_raw
validity_days = int(request.POST.get('validity_days')) or 0
if validity_days > 10000:
validity_days = 10000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Raise an error rather than forcing a maximum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit # 1367cfd
Its part of PR #39

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please incorporate the change into this review

return redirect(urlresolvers.reverse('network-denominations'))

user_profile = models.UserProfile.objects.get(user=request.user)
with transaction.atomic():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)


if request.POST.get('transaction') == "" and request.POST.get(
'limit') == "":
error_text = 'Error : please provide value.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saying you can do a lot of these checks in subclasses of forms.Form and raise forms.ValidationError


def __unicode__(self):
return "Amount %s - %d for %s(days)" % (
self.start_amount, self.end_amount, self.validity_days)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: humanize_credits on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in commit # 1367cfd
Its part of PR #39

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please dont open new PRs for fixes to existing PRs its difficult to keep track of changes this way

@kkroo
Copy link
Contributor

kkroo commented Jun 5, 2017

Careful to split out reviews. I think this also contained #39

@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes

@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes

@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes

@kkroo
Copy link
Contributor

kkroo commented Jun 15, 2017

is this ready for review?

@piyushabad88
Copy link
Contributor

yes , please review.As suggested comments are incorporated.

{% block js %}

<script type="text/javascript"
src="https://ajax.aspnetcdn.com/ajax/jquery.validate/1.11.1/jquery.validate.min.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep changes local to the PR. This will result in merge conflicts

if abs(amount) > 2147483647:
raise ValueError(error_text)
if (sub.balance + amount) > network.max_balance:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we wanted to disable this check of the operator UI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will implement this on the client

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.fixed in commit id :# 07e0730

CURRENCIES[currency]).amount_raw
validity_days = int(request.POST.get('validity_days')) or 0
if validity_days > 10000:
validity_days = 10000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please incorporate the change into this review

<a href="
{% if active_tab == 'limit' %} #
#{% else %}{% url 'network_balance_limit' %}
{% endif %}">Limit</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to Security/Safety

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments incorporated with commit id:# 07e0730

@@ -0,0 +1,305 @@
{% extends "dashboard/layout.html" %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our phone call, lets make the denominations a continuous range?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be fixed as part of sprint 5

…nce limit,Unsuccessful transactions and denomination brackets,disable max balance check of the operator UI and made denomination changes
@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes

@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes

@facebook-github-bot
Copy link

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@9muir
Copy link
Contributor

9muir commented Jul 19, 2017

Please address comments from our internal linter:

image

If you rebase against the current head then three of the F401 errors should go away (I fixed them in a commit recently).

@facebook-github-bot
Copy link

@shivkumarsah updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@shivkumarsah shivkumarsah changed the title Network balance limit - DO NOT MERGE Network balance limit (Client dependent) Aug 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants