Skip to content
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

REST API for channels #39

Merged
merged 12 commits into from
Jul 20, 2017
Merged

REST API for channels #39

merged 12 commits into from
Jul 20, 2017

Conversation

noisecapella
Copy link

@noisecapella noisecapella commented Jul 14, 2017

What are the relevant tickets?

Part of #29

What's this PR do?

Implements rest API for channels

How should this be manually tested?

You can go to /api/v0/channels/ and play around with it using the DRF HTML view. You should also be able to go to /api/v0/channels/<subreddit>/ to view a particular channel or to patch it

@codecov-io
Copy link

codecov-io commented Jul 14, 2017

Codecov Report

Merging #39 into master will increase coverage by 1.39%.
The diff coverage is 91.8%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #39      +/-   ##
=========================================
+ Coverage   89.21%   90.6%   +1.39%     
=========================================
  Files          18      22       +4     
  Lines         519     575      +56     
  Branches       38      41       +3     
=========================================
+ Hits          463     521      +58     
+ Misses         46      42       -4     
- Partials       10      12       +2
Impacted Files Coverage Δ
open_discussions/urls.py 62.5% <ø> (ø) ⬆️
open_discussions/settings.py 89.79% <ø> (ø) ⬆️
channels/conftest.py 100% <100%> (ø)
channels/views.py 100% <100%> (ø)
channels/urls.py 100% <100%> (ø)
channels/api.py 84.94% <50%> (+3.83%) ⬆️
channels/serializers.py 94.44% <94.44%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08fd0d1...e562e62. Read the comment docs.

@noisecapella noisecapella force-pushed the gs/channel_rest_api branch 2 times, most recently from 94055a5 to 179c6b6 Compare July 19, 2017 15:55
@rhysyngsun rhysyngsun self-assigned this Jul 19, 2017
api.user = True

name = instance.display_name
key_map = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a module-level constant instead?


def update(self, instance, validated_data):
api = Api()
api.user = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see this burning us later on, I think maybe it's better to just comment out any validation code (an early return from _assert_authenticated should suffice to avoid this.

name = instance.display_name
key_map = {
'title': 'title',
'channel_type': 'subreddit_type',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arg for update_channel is actually also channel_type so this mapping is unnecessary and actually causing updates to channel_type to not work. update_channel validates it's kwargsso you could probably just catch theValueErrorand reraise as a drfValidationError`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried fixing this locally and still didn't see the channel_type update so something else is going on here too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a problem too, I'll look into it

@noisecapella
Copy link
Author

Should be ready for review again. I also rebased on #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants