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

Solution #1078

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

Solution #1078

wants to merge 4 commits into from

Conversation

rzaliznyi
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request:

  1. Return Type Annotations: The return type annotations for several functions in your service files are incorrect. Functions like create_movie, get_movie_by_id, get_movies, create_cinema_hall, create_movie_session, get_movie_session_by_id, get_movies_sessions, and update_movie_session are annotated to return str, but they should return instances of their respective models or QuerySet objects. Correcting these annotations is crucial for maintaining code clarity and preventing potential runtime errors.

  2. Foreign Key Assignments: In your service functions, the cinema_hall and movie fields should be assigned instances of CinemaHall and Movie, respectively, rather than their IDs. Use CinemaHall.objects.get(id=cinema_hall_id) and Movie.objects.get(id=movie_id) to retrieve the instances. This ensures that the foreign key relationships are correctly established.

  3. Related Name Conflicts: The related_name attributes for the actors and genres fields in the Movie model are set to "actors" and "genres", which might conflict with Django's default related names. Consider using more descriptive names like "movies_as_actor" and "movies_as_genre" to avoid potential conflicts.

Once these issues are resolved, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you have any questions or need further assistance. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

Comment on lines +42 to +43
"actors",
models.ManyToManyField(related_name="actors", to="db.actor"),

Choose a reason for hiding this comment

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

The related_name for the actors field is set to 'actors'. This might conflict with the default related name Django generates for reverse lookups. Consider using a more specific name like 'movies_as_actor' to avoid potential conflicts.

Comment on lines +46 to +47
"genres",
models.ManyToManyField(related_name="genres", to="db.genre"),

Choose a reason for hiding this comment

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

The related_name for the genres field is set to 'genres'. This might conflict with the default related name Django generates for reverse lookups. Consider using a more specific name like 'movies_as_genre' to avoid potential conflicts.

db/models.py Outdated
class Movie(models.Model):
title = models.CharField(max_length=255)
description = models.TextField(null=True)
actors = models.ManyToManyField(Actor, related_name="actors")

Choose a reason for hiding this comment

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

The related_name for the actors field is set to 'actors'. This could conflict with the default related name Django generates for reverse lookups. Consider using a more specific name like 'movies_as_actor' to avoid potential conflicts.

db/models.py Outdated
title = models.CharField(max_length=255)
description = models.TextField(null=True)
actors = models.ManyToManyField(Actor, related_name="actors")
genres = models.ManyToManyField(Genre, related_name="genres")

Choose a reason for hiding this comment

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

The related_name for the genres field is set to 'genres'. This could conflict with the default related name Django generates for reverse lookups. Consider using a more specific name like 'movies_as_genre' to avoid potential conflicts.

hall_name: str,
hall_rows: int,
hall_seats_in_row: int
) -> str:

Choose a reason for hiding this comment

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

The return type of the create_cinema_hall function is specified as str, but it should be CinemaHall since the function returns an instance of the CinemaHall model.

Comment on lines 11 to 12
cinema_hall=cinema_hall_id,
movie=movie_id

Choose a reason for hiding this comment

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

The cinema_hall and movie fields should be assigned CinemaHall and Movie instances, respectively, not their IDs. Consider using CinemaHall.objects.get(id=cinema_hall_id) and Movie.objects.get(id=movie_id) to retrieve the instances.

)


def get_movies_sessions(session_date: str = None) -> str:

Choose a reason for hiding this comment

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

The return type of the get_movies_sessions function is specified as str, but it should be QuerySet or list depending on the desired return type, as the function returns a queryset of MovieSession instances.

return query


def get_movie_session_by_id(movie_session_id: int) -> str:

Choose a reason for hiding this comment

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

The return type of the get_movie_session_by_id function is specified as str, but it should be MovieSession since the function returns an instance of the MovieSession model.

show_time: str = None,
movie_id: int = None,
cinema_hall_id: int = None
) -> str:

Choose a reason for hiding this comment

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

The return type of the update_movie_session function is specified as str, but it should be MovieSession since the function returns an instance of the MovieSession model.

