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

implemented tickets-and-orders #391

Closed
wants to merge 3 commits into from
Closed

implemented tickets-and-orders #391

wants to merge 3 commits into from

Conversation

mariiahorbova
Copy link

No description provided.

db/models.py Outdated
@@ -17,7 +18,7 @@ def __str__(self) -> str:


class Movie(models.Model):
title = models.CharField(max_length=255)
title = models.CharField(max_length=255, db_index=True)

Choose a reason for hiding this comment

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

I think its bad practice when you use db_index=True. Because when the model expands it will be difficult to maintain

def __str__(self) -> str:
return f"{self.created_at}"

class Meta:

Choose a reason for hiding this comment

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

class meta should be after attribute and before methods

db/models.py Outdated
)

def clean(self) -> None:
from django.core.exceptions import ValidationError

Choose a reason for hiding this comment

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

bad practice I think mentors reject if u will use import like this

db/models.py Outdated
def clean(self) -> None:
from django.core.exceptions import ValidationError

if not 1 <= self.row <= self.movie_session.cinema_hall.rows:

Choose a reason for hiding this comment

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

if not (1 <= self.row <= self.movie_session.cinema_hall.rows):

db/models.py Outdated
]
}
)
if not 1 <= self.seat <= self.movie_session.cinema_hall.seats_in_row:

Choose a reason for hiding this comment

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

same here

return queryset


def get_movie_by_id(movie_id: int) -> Movie:
return Movie.objects.get(id=movie_id)


@atomic

Choose a reason for hiding this comment

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

probably better use transtaction.atomic() but not sure. Mb mentors can help with this)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it is more usual to use @transaction.atomic decorator,but it think this is not critical

db/models.py Outdated

class Order(models.Model):
created_at = models.DateTimeField(auto_now_add=True)
user = models.ForeignKey(to=User, on_delete=models.CASCADE)

Choose a reason for hiding this comment

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

related_name

@@ -42,3 +44,11 @@ def update_movie_session(

def delete_movie_session_by_id(session_id: int) -> None:
MovieSession.objects.get(id=session_id).delete()


def get_taken_seats(movie_session_id: int) -> List[dict]:

Choose a reason for hiding this comment

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

I think bad formatting but not sure

from settings import AUTH_USER_MODEL


def create_user(

Choose a reason for hiding this comment

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

u can use here kwargs and check if key in ["first_name".....] - Oleg hint

Choose a reason for hiding this comment

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

with setattr

services/user.py Outdated


def get_user(user_id: int) -> AUTH_USER_MODEL:
user = get_user_model()

Choose a reason for hiding this comment

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

optional, but zero sense to create variable

return user.objects.get(id=user_id)


def update_user(

Choose a reason for hiding this comment

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

same like create

db/models.py Outdated

class Order(models.Model):
created_at = models.DateTimeField(auto_now_add=True)
user = models.ForeignKey(to=User, on_delete=models.CASCADE)

Choose a reason for hiding this comment

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

to=settings.AUTH_USER_MODEL

db/models.py Outdated
@@ -17,7 +18,7 @@ def __str__(self) -> str:


class Movie(models.Model):
title = models.CharField(max_length=255)
title = models.CharField(max_length=255, db_index=True)
description = models.TextField()
actors = models.ManyToManyField(to=Actor)

Choose a reason for hiding this comment

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

related_name

from settings import AUTH_USER_MODEL


def create_user(

Choose a reason for hiding this comment

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

with setattr

return queryset


def get_movie_by_id(movie_id: int) -> Movie:
return Movie.objects.get(id=movie_id)


@atomic
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it is more usual to use @transaction.atomic decorator,but it think this is not critical

@mariiahorbova mariiahorbova closed this by deleting the head repository Mar 13, 2024
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