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

Youssef2 #8

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

Youssef2 #8

wants to merge 14 commits into from

Conversation

Ewid
Copy link

@Ewid Ewid commented Jul 24, 2024

Controllers to handle the http requests

Created the Movie entity and then updated for the second user story to include additional fields and relationships:

Added duration, overview, language, and genres fields.
Added relationships for director, writer, and actors.

Used the initial Director entity to manage both directors and writers by adding a type field (director or writer).

Updated the MovieService to handle all the new relationships and fields

Updated the Movie_actor entity to have a relationship with Movie_award to link the Actor Nominations with the movie and actor and the award.

Created Movie_award entity to give a few attributes to the award and the relationships with the movie_actor to link them together

Ewid added 8 commits July 7, 2024 15:47
migration and .env and search
Creation of Tables and their services, and controllers
Creation of festival table and movie_festival junction table
Modified the date formatting to show only the year instead of the whole timestamp for the Movie on the frontend
Added Genre, Overview, Language, Duration and writer relation to movie entity

Added type to director entity to make the director entity either writer or director and updated dtos accordingly for both.
created the writer type in the director entity and added it to the dtos
fix
@Ewid Ewid closed this Jul 24, 2024
@Ewid Ewid reopened this Jul 24, 2024
Copy link

@AlyMBarakat AlyMBarakat left a comment

Choose a reason for hiding this comment

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

Please add a description!!

@AlyMBarakat AlyMBarakat self-assigned this Jul 24, 2024
Copy link

@AlyMBarakat AlyMBarakat left a comment

Choose a reason for hiding this comment

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

Remove all unused controller/service methods, only leave endpoints that are covered in the stories

Comment on lines 27 to 31
@CreateDateColumn({ type: 'timestamptz' })
created_at: Date;

@UpdateDateColumn({ type: 'timestamptz' })
updated_at: Date;

Choose a reason for hiding this comment

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

U can create a class called BaseEntity that has created_at, updated_at and uuid then u can extend this class in all other entities u use instead of duplicating them on every entity

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, created base entity and extended it to all needed entities

Comment on lines +51 to +57
@ManyToOne(() => Director, director => director.movies)
@JoinColumn({ name: 'director_id' })
director: Director;

@ManyToOne(() => Director, writer => writer.movies, { nullable: true })
@JoinColumn({ name: 'writer_id' })
writer: Director;

Choose a reason for hiding this comment

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

How can a writer be of type Director, u need to change Director entity to movie worker/maker/cast, etc., something generic as now it has a type that identifies the role of this cast member

Copy link
Author

Choose a reason for hiding this comment

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

I can't fix this to be honest i tried to change it but i could not find all the imports to fix it

Comment on lines 65 to 70
@BeforeInsert()
generateUUID() {
if (!this.uuid) {
this.uuid = uuidv4();
}
}

Choose a reason for hiding this comment

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

Move this to the base entity too

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 48 to 49
@Column("text", { array: true, default: [] })
genres: string[];

Choose a reason for hiding this comment

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

Genres are usually not something unique that changes with every movie, we can store genre ids instead of recreating genres with every movie, right?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Choose a reason for hiding this comment

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

Why do we have, actor, writer and director, while they all share almost the same attributes, this causes a problem with movie awards as it's tied to actors now, while it's quite popular that an award can be best picture, best story, best screen-play, all of these awards are not given to actor, and we are not going to create a table for each person that might participate in a movie while they all share the same attributes, by the end, they are cast, or movie workers with different jobs

Choose a reason for hiding this comment

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

Let's discuss the change that needs to be done to unify the redundancy caused by this table

@Ewid Ewid requested a review from AlyMBarakat August 1, 2024 09:04
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