Denomination bracket - DO NOT MERGE #39
Conversation
|
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. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
cloud/endagaweb/models.py
Outdated
| network = models.ForeignKey('Network', null=True, on_delete=models.CASCADE) | ||
|
|
||
| def __unicode__(self): | ||
| return "Amount %s - %d for %s(days)" % ( |
There was a problem hiding this comment.
NIT: humanize_credits on this?
There was a problem hiding this comment.
Fixed in commit # 1367cfd
cloud/endagaweb/views/network.py
Outdated
| CURRENCIES[currency]).amount_raw | ||
| validity_days = int(request.POST.get('validity_days')) or 0 | ||
| if validity_days > 10000: | ||
| validity_days = 10000 |
There was a problem hiding this comment.
NIT: Raise an error rather than forcing a maximum? Just saying you can do a lot of these checks in subclasses of forms.Form and raise forms.ValidationError and keep the post handler for more involved checks of combined fields/writing
There was a problem hiding this comment.
Fixed in commit # 1367cfd Create configurable setting for limit and showing error now
| return redirect(urlresolvers.reverse('network-denominations')) | ||
|
|
||
| user_profile = models.UserProfile.objects.get(user=request.user) | ||
| with transaction.atomic(): |
|
@shivkumarsah updated the pull request - view changes |
|
To vacuum subscriber balances after validity_days, we have created another PR #42 |
|
@kkroo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| self.client.get('/logout') | ||
|
|
||
|
|
||
| class DenominationUITest(TestBase): |
There was a problem hiding this comment.
This isn't really a UI test, but an API test. And it's testing Denomination, not User.
| class DenominationUITest(TestBase): | ||
| """Testing that we can add User in the UI.""" | ||
|
|
||
| def test_add_denominaton(self): |
There was a problem hiding this comment.
The tests should be broken out into a small set of tests that verify, for each operation, that an unauthenticated user cannot do them (and also that an authenticated user without the appropriate permission cannot do them either), and then a much larger set of tests that verify the various constraints on denominations.
| self.logout() | ||
| response = self.client.get('/dashboard/network/denominations') | ||
| # Anonymous User can not see this page so returning permission denied. | ||
| self.assertEqual(302, response.status_code) |
There was a problem hiding this comment.
302 doesn't seem like the correct return code here, unless it's an artifact of authentication failure being translated into redirection back to the login page. That doesn't seem like the right behaviour an API call though.
| 'validity_days': 3 | ||
| } | ||
| response = self.client.post('/dashboard/network/denominations', data) | ||
| self.assertEqual(302, response.status_code) |
There was a problem hiding this comment.
Why does an authenticated user get a 302 here? Isn't the point of this test that it should pass?
Please review network denomination bracket which include the below features: