Network balance limit (Client dependent) by shivkumarsah · Pull Request #38 · facebookarchive/CommunityCellularManager

shivkumarsah · 2017-06-02T14:48:00Z

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 · 2017-06-02T14:48:07Z

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 cla@fb.com. 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 cla@fb.com if you have any questions.

facebook-github-bot · 2017-06-02T14:59:27Z

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 · 2017-06-02T15:19:42Z

@shivkumarsah updated the pull request - view changes

kkroo

Thanks! Some small changes.

kkroo · 2017-06-05T15:59:42Z

cloud/endagaweb/forms/dashboard_forms.py

+    """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",


More clear name like max_balalance?

comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf

kkroo · 2017-06-05T16:00:37Z

cloud/endagaweb/forms/dashboard_forms.py

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


max_transfer_attempts or max_unsuccessful_transfers?

comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf

kkroo · 2017-06-05T16:01:34Z

cloud/endagaweb/models.py

    # 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)


this should mention balance max_balance maybe?

comments incorporated in commit id :1e5832b491ca904b5fd002d9d61f3d1fca09cbcf

kkroo · 2017-06-05T16:02:04Z

cloud/endagaweb/models.py

    environment = models.TextField(default="default")
+    # Added for Network Balance Limit
+    max_account_limit = models.BigIntegerField(default=10000)
+    max_failure_transaction = models.IntegerField(default=3)


is this specific to all transactions or transfers?

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

kkroo · 2017-06-05T16:03:13Z

cloud/endagaweb/templates/dashboard/network_detail/denomination.html

+{% block js %}
+
+    <script type="text/javascript"
+            src="https://ajax.aspnetcdn.com/ajax/jquery.validate/1.11.1/jquery.validate.min.js"></script>


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

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

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

kkroo · 2017-06-05T16:06:38Z

cloud/endagaweb/views/dashboard.py

            if abs(amount) > 2147483647:
                raise ValueError(error_text)
+            if sub.balance + amount > network.max_account_limit:
+                error_text = 'Error : Crossed Credit Limit.'


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?

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:

kkroo · 2017-06-05T16:10:48Z

cloud/endagaweb/views/network.py

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


NIT: Raise an error rather than forcing a maximum?

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

please incorporate the change into this review

kkroo · 2017-06-05T16:11:59Z

cloud/endagaweb/views/network.py

+                return redirect(urlresolvers.reverse('network-denominations'))
+
+            user_profile = models.UserProfile.objects.get(user=request.user)
+            with transaction.atomic():


kkroo · 2017-06-05T16:15:18Z

cloud/endagaweb/views/network.py

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


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

kkroo · 2017-06-05T16:17:03Z

cloud/endagaweb/models.py

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


NIT: humanize_credits on this?

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

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

kkroo · 2017-06-05T16:25:28Z

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

facebook-github-bot · 2017-06-08T17:04:04Z

@shivkumarsah updated the pull request - view changes

facebook-github-bot · 2017-06-08T17:07:49Z

@shivkumarsah updated the pull request - view changes

facebook-github-bot · 2017-06-13T06:52:38Z

@shivkumarsah updated the pull request - view changes

kkroo · 2017-06-15T02:02:35Z

is this ready for review?

piyushabad88 · 2017-06-15T04:08:42Z

yes , please review.As suggested comments are incorporated.

kkroo · 2017-06-15T16:59:07Z

cloud/endagaweb/templates/dashboard/network_detail/denomination.html

+{% block js %}
+
+    <script type="text/javascript"
+            src="https://ajax.aspnetcdn.com/ajax/jquery.validate/1.11.1/jquery.validate.min.js"></script>


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

kkroo · 2017-06-15T17:02:32Z

cloud/endagaweb/views/dashboard.py

+            currency_value = str(humanize_credits(network.max_balance, CURRENCIES[currency]))
            if abs(amount) > 2147483647:
                raise ValueError(error_text)
+            if (sub.balance + amount) > network.max_balance:


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

We will implement this on the client

removed.fixed in commit id :# 07e0730

kkroo · 2017-06-15T17:04:16Z

cloud/endagaweb/views/network.py

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


please incorporate the change into this review

kkroo · 2017-06-15T17:05:24Z

cloud/endagaweb/templates/dashboard/network_detail/nav.html

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


Rename this to Security/Safety

comments incorporated with commit id:# 07e0730

kkroo · 2017-06-15T17:06:21Z

cloud/endagaweb/templates/dashboard/network_detail/denomination.html

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


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

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 · 2017-06-29T10:12:10Z

@shivkumarsah updated the pull request - view changes

facebook-github-bot · 2017-06-29T11:11:55Z

@shivkumarsah updated the pull request - view changes

facebook-github-bot · 2017-07-19T21:29:11Z

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

9muir · 2017-07-19T21:54:27Z

Please address comments from our internal linter:

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

…tyCellularManager into sprint2_pr_network_limit

facebook-github-bot · 2017-07-21T09:52:51Z

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

facebook-github-bot · 2017-07-21T13:43:51Z

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

Credit limit setting

8e88b74

facebook-github-bot added GH Review: review-needed CLA Signed labels Jun 2, 2017

Denomination

847fc80

kkroo suggested changes Jun 5, 2017

View reviewed changes

shivkumarsah mentioned this pull request Jun 6, 2017

Subscriber adjust credit - DO NOT MERGE #40

Closed

refactored code

1e5832b

removed print statement

75f24ad

updated test case and error print statement

f32fa02

kkroo suggested changes Jun 15, 2017

View reviewed changes

review comments incorporated to display text description for max bala…

07e0730

…nce limit,Unsuccessful transactions and denomination brackets,disable max balance check of the operator UI and made denomination changes

resolved conflicts

d745522

facebook-github-bot added the Import Started label Jul 19, 2017

piyushabad88 added 2 commits July 21, 2017 14:37

Merge branch 'master' of https://github.com/facebookincubator/Communi…

8c1a4e8

…tyCellularManager into sprint2_pr_network_limit

updated with master and comments incorporated as per linter

3c1352c

shivkumarsah changed the title ~~Network balance limit - DO NOT MERGE~~ Network balance limit (Client dependent) Aug 4, 2017

Comments

Conversation

shivkumarsah commented Jun 2, 2017

Uh oh!

facebook-github-bot commented Jun 2, 2017

Uh oh!

facebook-github-bot commented Jun 2, 2017

Uh oh!

facebook-github-bot commented Jun 2, 2017

Uh oh!

kkroo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkroo commented Jun 5, 2017

Uh oh!

facebook-github-bot commented Jun 8, 2017

Uh oh!

facebook-github-bot commented Jun 8, 2017

Uh oh!

facebook-github-bot commented Jun 13, 2017

Uh oh!

kkroo commented Jun 15, 2017

Uh oh!

piyushabad88 commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment