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.

Sign up for GitHub

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

Jump to bottom

Modify game event template, Implement logic on join event #107

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Modify game event template, Implement logic on join event #107

wants to merge 1 commit into from

Conversation

amitkaplansky
Copy link
Collaborator

@amitkaplansky amitkaplansky commented Jun 2, 2023 •
edited
Loading

Desired Outcome

  • Game event template will not need the refresh script as before, we had conflicts with the chat and the refresh.
  • No result.html every change will we shown in the game_event.html.
  • Add logic to join event.

Implemented Changes

  • Added messages error and success to game_event html.

  • modified join_event, remove_from_event, process_answer_game_event in views.

  • removed result html

  • Added is_event_time_available in player model to check if player can join the event.

  • Added style to game_event css for presenting error and success messages.

  • Added two tests in game_event/tests.py

  • Added two fixtures to conftest.

  • removed id field from game_event model, added the new migration 0003_alter_gameevent_id.py, it will be removed after Add player-event linking and event deletion #99 is merged.

Connected Issue/Story

Resolves #108
Resolves #109
Resolves #41

Dependencies

Definition of Done

  • No need for refresh on game_event.html.
  • Show relevant messages in game_event.html.
  • Player can join an event only if he is 'free'.

Test coverage

  • This PR includes two unit and integration tests to go with the code

@amitkaplansky
Copy link
Collaborator Author

game.event.template.mov

You can take a look at the changes (:

@amitkaplansky amitkaplansky force-pushed the mofifyGameEventTemplate branch from a76f97e to b1b85a9 Compare June 2, 2023 09:48
@amitkaplansky amitkaplansky changed the title Modify game event template and Implement Implement logic on join event Modify game event template, Implement logic on join event Jun 2, 2023
@amitkaplansky amitkaplansky force-pushed the mofifyGameEventTemplate branch 2 times, most recently from cb890cb to 3fac629 Compare June 2, 2023 10:01
Danielsio
Danielsio reviewed Jun 2, 2023
View reviewed changes
game_event/tests.py Outdated Show resolved Hide resolved
@Danielsio
Copy link
Collaborator

Danielsio commented Jun 2, 2023 •
edited
Loading

i think you can reference #41 issue as well
edited:
please use full keyword per issue otherwise it wont close it on merge

ZvikaNaorCohen
ZvikaNaorCohen reviewed Jun 2, 2023
View reviewed changes
game_event/models.py
@@ -51,3 +50,9 @@ def validate_game_event(court, ball_game, min_number_of_players, max_number_of_p
@staticmethod
def is_ball_game_playable_at_court(court, ball_game):
return CourtBallGame.is_ball_game_playable(court, ball_game)

def is_event_full(self):
from game_event_player.models import GameEventPlayer
Copy link
Collaborator

@ZvikaNaorCohen ZvikaNaorCohen Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can we move this import to the beginning of the file?
Also in def is_event_time_available(self, desired_event_time) in player/models.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, It will cause a circular dependency

ZvikaNaorCohen
ZvikaNaorCohen reviewed Jun 2, 2023
View reviewed changes
game_event/templates/game_event/join-event.html Outdated
@@ -3,6 +3,7 @@
<html>
<head>
<link rel="stylesheet" type="text/css" href="{% static 'css/game-event.css' %}">
<link rel="stylesheet" type="text/css" href="{% static 'css/game-events.css' %}">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't see file static/css/game-events.css in this PR. Perhaps this line is redundant?

@ZvikaNaorCohen
Copy link
Collaborator

@amitkaplansky and @MaayanMashhadi , the video looks awesome! I really like the new features you've added.
I left some small comments. Looks very good to me.

Yuval-Vino
Yuval-Vino reviewed Jun 5, 2023
View reviewed changes
Copy link
Collaborator

@Yuval-Vino Yuval-Vino left a comment

Choose a reason for hiding this comment

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

I think it will be better to disable the 'Join Event' button when a player is unable to join the event? Instead of rendering the 'Are you bringing a ball?' message and only then receiving the 'Can't join' error.

game_event/templates/game_event/game-event.html Outdated
<div class="button-container">
{% if in_event %}
<a href="/game-events/remove-from-event/{{ id }}" class="round-button remove-event-button">
Remove Event
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove event sounds like the event is being removed and not the player-event

Suggested change
Remove Event
Leave Event

SchSeba
SchSeba reviewed Jun 6, 2023
View reviewed changes
game_event/templates/game_event/game-event.html
</html>
{%endblock%}
{%endblock%}
Copy link

Choose a reason for hiding this comment

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

please add a new line here

game_event/views.py
is_event_time_available = player.is_event_time_available(event.time)

error_messages = []
if is_event_full:
Copy link

Choose a reason for hiding this comment

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

I think this validation should be on the even page not after we click on the join

player/models.py
@@ -54,3 +55,13 @@ def validate_and_save(self, birth_date, favorite_ball_game):

else:
self.save()

def is_event_time_available(self, desired_event_time):
from game_event_player.models import GameEventPlayer
Copy link

Choose a reason for hiding this comment

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

please move the import no need todo import inside a function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will cause a circular dependency between the player model and the game_event_player

@amitkaplansky amitkaplansky force-pushed the mofifyGameEventTemplate branch 3 times, most recently from 99e5916 to 000a2bc Compare June 8, 2023 15:40
@amitkaplansky
Modify game event template
56bf0c5 
Implement logic in join event
@amitkaplansky amitkaplansky force-pushed the mofifyGameEventTemplate branch from 000a2bc to 56bf0c5 Compare June 8, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Danielsio Danielsio Danielsio left review comments

@Yuval-Vino Yuval-Vino Yuval-Vino left review comments

@ZvikaNaorCohen ZvikaNaorCohen ZvikaNaorCohen left review comments

@SchSeba SchSeba SchSeba left review comments

@yftacherzog yftacherzog Awaiting requested review from yftacherzog yftacherzog is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modify Game event template Implement logic on join event Add logic to Time in Game Event entity - Time
5 participants
@amitkaplansky @Danielsio @ZvikaNaorCohen @SchSeba @Yuval-Vino