Comment on lines 38 to 40
session.movie = movie_id
if cinema_hall_id:
session.cinema_hall = cinema_hall_id

Choose a reason for hiding this comment

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

The movie and cinema_hall fields should be assigned Movie and CinemaHall instances, respectively, not their IDs. Consider using Movie.objects.get(id=movie_id) and CinemaHall.objects.get(id=cinema_hall_id) to retrieve the instances.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great job on your progress! 🎉 While there are some areas that need improvement, your work is on the right track. Here are the key points to address:

  1. Return Type Annotations: Several functions have incorrect return type annotations. Ensure that:

    • create_movie, get_movie_by_id, and get_movies return instances of Movie or a QuerySet of Movie instances.
    • create_cinema_hall returns an instance of CinemaHall.
    • create_movie_session, get_movie_session_by_id, get_movies_sessions, and update_movie_session return instances of MovieSession or a QuerySet of MovieSession instances.
  2. Foreign Key Assignments: When creating a Movie, use the set method to assign actors and genres after the movie object is created, rather than assigning actors_ids and genres_ids directly.

  3. Related Name Conflicts: You've updated the related_name attributes for the actors and genres fields in the Movie model to "movies_as_actor" and "movies_as_genre", which is a good practice to avoid conflicts.

  4. Parameter Types: Ensure that the movie_show_time and show_time parameters are of type datetime to match the DateTimeField in the MovieSession model.

  5. Unrelated Files: Consider removing unrelated files like settings.py from the submission to keep the focus on the task requirements.

Keep up the good work, and make sure to address these issues in future iterations to enhance the functionality and maintainability of your code. Your dedication to learning and improving is commendable! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

("title", models.CharField(max_length=255)),
(
"actors",
models.ManyToManyField(related_name="actors", to="db.actor"),

Choose a reason for hiding this comment

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

The related_name for the actors field is set to "actors", which might conflict with Django's default related names. Consider using a more descriptive name like "movies_as_actor".

),
(
"genres",
models.ManyToManyField(related_name="genres", to="db.genre"),

Choose a reason for hiding this comment

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

The related_name for the genres field is set to "genres", which might conflict with Django's default related names. Consider using a more descriptive name like "movies_as_genre".

hall_name: str,
hall_rows: int,
hall_seats_in_row: int
) -> str:

Choose a reason for hiding this comment

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

The return type annotation for create_cinema_hall is incorrect. It should return an instance of CinemaHall, not str. Update the return type annotation to CinemaHall.

from db.models import Genre, Actor, Movie


def get_movies(genres_ids: list = None, actors_ids: list = None) -> str:

Choose a reason for hiding this comment

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

The return type annotation for get_movies is incorrect. It should return a QuerySet of Movie instances, not str. Update the return type annotation accordingly.

return query.distinct()


def get_movie_by_id(movie_id: int) -> str:

Choose a reason for hiding this comment

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

The return type annotation for get_movie_by_id is incorrect. It should return an instance of Movie, not str. Update the return type annotation accordingly.

movie=movie,
)

def get_movies_sessions(session_date: str = None) -> str:

Choose a reason for hiding this comment

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

The return type annotation for get_movies_sessions is incorrect. It should return a QuerySet of MovieSession instances, not str. Update the return type annotation accordingly.

return query


def get_movie_session_by_id(movie_session_id: int) -> str:

Choose a reason for hiding this comment

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

The return type annotation for get_movie_session_by_id is incorrect. It should return an instance of MovieSession, not str. Update the return type annotation accordingly.

show_time: str = None,
movie_id: int = None,
cinema_hall_id: int = None
) -> str:

Choose a reason for hiding this comment

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

The return type annotation for update_movie_session is incorrect. It should return an instance of MovieSession, not str. Update the return type annotation accordingly.



def create_movie_session(
movie_show_time: str,

Choose a reason for hiding this comment

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

The movie_show_time parameter should be of type datetime instead of str to match the DateTimeField in the MovieSession model.


def update_movie_session(
session_id: int,
show_time: str = None,

Choose a reason for hiding this comment

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

The show_time parameter should be of type datetime instead of str to match the DateTimeField in the MovieSession model.

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.

2 participants