-
Notifications
You must be signed in to change notification settings - Fork 0
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
Expanded actions for friends to allow for invitations to games, invitations to tournaments #199
base: dev
Are you sure you want to change the base?
Conversation
When I try to run your code using "python3 manage.py runserver", I get the following error "ImportError: cannot import name 'Group' from partially initialized module 'chigame.users.models' (most likely due to a circular import) (/root/chigame/src/chigame/users/models.py)" |
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.
Make sure you address the circular import issue, and pass the checks.
src/chigame/users/views.py
Outdated
def invite_to_game(request, pk, match_id): | ||
sender = User.objects.get(pk=request.user.id) | ||
receiver = User.objects.get(pk=pk) | ||
match = User.objects.get(pk=match_id) |
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.
when defining match, I think you defined it incorrectly, and it should be Match.objects instead of User.objects, as the User model doesn't have a match_id
src/chigame/users/views.py
Outdated
def invite_to_tournament(request, pk, tournament_id): | ||
sender = User.objects.get(pk=request.user.id) | ||
receiver = User.objects.get(pk=pk) | ||
tournament = User.objects.get(pk=tournament_id) |
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.
Same thing here where it should be Tournament.obkects instead of User.objects , as the User model doesn't have a tournament_id
src/chigame/users/views.py
Outdated
sender = User.objects.get(pk=request.user.id) | ||
receiver = User.objects.get(pk=pk) | ||
match = User.objects.get(pk=match_id) | ||
GameInvitation.objects.create(sender=sender, receiver=receiver, group=group, match=match) |
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.
When did you define group in the function? Currently it isn't defined, and may be why you aren't passing the checks.
Reviewed change requests and modified code in views.py accordingly to properly define match and tournament. Also made changes to imports in models.py to account for circular dependencies. |
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 left a few comments, but overall the invitation to game and invitation to tournaments work perfectly, it's really awesome.
One comment:
-When a tournament has ended, it shouldn't give the option to join or withdraw the tournament
src/chigame/users/views.py
Outdated
actor=invitation, receiver=other_user, type=Notification.FRIEND_REQUEST | ||
actor=invitation, | ||
receiver=other_user, | ||
type=Notification.FRIEND_REQUEST, |
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 like that you made the style cleaner and more readable
src/chigame/api/tests/factories.py
Outdated
description = Faker("text") | ||
rules = Faker("text") | ||
draw_rules = Faker("text") | ||
num_winner = Faker("pyint", min_value=1, max_value=1000) |
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.
When creating a new tournament, the website view field is called "Num winner," you should have a better name for it so it's a bit clearer what you're asking when creating the tournament. It's a good name for coding, but it should be changed for the user view on the website.
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 think these are changes handled by us. The only files we changed are src/chigame/urls.py, src/chigame/models.py, and src/chigame/views.py.
src/chigame/games/models.py
Outdated
max_players = models.PositiveIntegerField() | ||
description = models.TextField() # not limited to 255 characters | ||
rules = models.TextField() # not limited to 255 characters | ||
draw_rules = models.TextField() # not limited to 255 characters | ||
num_winner = models.PositiveIntegerField(default=1) # number of possible winners for the tournament |
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.
Good style that it has a default of 1 for better user usability
src/chigame/games/models.py
Outdated
else: # the tournament has ended | ||
return "tournament ended" | ||
|
||
def clean(self): # restriction |
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.
Good error checking!
Update: Rishabh removed "remove friend" functionality due to that now being addressed in a separate PR/Issue. Merge conflicts have been resolved and checks have passed. |
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.
Clarification was given over slack that num winner and the buttons for join and withdraw are not incorporated in this PR and deals with tournaments. The code LGTM!
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.
Nice addition for invitations, but there are some errors in the views and urls. Please test with a match and tournament instance
src/chigame/users/urls.py
Outdated
@@ -31,8 +32,10 @@ | |||
path("add_friend/<int:pk>", view=send_friend_invitation, name="add-friend"), | |||
path("remove_friend/<int:pk>", view=remove_friend, name="remove-friend"), | |||
path("cancel_friend_invitation/<int:pk>", view=cancel_friend_invitation, name="cancel-friend-invitation"), | |||
path("invite_to_game/<int:pk>", view=invite_to_game, name="invite-to-game"), |
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 think these urls are correct because they don't have the pk for the match which is needed for the view
src/chigame/users/views.py
Outdated
@@ -185,6 +185,25 @@ def cancel_friend_invitation(request, pk): | |||
|
|||
|
|||
@login_required | |||
def invite_to_game(request, pk, match_id): | |||
sender = User.objects.get(pk=request.user.id) | |||
receiver = User.objects.get(pk=pk) |
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.
Everything should be in a try statement because there is no error handling for if the receiver or match don't exist. Same goes for tournament
c3e2906
to
55ead9c
Compare
Update: got rid of GameInvitation but kept TournamentInvitation. Tested it and the URL works as expected. |
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 minor changes, but generally looks good!
@@ -185,6 +185,18 @@ def cancel_friend_invitation(request, pk): | |||
|
|||
|
|||
@login_required | |||
def invite_to_tournament(request, pk, tournament_pk): | |||
try: | |||
sender = get_object_or_404(User, pk=request.user.id) |
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.
Need to do one more check where sender and receiver are not the same. You shouldn't be able to send an invite to yourself.
sender = get_object_or_404(User, pk=request.user.id) | ||
receiver = get_object_or_404(User, pk=pk) | ||
tournament = get_object_or_404(Tournament, pk=tournament_pk) | ||
TournamentInvitation.objects.create(sender=sender, receiver=receiver, tournament=tournament) |
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.
Along with the check on user id, the invitation object should only be created if that check succeeds.
This PR involves removing friends, inviting them to games, and inviting them to tournaments. These functionalities would only be accessible if a user is logged in. Next steps would be to create functionality to accept/decline these requests, and incorporate the notification framework.