-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validate maximum length of account number and bank blocks #132
Conversation
Hey @JonSchapiro Are these test cases working for you locally? |
@shahraizali my second test now works. However, it seems that the validator is not working on the query param while asking for an account by account_number? |
okay, btw if don't know already. you can run the tests locally as well so you don't have to push to test. I am judging from multiple commits. |
yes, apologies. I realized that now and can run them locally. |
No worries. have at it, try to debug the issue now that you can run it locally. make sure you are pinging the correct endpoint and also make sure you are sending the account number along in the expected and correct way. You can update here or in the channel with your findings. |
fc53f27
to
b05c63f
Compare
@shahraizali could I have a review on this please :)? |
@vosi @shahraizali can this be merged? |
@shahraizali @vosi just checking in again to see if someone can merge? |
@@ -6,7 +6,7 @@ | |||
|
|||
|
|||
class AccountFactory(DjangoModelFactory): | |||
account_number = Faker('pystr', max_chars=VERIFY_KEY_LENGTH) | |||
account_number = Faker('pystr', max_chars=VERIFY_KEY_LENGTH - 1) |
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.
why -1?
@@ -31,12 +33,18 @@ class AccountViewSet( | |||
|
|||
@action(methods=['get'], detail=True) | |||
def balance(self, request, account_number=None): | |||
if account_number is not None and len(account_number) >= VERIFY_KEY_LENGTH: | |||
return Response({'balance': None}, status=status.HTTP_401_UNAUTHORIZED) |
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.
why 401?
@@ -31,12 +33,18 @@ class AccountViewSet( | |||
|
|||
@action(methods=['get'], detail=True) | |||
def balance(self, request, account_number=None): | |||
if account_number is not None and len(account_number) >= VERIFY_KEY_LENGTH: |
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.
>=
? 64 is ok if I'm not mistaking.
Closing due to inactivity. Want to allow another contributors to work on these tests. |
No description provided.