-
Notifications
You must be signed in to change notification settings - Fork 44
Fixes #86 - Add a REST api for Tasks and TaskAreas #87
Conversation
bobsilverberg
commented
Dec 11, 2013
- add the djangorestframework to requirements
- add serializers, views and routes for the api
- set api authorization to admin users only
@Osmose r? This is my first time using this framework so there may be a bunch of stuff to fix/change in here, but I've tested it and it works. I opted to just go with a basic form of authorization now, where only admin users can access the API. |
@Osmose I guess we'd best hold off on this for now. @stephendonner is in the process of convincing me that we should at least try out putting the functional tests in the repo. I know we'll lose a bunch of functionality that we now have, but we can see how difficult it is to get close to what we now have with py.test and our plugin. |
Geez, I really should read up on related issues before commenting. :( I just saw @willkg's response on the issue and now I'm not sure what to think. There are pros and cons to both approaches, but I do think that trying to come up with a solution that will give us what we have now, using liveservertestcase, will be far more work, so maybe we should just give this a try for now. |
@Osmose One thing I neglected to mention about this: when testing I found I had to access the api using a locale. I.e., I could not access the api at |
The SUPPORTED_NONLOCALES setting is what you're looking for. Just add |
|
||
|
||
class TaskAreaSerializer(serializers.ModelSerializer): | ||
|
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.
Excessive spacing.
So if I'm understanding this correctly, the tests will log in via the normal Django admin page, then use the session they set up that way to make API requests? |
I've never used django-rest-framework but this seems straightforward. If you're confident that it'll work I'm r+wc on this. If you want we can also have someone who has used it before look it over but IMO that's not necessary. Nice work! |
Two things:
|
@willkg That makes sense about the dependencies. I actually did have it in a separate commit, but then I squashed everything before pushing. |
@Osmose I have pushed a new commit that addresses all of your comments. Regarding how the tests will use the API, they will just pass credentials in the request - they won't use the django admin at all. |
Still an r+ on this. Should probably squash, then amend to split out the library vs non-library changes. Let me know if you need me to merge this, I imagined you would handle it. :D |
- add the djangorestframework to requirements - add serializers, views and routes for the api - set api authorization to admin users only
@Osmose Ok, I created a separate commit for the library changes, and I will merge it. I'm not accustomed to merging my own PRs, but with your r+, hey, why not? |
Fixes #86 - Add a REST api for Tasks and TaskAreas