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

Add rest API endpoints to Circles to allow CRUD requests #213

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

werberger
Copy link
Contributor

This resolves issue #195.

@brylie please review and give me any feedback. I have tested using the Browsable API and Postman.

Test coverage was 83%
isort and black both run.

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage: 81.25% and project coverage change: -0.04 ⚠️

Comparison is base (9343835) 82.48% compared to head (76ef839) 82.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
- Coverage   82.48%   82.44%   -0.04%     
==========================================
  Files          23       24       +1     
  Lines         508      524      +16     
==========================================
+ Hits          419      432      +13     
- Misses         89       92       +3     
Impacted Files Coverage Δ
project/circles/views.py 56.80% <70.00%> (+1.14%) ⬆️
project/circles/serializers.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@brylie
Copy link
Member

brylie commented Mar 9, 2023

This looks good so far, in that it creates a view that lists all circles.

The next step would be to return only the circles that the requesting user has the right to view. Check the code for the existing "circles" view to get an idea of how to narrow down the queryset.

https://github.com/CompanionshipCare/companionship-care/blob/9343835b92c1ba0202a03f7b8eff60ac727c37de/project/circles/templates/circles/circle_list.html#L23

@werberger
Copy link
Contributor Author

werberger commented Mar 16, 2023

I have got this working. At least it works in Postman. However attempting a GET request in the browser returns:

"detail": "Authentication credentials were not provided."

I have attempted including the token as part of the url, i.e.

http://127.0.0.1:8000/api/v1/circles/?format=api&Authorization=Token <token_value>

as well as passing it as a query parameter, i.e. in the browser console, using

document.cookie = 'Authorization=Token <token_value>;'

but neither of these solutions work

Assuming this doesn't matter I'll update the PR. But if it does matter, would appreciate some suggestions.

@brylie
Copy link
Member

brylie commented Mar 19, 2023

Typically, authorization tokens are passed in via a request header. See the following document for more details about authorizing requests with Postman: https://learning.postman.com/docs/sending-requests/authorization/

@werberger
Copy link
Contributor Author

Typically, authorization tokens are passed in via a request header. See the following document for more details about authorizing requests with Postman: https://learning.postman.com/docs/sending-requests/authorization/

Is this article not talking about passing them in Postman? I have it working in Postman. It is in the browsable API that I can't get it working.

@werberger
Copy link
Contributor Author

@brylie I managed to get it working - I needed to use a browser extension to edit the header.

@brylie
Copy link
Member

brylie commented Mar 20, 2023

Ah, I understand. FWIW, we don't need to test these API endpoints in a browser since they will be used in a mobile client. Only Postman should be necessary, along with automated tests from Python.

@werberger
Copy link
Contributor Author

To explain the rogue activity on this PR, I pushed my work on #197 which automatically added it to this PR. I subsequently reverted the commits.

I hope I haven't caused any issues @brylie. Please let me know if there was a better way to have dealt with this.

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

Successfully merging this pull request may close these issues.

2 participants