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

implement tickets and orders #857

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

Conversation

vladislav-tsybuliak1
Copy link

No description provided.

seat=ticket["seat"]
)

return order

Choose a reason for hiding this comment

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

You don’t need to return anything in create/update/delete methods, check everywhere

services/user.py Outdated
user.first_name = first_name
if last_name:
user.last_name = last_name
user.save()

Choose a reason for hiding this comment

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

Try to use update function here

Choose a reason for hiding this comment

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

In Django documentation it is written:
Sometimes you want to set a field to a particular value for all the objects in a QuerySet. You can do this with the update() method.
And these:
To save changes to an object that’s already in the database, use save().

I think in this case, because we update only 1 object, it is better to use save().

Also, with the update function I see two possible ways how to implement it here and problems that arise after:
First, I can collect all changes in a dict and make one update in the end. I will have more code here, And I will need to create an additional dict and then itterate over it. It won't be as efficient as the code above.
Second, I can update everytime I check an argument - this will create many queries to the database If I want to change many fields.. (1 query per field). I thinks it's also innefficient.

If I'm wrong, looking forward to read from you.)

Choose a reason for hiding this comment

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

I just want you try to use update method, because this also an way to learn it better)
Collect all changes

settings.py Outdated

Choose a reason for hiding this comment

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

Don't need to commit the file

db/models.py Outdated
from django.db import models
from django.db.models import UniqueConstraint

import settings
Copy link

Choose a reason for hiding this comment

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

it's better to use
from django.conf import settings because it defines main settings for sure.
It's possible that you have multiple settings files or a folder named "settings", thus python will incorrectly import settings

@@ -6,6 +7,7 @@
def get_movies(
genres_ids: list[int] = None,
actors_ids: list[int] = None,
title: str = None
Copy link

Choose a reason for hiding this comment

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

str or None type. fix annotation everywhere

Choose a reason for hiding this comment

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

Okay, but this code was written before and in the task description I didn't find anything about editing it. But I have noticed this annotation mistakes.

return [
{"row": ticket.row, "seat": ticket.seat}
for ticket
in movie_session.tickets.all()
Copy link

Choose a reason for hiding this comment

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

it's better to use .values() method so that you avoid manually creating dicts

hall_name: str, hall_rows: int, hall_seats_in_row: int
hall_name: str,
hall_rows: int,
hall_seats_in_row: int
) -> CinemaHall:
return CinemaHall.objects.create(

Choose a reason for hiding this comment

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

You don’t need to return anything in create/update/delete methods, check everywhere

Copy link
Author

@vladislav-tsybuliak1 vladislav-tsybuliak1 Aug 31, 2024

Choose a reason for hiding this comment

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

okay, but this part of the code was written not by me - this code was given.
And in the task description it is nowhere said that I need to change the existing code.
Maybe, it is better to mention it in the task.

services/user.py Outdated
user.first_name = first_name
if last_name:
user.last_name = last_name
user.save()

Choose a reason for hiding this comment

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

I just want you try to use update method, because this also an way to learn it better)
Collect all changes

from django.db.models import QuerySet

from db.models import Movie


def get_movies(
genres_ids: list[int] = None,
actors_ids: list[int] = None,
genres_ids: list[int] | None = None,

Choose a reason for hiding this comment

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

fix indentation here

movie.actors.set(actors_ids)

return movie
movie_title: str,

Choose a reason for hiding this comment

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

this is also overindented

movie_show_time: str, movie_id: int, cinema_hall_id: int
) -> MovieSession:
return MovieSession.objects.create(
movie_show_time: str,

Choose a reason for hiding this comment

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

indentation

show_time: str = None,
movie_id: int = None,
cinema_hall_id: int = None,
session_id: int,

Choose a reason for hiding this comment

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

overindented



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

Choose a reason for hiding this comment

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

or you can do this list(get_movie_session_by_id(movie_session_id).tickets.values("row", "seat"))

services/user.py Outdated


def get_user(user_id: int) -> get_user_model():
return get_user_model().objects.get(id=user_id)

Choose a reason for hiding this comment

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

use pk here

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.

4 participants