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

🐛 fix query get duplicate movie_ids #98

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

Conversation

Gpocas
Copy link
Contributor

@Gpocas Gpocas commented Oct 14, 2024

outra tentativa de resolver a issue #75, dessa vez implementei a query de forma diferente.

Como a intenção da pagina de posters é exibir apenas a url da imagem, não fazia muito sentido fazer o select em movie e depois o join com Screening sendo que todas as informações que precisamos já estão em uma unica tabela, mudei a query para realizar select diretamente na model Screening e fiz uma subquery para selecionar o maior screening_id agrupando por movie_id, fazendo com que o movie_id apareça apenas uma vez.

Essa solução não vai cobrir os casos em que exista o mesmo posters com movie_id e url diferentes,

@guites
Copy link
Collaborator

guites commented Oct 14, 2024

@Gpocas ótima solução! não tive tempo de testar ainda, mas fiquei pensando se não seria melhor ignorar se os screenings pertencem a um mesmo filme ou não, e só puxar os image_url distintos.

Afinal um mesmo filme pode ter duas screenings com imagens diferentes.

Assim que eu testar aqui te dou um feedback melhor. Valeu!

Copy link
Collaborator

@guites guites left a comment

Choose a reason for hiding this comment

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

O PR resolve o problema proposto, apenas deixei uma proposta de alteração antes de aprovar.

Comment on lines +29 to +37
query = (
db_session.query(Screening)
.filter(
Screening.id.in_(
db_session.query(func.max(Screening.id)).group_by(Screening.movie_id)
)
)
.order_by(Screening.id.desc())
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

tchê eu acho que esse .filter aqui pode ser removida por dois motivos:

  1. Ela deixa o query meio complexo, e talvez conforme a quantidade de filmes aumente, acabe tornando o endpoint mais lento. É interessante que aqui seja o mais rápido possível pq tem impacto na fluidez da página dos posters. (admito que não sei dizer se deixa lento ou não, só tô chutando).
  2. Esse filtro só permite uma screening por filme. Acontece que muitos filmes passam em mais de um cinema. Um exemplo disso é o filme "Motel Destino". Ele tem uma screening na Cinemateca Paulo Amorim, com esse poster: https://i.ibb.co/hcBbc5N/ed26e96a9936.png e outra screening no CineBancários, com esse poster: https://i.ibb.co/wKBzhLD/f346e77a6585.webp.

Eu testei aqui e essa solução, com ou sem o filtro, resolve o problema anterior onde as screenings vinham repetidas pra diferentes valores de paginação (current_page).

Suggested change
query = (
db_session.query(Screening)
.filter(
Screening.id.in_(
db_session.query(func.max(Screening.id)).group_by(Screening.movie_id)
)
)
.order_by(Screening.id.desc())
)
query = (
db_session.query(Screening)
.order_by(Screening.id.desc())
)

Que tal removermos o filtro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

quando eu estava desenvolvendo eu até pensei nesse segundo caso que você comentou sobre o filme "Motel Destino" ter dois posters e essa modificação iria exibir apenas 1 deles, porem na estrutura do projeto atual eu não consegui pensar em nenhum outro jeito, na minha visão eu acho melhor omitir alguns filmes do que continuar repetindo, depois podemos pensar em uma solução mais robusta.

flask_backend/routes/movie.py Show resolved Hide resolved
except ValueError:
abort(400)

user_logged_in = g.user is not None
movies = get_paginated(page, limit, user_logged_in)
scrennigs_list = get_paginated(page, limit, user_logged_in)
Copy link
Collaborator

@guites guites Oct 15, 2024

Choose a reason for hiding this comment

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

aqui tem um pequeno typo, acho que é screenings_list:

Suggested change
scrennigs_list = get_paginated(page, limit, user_logged_in)
scrennings_list = get_paginated(page, limit, user_logged_in)

(daí tem que mudar ali embaixo nos outros lugares onde é utilizado).

@guites guites linked an issue Oct 15, 2024 that may be closed by this pull request
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.

Tela de posters mostra alguns filmes repetidos
2 participants