refactor(journal): encapsular lógica de adição de instituições no mod… #1269
Conversation
…elo Journal - Adiciona métodos helper (add_publisher, add_owner, add_sponsor, add_copyright_holder) ao model Journal - Refatora am_to_core.py para utilizar os novos métodos, reduzindo a duplicação de código - Centraliza a criação de registros de histórico de instituições
There was a problem hiding this comment.
Pull request overview
This PR refactors the institution addition logic in the journal module by encapsulating repetitive code into helper methods on the Journal model. The refactoring aims to reduce code duplication and centralize the creation of institution history records.
Changes:
- Added four helper methods (
add_publisher,add_owner,add_sponsor,add_copyright_holder) to the Journal model - Refactored
am_to_core.pyto use these new methods instead of directly calling institution models - Removed unused import of institution models from
am_to_core.py
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| journal/models.py | Added four new helper methods to the Journal model for adding publisher, owner, sponsor, and copyright holder with their respective history records |
| journal/sources/am_to_core.py | Removed direct institution creation code and replaced with calls to the new Journal helper methods; removed unused imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| publisher_history = PublisherHistory.get_or_create( | ||
| institution=created_publisher, | ||
| # Usa novo método add_owner ao invés de OwnerHistory |
There was a problem hiding this comment.
The comment has trailing whitespace at the end of the line. Remove the trailing spaces for consistency with code style standards.
| # Usa novo método add_owner ao invés de OwnerHistory | |
| # Usa novo método add_owner ao invés de OwnerHistory |
| if not created_publisher and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") | ||
|
|
||
| # Cria/busca o PublisherHistory | ||
| publisher_history = PublisherHistory.get_or_create( | ||
| institution=created_publisher, |
There was a problem hiding this comment.
The organization parameter is never used in this method. When original_data is None but organization is provided, the validation will pass but created_publisher will remain None, causing institution=None to be passed to PublisherHistory.get_or_create(). Either remove the unused organization parameter or implement the logic to use it when provided.
| if not created_owner and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") | ||
|
|
||
| # Cria/busca o OwnerHistory | ||
| owner_history = OwnerHistory.get_or_create( | ||
| institution=created_owner, |
There was a problem hiding this comment.
The organization parameter is never used in this method. When original_data is None but organization is provided, the validation will pass but created_owner will remain None, causing institution=None to be passed to OwnerHistory.get_or_create(). Either remove the unused organization parameter or implement the logic to use it when provided.
| if not created_sponsor and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") | ||
|
|
||
| # Cria/busca o SponsorHistory | ||
| sponsor_history = SponsorHistory.get_or_create( | ||
| institution=created_sponsor, |
There was a problem hiding this comment.
The organization parameter is never used in this method. When original_data is None but organization is provided, the validation will pass but created_sponsor will remain None, causing institution=None to be passed to SponsorHistory.get_or_create(). Either remove the unused organization parameter or implement the logic to use it when provided.
| if not created_copyright_holder and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") | ||
|
|
There was a problem hiding this comment.
The organization parameter is never used in this method. When original_data is None but organization is provided, the validation will pass but created_copyright_holder will remain None, causing institution=None to be passed to CopyrightHolderHistory.get_or_create(). Either remove the unused organization parameter or implement the logic to use it when provided.
| if not created_copyright_holder and not organization: | |
| raise ValueError("Either original_data or organization must be provided") | |
| # Se nenhum CopyrightHolder foi criado/encontrado, não é seguro continuar. | |
| if not created_copyright_holder: | |
| if organization is not None: | |
| # Ainda não há lógica para derivar um CopyrightHolder a partir de organization. | |
| # Evita criar um histórico com institution=None. | |
| raise ValueError( | |
| "original_data must be provided to create a CopyrightHolder " | |
| "when organization is used." | |
| ) | |
| raise ValueError("Either original_data or organization must be provided") |
journal/models.py
Outdated
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=None, # Pode ser ajustado conforme necessário |
There was a problem hiding this comment.
The refactoring changed the behavior: the old code passed the location variable (created on line 199) to Publisher.get_or_create(), but the new add_publisher() method hardcodes location=None. This may result in data loss if location information was previously being stored. Consider adding a location parameter to add_publisher() and passing it through, or verify this change is intentional.
journal/models.py
Outdated
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=None, # Pode ser ajustado conforme necessário |
There was a problem hiding this comment.
The refactoring changed the behavior: the old code passed the location variable to Owner.get_or_create(), but the new add_owner() method hardcodes location=None. This may result in data loss if location information was previously being stored. Consider adding a location parameter to add_owner() and passing it through, or verify this change is intentional.
- Atualiza métodos add_publisher, add_owner, add_sponsor e add_copyright_holder para aceitarem o parâmetro location. - Atualiza funções auxiliares em am_to_core.py para propagar o objeto location. - Adiciona a função centralizada create_location_and_add_institutions para facilitar a criação de registros vinculados.
1 participant
O que esse PR faz?
Este PR realiza uma refatoração no modelo
Journalpara encapsular a lógica de criação e associação de instituições vinculadas ao histórico do periódico (Publishers, Owners, Sponsors e Copyright Holders).As principais mudanças incluem:
add_publisher,add_owner,add_sponsoreadd_copyright_holderdiretamente na classeJournal.am_to_core.py, substituindo blocos repetitivos de código por chamadas simplificadas aos novos métodos do modelo.Onde a revisão poderia começar?
A revisão deve começar pelo arquivo
journal/models.py, onde os novos métodos foram implementados para entender a lógica de abstração. Em seguida, verifiquejournal/sources/am_to_core.pypara validar como o código ficou mais enxuto após a substituição das chamadas diretas aos modelos de instituição.Como este poderia ser testado manualmente?
am_to_core.py).PublisherHistory,SponsorHistory, etc.) foram gerados e vinculados aoJournale aoUsercorretos.journal_instance.add_publisher(user=user, original_data="Nome da Editora").Algum cenário de contexto que queira dar?
Anteriormente, a lógica de instanciar uma instituição e logo em seguida criar seu registro de histórico estava espalhada por diversos pontos dos scripts de conversão. Isso gerava código duplicado e dificultava a manutenção caso o esquema do histórico mudasse. Ao mover essa lógica para o
Journal, tornamos o código mais "Django-like" e facilitamos a reutilização em outras partes do sistema.Screenshots
N/A (Alteração de backend/lógica de modelos)
Quais são tickets relevantes?
Referências