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 endpoints for Activity (create/edit/delete) #196 #210

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

Conversation

ErnstBas
Copy link

@ErnstBas ErnstBas commented Mar 2, 2023

This is a solution for #196

I have created two api endpoints: activities/api for retrieving all activities and the activities/api/ for the individual activities

The requested CRUD functionality is added as well as the permissions for companions and organizers.

I am not entirely sure about line 20 in activities/views.py. Please have a look whether this is a good solution to select the circle_id in order to check if the user is part of the circle.

And any other feed back to improve this solution is highly appreciated.

Thanks

@CLAassistant
Copy link

CLAassistant commented Mar 2, 2023

CLA assistant check
All committers have signed the CLA.

@ErnstBas ErnstBas changed the title 196 Add REST endpoints for Activity (create/edit/delete) #196 Mar 2, 2023
@ErnstBas
Copy link
Author

ErnstBas commented Mar 5, 2023

Hi @brylie shall I resolve the issues that where automatically detected and submit a new PR, or what is the procedure to follow?
The other question regarding the CLA puzzles me. I signed the CLA before submitting, using the email address I always use for Guthub. I see that other users have encountered this same issue. Maybe signing it again and submitting a new PR could help. Any suggestions?

@brylie
Copy link
Member

brylie commented Mar 8, 2023

You can resolve the issues on the current PR :-)

It looks like your Git commits were authored with the following email: zx88707@ou.ac.uk

https://github.com/CompanionshipCare/companionship-care/commit/e130b12611c1e507ebada6057c21a004e5473f7c.patch

Resolved - hopefully - the issues raised by DeepSource

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
fixing deepsource issues

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
deepsource issues fixed

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
deepsource issues fixed

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
DeepSource issues fixed

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
DeepSource issue fixed

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
DeepSource issue fixed

Signed-off-by: Bas Ernst <50556177+ErnstBas@users.noreply.github.com>
@ErnstBas
Copy link
Author

Hello @brylie, if you have any feedback on the code, please let me know. Otherwise, I could maybe work on some of the other issues?

Circle member(s) can create a Circle Activity via a POST request
Circle members and coordinators can view all Circle Activities via a GET request
"""
circle_id = Activity.objects.values_list("circle_id", flat=True).first()
Copy link
Member

Choose a reason for hiding this comment

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

Since Activities are logically scoped to a circle, let's pass in the circle_id from the URL:

  • api/v1/circles/{ circle_id }/activities



@api_view(['GET', 'POST'])
def activity_list(request):
Copy link
Member

Choose a reason for hiding this comment

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

For cleaner code, let's use class-based API views.

https://www.django-rest-framework.org/tutorial/3-class-based-views/

circle_id = Activity.objects.values_list("circle_id", flat=True).first()
circle = Circle.objects.get(id=circle_id)

if request.user in circle.companions:
Copy link
Member

Choose a reason for hiding this comment

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

When using class-based views, we can use permission_classes for re-usable permission logic:

https://www.django-rest-framework.org/tutorial/4-authentication-and-permissions/#object-level-permissions

if request.user in circle.companions:

if request.method == 'GET':
activities = Activity.objects.all()
Copy link
Member

Choose a reason for hiding this comment

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

This would seem to be a heavy database query, since it could get a cursor to every activity for every circle. The activities should be scoped to the circle.

@ErnstBas
Copy link
Author

Thanks for your feedback @brylie I will look at it during the weekend.

@ErnstBas
Copy link
Author

Hi @brylie , I am punching a bit above my weight here. I need a better understanding of Django and DRF to resolve these problems without losing a lot of time, which unfortunately I don't have at the moment. Please feel free to reassign this issue to someone else. I would like to come back this summer, better prepared.

@brylie
Copy link
Member

brylie commented Mar 29, 2023

Hey @ErnstBas. No worries. DRF and the likes are quite complicated and I'm confused as well.

Would you like to help with more conventional Django tasks on a website we are trying to launch by August of this year?

I've been developing the Western Friend website using Django and Wagtail CMS since 2019, and we're in the late stages of the project:

https://github.com/WesternFriend/WF-website

Many of the remaining issues are related to HTML, CSS, and Bootstrap styling. If you'd like to help with the Western Friend project, I'd appreciate the assistance and gladly help you get started with a good first issue.

@ErnstBas
Copy link
Author

Thanks for your understanding @brylie . I like working on this project, but did not really manage linking circle members and organisers and activities. Eventually I will get there. Anyway, the next few weeks I will be pretty busy, but I will consider the other project too. Thanks for the opportunity!

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.

3 participants