Cria em Journal os métodos add_publisher, add_owner, add_sponsor e add_copyright_holder#1270
Conversation
…elo Journal - Adiciona métodos helper (add_publisher, add_owner, add_sponsor, add_copyright_holder) ao model Journal
… journals - 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 Journal model to encapsulate institution creation logic by adding four new methods: add_publisher, add_owner, add_sponsor, and add_copyright_holder. The refactoring aims to consolidate repeated code patterns in the migration script am_to_core.py and follow Django best practices by moving business logic into model methods.
Changes:
- Added four new methods to the
Journalmodel for managing institution history - Updated
am_to_core.pyto use the new methods instead of directly creating institution history records - Added a new helper function
create_location_and_add_institutions(though it's not currently used) - Removed direct imports of institution models from
am_to_core.py
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| journal/models.py | Added four new methods (add_publisher, add_owner, add_sponsor, add_copyright_holder) to encapsulate institution history creation logic; includes formatting changes to imports and help text strings |
| journal/sources/am_to_core.py | Replaced direct institution history creation with calls to new Journal methods; added new helper function create_location_and_add_institutions; removed institution model imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
journal/models.py
Outdated
| sponsor_history = SponsorHistory.get_or_create( | ||
| institution=created_sponsor, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) |
There was a problem hiding this comment.
The SponsorHistory.get_or_create() method (inherited from BaseHistoryItem) doesn't accept an organization parameter. This will cause a TypeError at runtime.
journal/models.py
Outdated
| if not created_publisher and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") |
There was a problem hiding this comment.
The validation logic is problematic. If a caller provides organization without original_data, the method creates created_publisher=None, then passes it to PublisherHistory.get_or_create(institution=None, ...). This will cause a ValueError from BaseHistoryItem.get() at line 441: "Requires institution and initial_date or final_dateparameters". The ValueError message also has a typo (missing space: "final_dateparameters").
Consider validating that at least one of original_data or organization is provided at the beginning of the method, before attempting to create anything. The same issue exists in all four add methods.
journal/models.py
Outdated
| def add_publisher( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona publisher usando PublisherHistory.""" | ||
| created_publisher = None | ||
| if original_data: | ||
| # Cria/busca o Publisher baseado nos dados originais | ||
| created_publisher = Publisher.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| publisher_history.journal = self | ||
| publisher_history.save() | ||
| return publisher_history | ||
|
|
||
| def add_owner( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona owner usando OwnerHistory.""" | ||
| created_owner = None | ||
| if original_data: | ||
| # Cria/busca o Owner baseado nos dados originais | ||
| created_owner = Owner.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| owner_history.journal = self | ||
| owner_history.save() | ||
| return owner_history | ||
|
|
||
| def add_sponsor( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona sponsor usando SponsorHistory.""" | ||
| created_sponsor = None | ||
| if original_data: | ||
| # Cria/busca o Sponsor baseado nos dados originais | ||
| created_sponsor = Sponsor.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| sponsor_history.journal = self | ||
| sponsor_history.save() | ||
| return sponsor_history | ||
|
|
||
| def add_copyright_holder( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona copyright_holder usando CopyrightHolderHistory.""" | ||
| created_copyright_holder = None | ||
| if original_data: | ||
| # Cria/busca o CopyrightHolder baseado nos dados originais | ||
| created_copyright_holder = CopyrightHolder.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| if not created_copyright_holder and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") | ||
|
|
||
| # Cria/busca o CopyrightHolderHistory | ||
| copyright_holder_history = CopyrightHolderHistory.get_or_create( | ||
| institution=created_copyright_holder, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| copyright_holder_history.journal = self | ||
| copyright_holder_history.save() | ||
| return copyright_holder_history |
There was a problem hiding this comment.
These four methods (add_publisher, add_owner, add_sponsor, add_copyright_holder) contain nearly identical logic with only the class names differing (Publisher vs Owner vs Sponsor vs CopyrightHolder, and their corresponding History classes). Consider refactoring to reduce code duplication, for example by creating a generic helper method that accepts the institution class and history class as parameters. This would make the code more maintainable and reduce the risk of inconsistent behavior if changes are needed in the future.
| _("""Description of how authors contributions should be specified. | ||
| Does it use any taxonomy? If yes, which one? | ||
| Does the article text explicitly state the authors contributions? | ||
| Preferably, use the CREDiT taxonomy structure: <a target='_blank' | ||
| href='https://casrai.org/credit/'>https://casrai.org/credit/</a> | ||
| """ | ||
| ) | ||
| """) |
There was a problem hiding this comment.
This formatting change compresses a multi-line help_text string into a single long line, making it harder to read. The previous format was more readable. Consider reverting this change for consistency and readability.
journal/models.py
Outdated
| copyright_holder_history = CopyrightHolderHistory.get_or_create( | ||
| institution=created_copyright_holder, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) |
There was a problem hiding this comment.
The CopyrightHolderHistory.get_or_create() method (inherited from BaseHistoryItem) doesn't accept an organization parameter. This will cause a TypeError at runtime.
| def create_location_and_add_institutions( | ||
| journal, | ||
| publisher=None, | ||
| copyright_holder=None, | ||
| sponsor=None, | ||
| address=None, | ||
| publisher_country=None, | ||
| publisher_state=None, | ||
| publisher_city=None, | ||
| user=None, | ||
| ): | ||
| """ | ||
| Função centralizada para criar location e adicionar todas as instituições ao journal. | ||
|
|
||
| Args: | ||
| journal: O objeto Journal | ||
| publisher: Dados do publisher (string ou lista) | ||
| copyright_holder: Dados do copyright holder | ||
| sponsor: Dados do sponsor (string ou lista) | ||
| address: Endereço para criação do location | ||
| publisher_country: País do publisher | ||
| publisher_state: Estado do publisher | ||
| publisher_city: Cidade do publisher | ||
| user: Usuário responsável pela criação | ||
|
|
||
| Returns: | ||
| location: Objeto Location criado/atualizado | ||
| """ | ||
| if not user: | ||
| raise ValueError("User parameter is required") | ||
|
|
||
| # Cria ou atualiza o location | ||
| location = create_or_update_location( | ||
| journal=journal, | ||
| address=address, | ||
| publisher_country=publisher_country, | ||
| publisher_state=publisher_state, | ||
| publisher_city=publisher_city, | ||
| user=user, | ||
| ) | ||
|
|
||
| # Adiciona publisher(s) e owner(s) se fornecido | ||
| if publisher: | ||
| publisher_data = extract_value(publisher) | ||
| if isinstance(publisher_data, str): | ||
| publisher_data = re.split(r"\s*[-\/,]\s*", publisher_data) | ||
|
|
||
| if publisher_data: | ||
| for p in publisher_data: | ||
| if p: | ||
| # Adiciona publisher | ||
| journal.add_publisher( | ||
| user=user, | ||
| original_data=p, | ||
| location=location, | ||
| ) | ||
| # Adiciona owner (mesmo dado do publisher por padrão) | ||
| journal.add_owner( | ||
| user=user, | ||
| original_data=p, | ||
| location=location, | ||
| ) | ||
|
|
||
| # Adiciona sponsor(s) se fornecido | ||
| if sponsor: | ||
| get_or_create_sponsor( | ||
| journal=journal, sponsor=sponsor, user=user, location=location | ||
| ) | ||
|
|
||
| # Adiciona copyright holder(s) se fornecido | ||
| if copyright_holder: | ||
| get_or_create_copyright_holder( | ||
| journal=journal, | ||
| copyright_holder_name=copyright_holder, | ||
| user=user, | ||
| location=location, | ||
| ) | ||
|
|
||
| return location |
There was a problem hiding this comment.
The function create_location_and_add_institutions is defined but doesn't appear to be called anywhere in the codebase. If this function is intended for future use, consider adding a comment explaining this. If it's dead code, consider removing it to keep the codebase clean. If it's meant to replace existing code patterns, ensure it's actually being used where intended.
| help_text=_("""Describe the policy used by the journal on copyright issues. | ||
| We recommend that this section be in accordance with the recommendations of the SciELO criteria, | ||
| item 5.2.10.1.2. - Copyright""" | ||
| ), | ||
| item 5.2.10.1.2. - Copyright"""), |
There was a problem hiding this comment.
This formatting change compresses a multi-line help_text string into a single very long line (spanning lines 1562-1564), making it harder to read. The previous format with properly indented lines was more readable. Consider reverting this change to maintain code readability, especially since similar help_text strings in the codebase use multiple lines (see lines 1515-1516, 1549-1551, 1576-1577).
journal/models.py
Outdated
| def add_publisher( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona publisher usando PublisherHistory.""" | ||
| created_publisher = None | ||
| if original_data: | ||
| # Cria/busca o Publisher baseado nos dados originais | ||
| created_publisher = Publisher.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| publisher_history.journal = self | ||
| publisher_history.save() | ||
| return publisher_history | ||
|
|
||
| def add_owner( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona owner usando OwnerHistory.""" | ||
| created_owner = None | ||
| if original_data: | ||
| # Cria/busca o Owner baseado nos dados originais | ||
| created_owner = Owner.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| owner_history.journal = self | ||
| owner_history.save() | ||
| return owner_history | ||
|
|
||
| def add_sponsor( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona sponsor usando SponsorHistory.""" | ||
| created_sponsor = None | ||
| if original_data: | ||
| # Cria/busca o Sponsor baseado nos dados originais | ||
| created_sponsor = Sponsor.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| sponsor_history.journal = self | ||
| sponsor_history.save() | ||
| return sponsor_history | ||
|
|
||
| def add_copyright_holder( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona copyright_holder usando CopyrightHolderHistory.""" | ||
| created_copyright_holder = None | ||
| if original_data: | ||
| # Cria/busca o CopyrightHolder baseado nos dados originais | ||
| created_copyright_holder = CopyrightHolder.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| if not created_copyright_holder and not organization: | ||
| raise ValueError("Either original_data or organization must be provided") | ||
|
|
||
| # Cria/busca o CopyrightHolderHistory | ||
| copyright_holder_history = CopyrightHolderHistory.get_or_create( | ||
| institution=created_copyright_holder, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| copyright_holder_history.journal = self | ||
| copyright_holder_history.save() | ||
| return copyright_holder_history |
There was a problem hiding this comment.
The new methods add_publisher, add_owner, add_sponsor, and add_copyright_holder lack test coverage. Since these are public methods that encapsulate important business logic and could be called from various parts of the codebase, they should have unit tests to verify:
- Successful creation with valid
original_data - Successful creation with valid
organization - Proper error handling when neither
original_datanororganizationis provided - Proper association with the journal and user
- Correct handling of dates and location parameters
Consider adding tests in journal/tests.py for these methods.
journal/models.py
Outdated
| publisher_history = PublisherHistory.get_or_create( | ||
| institution=created_publisher, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) |
There was a problem hiding this comment.
The PublisherHistory.get_or_create() method (inherited from BaseHistoryItem at institution/models.py:450) only accepts parameters institution, initial_date, final_date, and user. However, this code is passing an organization parameter which is not supported. This will cause a TypeError at runtime: "get_or_create() got an unexpected keyword argument 'organization'".
The same issue exists in the add_owner, add_sponsor, and add_copyright_holder methods. Consider either:
- Overriding the
get_or_createmethod in each History class to support the organization parameter, or - Creating the History object directly without using
get_or_create, then setting the organization field afterwards.
journal/models.py
Outdated
| owner_history = OwnerHistory.get_or_create( | ||
| institution=created_owner, | ||
| organization=organization, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) |
There was a problem hiding this comment.
The OwnerHistory.get_or_create() method (inherited from BaseHistoryItem) doesn't accept an organization parameter. This will cause a TypeError at runtime. The same issue exists in add_publisher, add_sponsor, and add_copyright_holder methods.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
journal/models.py
Outdated
| def add_publisher( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona publisher usando PublisherHistory.""" | ||
| created_publisher = None | ||
| if original_data: | ||
| # Cria/busca o Publisher baseado nos dados originais | ||
| created_publisher = Publisher.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| publisher_history.journal = self | ||
| publisher_history.organization = organization | ||
| publisher_history.save() | ||
| return publisher_history |
There was a problem hiding this comment.
The new add_publisher method encapsulates important business logic for adding publishers to journals, but lacks test coverage. Since the journal tests.py file contains test cases for other journal functionality, consider adding tests for this method to ensure:
- Publishers are correctly created/retrieved with the provided data
- PublisherHistory entries are properly associated with the journal
- The organization parameter is correctly handled
- Edge cases (e.g., duplicate entries, missing user parameter) are handled appropriately
journal/models.py
Outdated
| def add_owner( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona owner usando OwnerHistory.""" | ||
| created_owner = None | ||
| if original_data: | ||
| # Cria/busca o Owner baseado nos dados originais | ||
| created_owner = Owner.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| owner_history.journal = self | ||
| owner_history.organization = organization | ||
| owner_history.save() | ||
| return owner_history |
There was a problem hiding this comment.
The new add_owner method encapsulates important business logic for adding owners to journals, but lacks test coverage. Since the journal tests.py file contains test cases for other journal functionality, consider adding tests for this method to ensure:
- Owners are correctly created/retrieved with the provided data
- OwnerHistory entries are properly associated with the journal
- The organization parameter is correctly handled
- Edge cases (e.g., duplicate entries, missing user parameter) are handled appropriately
journal/models.py
Outdated
| 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, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| owner_history.journal = self | ||
| owner_history.organization = organization | ||
| owner_history.save() |
There was a problem hiding this comment.
The logic for handling the organization parameter is unclear and potentially buggy. When organization is provided without original_data, the validation passes but created_owner will be None, resulting in an OwnerHistory with institution=None.
The code should either:
- Validate that at least one of
original_dataororganizationcreates a valid institution reference before proceeding, or - Document that it's valid to have
institution=Nonewhen onlyorganizationis provided (for cases where there's no legacy data but a modern organization reference exists).
If option 2 is intended, consider renaming the validation to make this clearer. If option 1 is intended, the condition should prevent calling get_or_create with institution=None when that's not the desired behavior.
| ): | ||
| """ | ||
| Função centralizada para criar location e adicionar todas as instituições ao journal. | ||
|
|
||
| Args: | ||
| journal: O objeto Journal | ||
| publisher: Dados do publisher (string ou lista) | ||
| copyright_holder: Dados do copyright holder | ||
| sponsor: Dados do sponsor (string ou lista) | ||
| address: Endereço para criação do location | ||
| publisher_country: País do publisher | ||
| publisher_state: Estado do publisher | ||
| publisher_city: Cidade do publisher | ||
| user: Usuário responsável pela criação | ||
|
|
||
| Returns: | ||
| location: Objeto Location criado/atualizado | ||
| """ | ||
| if not user: | ||
| raise ValueError("User parameter is required") | ||
|
|
||
| # Cria ou atualiza o location | ||
| location = create_or_update_location( | ||
| journal=journal, | ||
| address=address, | ||
| publisher_country=publisher_country, | ||
| publisher_state=publisher_state, | ||
| publisher_city=publisher_city, | ||
| user=user, | ||
| ) | ||
|
|
||
| # Adiciona publisher(s) e owner(s) se fornecido | ||
| if publisher: | ||
| publisher_data = extract_value(publisher) | ||
| if isinstance(publisher_data, str): | ||
| publisher_data = re.split(r"\s*[-\/,]\s*", publisher_data) | ||
|
|
||
| if publisher_data: | ||
| for p in publisher_data: | ||
| if p: | ||
| # Adiciona publisher | ||
| journal.add_publisher( | ||
| user=user, | ||
| original_data=p, | ||
| location=location, | ||
| ) | ||
| # Adiciona owner (mesmo dado do publisher por padrão) | ||
| journal.add_owner( | ||
| user=user, | ||
| original_data=p, | ||
| location=location, | ||
| ) | ||
|
|
||
| # Adiciona sponsor(s) se fornecido | ||
| if sponsor: | ||
| get_or_create_sponsor( | ||
| journal=journal, sponsor=sponsor, user=user, location=location | ||
| ) | ||
|
|
||
| # Adiciona copyright holder(s) se fornecido | ||
| if copyright_holder: | ||
| get_or_create_copyright_holder( | ||
| journal=journal, | ||
| copyright_holder_name=copyright_holder, | ||
| user=user, | ||
| location=location, | ||
| ) | ||
|
|
||
| return location |
There was a problem hiding this comment.
This new function create_location_and_add_institutions is defined but never called anywhere in the codebase. It appears to duplicate the functionality of update_panel_institution (lines 186-250) but with a slightly different interface.
If this function is intended for future use, consider adding a comment explaining its purpose and planned usage. If it's meant to replace update_panel_institution, the refactoring should be completed. Otherwise, this dead code should be removed to avoid confusion and maintenance burden.
| ): | |
| """ | |
| Função centralizada para criar location e adicionar todas as instituições ao journal. | |
| Args: | |
| journal: O objeto Journal | |
| publisher: Dados do publisher (string ou lista) | |
| copyright_holder: Dados do copyright holder | |
| sponsor: Dados do sponsor (string ou lista) | |
| address: Endereço para criação do location | |
| publisher_country: País do publisher | |
| publisher_state: Estado do publisher | |
| publisher_city: Cidade do publisher | |
| user: Usuário responsável pela criação | |
| Returns: | |
| location: Objeto Location criado/atualizado | |
| """ | |
| if not user: | |
| raise ValueError("User parameter is required") | |
| # Cria ou atualiza o location | |
| location = create_or_update_location( | |
| journal=journal, | |
| address=address, | |
| publisher_country=publisher_country, | |
| publisher_state=publisher_state, | |
| publisher_city=publisher_city, | |
| user=user, | |
| ) | |
| # Adiciona publisher(s) e owner(s) se fornecido | |
| if publisher: | |
| publisher_data = extract_value(publisher) | |
| if isinstance(publisher_data, str): | |
| publisher_data = re.split(r"\s*[-\/,]\s*", publisher_data) | |
| if publisher_data: | |
| for p in publisher_data: | |
| if p: | |
| # Adiciona publisher | |
| journal.add_publisher( | |
| user=user, | |
| original_data=p, | |
| location=location, | |
| ) | |
| # Adiciona owner (mesmo dado do publisher por padrão) | |
| journal.add_owner( | |
| user=user, | |
| original_data=p, | |
| location=location, | |
| ) | |
| # Adiciona sponsor(s) se fornecido | |
| if sponsor: | |
| get_or_create_sponsor( | |
| journal=journal, sponsor=sponsor, user=user, location=location | |
| ) | |
| # Adiciona copyright holder(s) se fornecido | |
| if copyright_holder: | |
| get_or_create_copyright_holder( | |
| journal=journal, | |
| copyright_holder_name=copyright_holder, | |
| user=user, | |
| location=location, | |
| ) | |
| return location |
journal/models.py
Outdated
| def add_sponsor( | ||
| self, | ||
| user, | ||
| organization=None, | ||
| original_data=None, | ||
| initial_date=None, | ||
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona sponsor usando SponsorHistory.""" | ||
| created_sponsor = None | ||
| if original_data: | ||
| # Cria/busca o Sponsor baseado nos dados originais | ||
| created_sponsor = Sponsor.get_or_create( | ||
| name=original_data, | ||
| acronym=None, | ||
| level_1=None, | ||
| level_2=None, | ||
| level_3=None, | ||
| user=user, | ||
| location=location, | ||
| official=None, | ||
| is_official=None, | ||
| url=None, | ||
| institution_type=None, | ||
| ) | ||
| 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, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| sponsor_history.journal = self | ||
| sponsor_history.organization = organization | ||
| sponsor_history.save() | ||
| return sponsor_history |
There was a problem hiding this comment.
The new add_sponsor method encapsulates important business logic for adding sponsors to journals, but lacks test coverage. Since the journal tests.py file contains test cases for other journal functionality, consider adding tests for this method to ensure:
- Sponsors are correctly created/retrieved with the provided data
- SponsorHistory entries are properly associated with the journal
- The organization parameter is correctly handled
- Edge cases (e.g., duplicate entries, missing user parameter) are handled appropriately
journal/models.py
Outdated
| 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, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| publisher_history.journal = self | ||
| publisher_history.organization = organization | ||
| publisher_history.save() |
There was a problem hiding this comment.
The logic for handling the organization parameter is unclear and potentially buggy. When organization is provided without original_data, the validation passes but created_publisher will be None, resulting in a PublisherHistory with institution=None.
The code should either:
- Validate that at least one of
original_dataororganizationcreates a valid institution reference before proceeding, or - Document that it's valid to have
institution=Nonewhen onlyorganizationis provided (for cases where there's no legacy data but a modern organization reference exists).
If option 2 is intended, consider renaming the validation to make this clearer. If option 1 is intended, the condition should prevent calling get_or_create with institution=None when that's not the desired behavior.
journal/models.py
Outdated
| 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, | ||
| initial_date=initial_date, | ||
| final_date=final_date, | ||
| user=user, | ||
| ) | ||
| sponsor_history.journal = self | ||
| sponsor_history.organization = organization | ||
| sponsor_history.save() |
There was a problem hiding this comment.
The logic for handling the organization parameter is unclear and potentially buggy. When organization is provided without original_data, the validation passes but created_sponsor will be None, resulting in a SponsorHistory with institution=None.
The code should either:
- Validate that at least one of
original_dataororganizationcreates a valid institution reference before proceeding, or - Document that it's valid to have
institution=Nonewhen onlyorganizationis provided (for cases where there's no legacy data but a modern organization reference exists).
If option 2 is intended, consider renaming the validation to make this clearer. If option 1 is intended, the condition should prevent calling get_or_create with institution=None when that's not the desired behavior.
- Refactored Journal model to reduce code duplication in history methods
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final_date=None, | ||
| location=None, | ||
| ): | ||
| """Adiciona instituição usando InstitutionHistory genérico.""" |
There was a problem hiding this comment.
The docstring says "Adiciona instituição usando InstitutionHistory genérico" (Adds institution using generic InstitutionHistory) but this is vague and doesn't explain the parameters, return value, or the behavior when only organization vs only original_data is provided. Consider adding more detailed documentation about the parameters, expected behavior, and return value to help future maintainers understand this generic helper method.
| """Adiciona instituição usando InstitutionHistory genérico.""" | |
| """ | |
| Add an institution history entry for this journal using generic models. | |
| This helper creates or retrieves an institution instance (via | |
| ``institution_class``) based on the ``original_data`` provided and then | |
| creates or retrieves a corresponding history record (via | |
| ``history_class``). The history record is associated with this | |
| :class:`Journal` instance and, optionally, with an | |
| :class:`organization.models.Organization`. | |
| At least one of ``original_data`` or ``organization`` must be provided. | |
| If only ``original_data`` is provided, an institution is created/located | |
| and linked to the history, with ``organization`` left unset. | |
| If only ``organization`` is provided, the history is created/located | |
| with ``institution`` set to ``None`` and only the ``organization`` | |
| field is populated. | |
| Parameters | |
| ---------- | |
| institution_class : | |
| Model or manager that exposes a ``get_or_create`` method with the | |
| signature used below to obtain an institution instance. | |
| history_class : | |
| Model or manager that exposes a ``get_or_create`` method accepting | |
| ``institution``, ``initial_date``, ``final_date`` and ``user`` to | |
| obtain an institution history instance. | |
| user : | |
| User instance associated with the creation/update of the records. | |
| original_data : str, optional | |
| Original textual representation of the institution name. When | |
| provided, it is used to create or retrieve an institution via | |
| ``institution_class``. | |
| organization : Organization, optional | |
| Organization instance to associate with the created history record. | |
| initial_date : datetime.date, optional | |
| Initial date for the institution history period. | |
| final_date : datetime.date, optional | |
| Final date for the institution history period. | |
| location : Location, optional | |
| Location to associate with the institution when it is created from | |
| ``original_data``. | |
| Returns | |
| ------- | |
| history_class instance | |
| The created or retrieved institution history object, already linked | |
| to this journal and saved. | |
| Raises | |
| ------ | |
| ValueError | |
| If neither ``original_data`` nor ``organization`` is provided. | |
| """ |
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