-
Notifications
You must be signed in to change notification settings - Fork 50
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
Improve API (API 1.1) #421
base: next
Are you sure you want to change the base?
Conversation
The other endpoints use 'Invalid ID' to signify either missing or invalid. This endpoint used to use 'Invalid Username' to signify that the member does not exist, while the other's would've used 'Member not found' for that.
There's an inconsistency in whether to call a member a user. Generally, users are the ones with admin access, while members are the customers.
0a51575
to
1b2a2e5
Compare
Even though it's a bool, I believe it's more appropriate to keep the naming consistent with non-bool return types. Also, it still returns an object.
I won't rename the actual endpoint because of backwards compatibility.
QR-endpoint apparently uses Stregdollars as opposed to Stregøre. I feel like streamlining this is too big a change. |
More meaningful response in Active Products-endpoint, now returns {"1": {"name": "Beer", "price": 600}}, instead of {"1": ["Beer", 600]}
Changed response format for Active Products-endpoint: Instead of |
Changed format for Category_mapping endpoint output: Much more verbose, but it's more like the other endpoints. Considering changing it to not return products which don't have any mapped categories |
I've decided to leave the sales API as is |
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.
a few small changes, but looks nice 👍
re_path(r'^api/member/get_id$', views.get_member_id, name="get_member_id"), | ||
re_path(r'^api/member/balance$', views.get_member_balance, name="get_member_balance"), | ||
re_path(r'^api/member$', views.get_member_info, name="get_member_info"), | ||
re_path(r'^api/products/named_products$', views.dump_named_products, name="dump_named_products"), |
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 not get_named_products
and get_active_products
? this change seems related to making better function names, so why not follow what is above?
|
||
data = 'mobilepay://send?{}'.format(urllib.parse.urlencode(query)) |
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.
remove urllib.parse import
try: | ||
member = Member.objects.get(username=username) | ||
except Member.DoesNotExist: | ||
return HttpResponseBadRequest("Invalid username") | ||
return HttpResponseBadRequest("Member not found") |
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.
The rest of these error messages are prefixed with something like Parameter missing:
but not this one
Edit: i see more that aren't prefixed as well, i think it would be nice to have them all prefixed like that
@@ -72,7 +72,7 @@ def __init__(self, *args, **kwargs): | |||
|
|||
class QRPaymentForm(forms.Form): | |||
member = forms.CharField(max_length=16) | |||
amount = forms.DecimalField(min_value=50, decimal_places=2, required=False) | |||
amount = forms.DecimalField(min_value=0.01, decimal_places=2, required=False) |
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.
I don't see the point in this change? don't we disallow payments below 50?
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.
also when i send a request to http://localhost:8000/api/member/payment/qr?member=tester&amount=0.01
it fails, while amount=0.02
doesn't
#420 aims to document the API. This PR addresses inconsistencies found during that documentation effort. Both in implementation and in the actual API.
It increments the API versioning to 1.1