Skip to content

Conversation

@robertatakenaka
Copy link
Member

PR Title: Refactor Organization Data Structure and Normalize Institution Properties

Description

Este PR implementa uma refatoração significativa na forma como os dados de organizações (Publishers, Owners, Sponsors e Copyright Holders) são acessados e exibidos no sistema. O objetivo principal é desatolar a lógica complexa de busca de nomes em modelos aninhados, centralizando-os em propriedades padronizadas no modelo Journal.

As mudanças abrangem desde a camada de dados (Models) até a camada de apresentação (Templates e API), garantindo consistência na exibição dos nomes das instituições.

Key Changes

🏗️ Core & Models

  • Journal Model: Adição de métodos @property (owner_names, publisher_names, sponsors, copyright_holders) para encapsular a lógica de recuperação de nomes, tratando tanto o novo modelo de Organization quanto o legado.
  • Institution Mixins: Otimização das propriedades de localização e correção de tipos (acrônimo de país).

🔌 API & Formats

  • Serializers (V1): Atualização dos serializers de Issue e Journal para consumir as novas propriedades, simplificando o payload da API.
  • ArticleMeta Formatters: Refatoração dos formatadores para suportar a nova estrutura de dados de organizações durante a exportação.

🎨 UI & Search

  • Templates: Atualização das páginas de "About" e tabelas de periódicos para utilizar as propriedades achatadas (flattened), eliminando loops complexos e condicionais excessivas nos templates HTML.
  • Search Index: Ajuste no template de indexação do Solr para garantir que múltiplos publishers sejam indexados corretamente.

Commits Included

  • fbeb5c4: fix: update search index template for publishers
  • 01dea6b: refactor: update JournalPage context to use standardized sponsors property
  • bc33298: ui: update journal and publisher templates to use flattened organization name properties
  • 872c61d: refactor: update ArticleMeta formatters to handle new organization data structure
  • f45cb37: refactor: update Issue and Journal serializers to use new organization name properties
  • [hash_anterior]: feat: add property methods for owner, publisher, and sponsor names in Journal model

How to Test

  1. Acesse a página "Sobre" de um periódico e verifique se os nomes do Publisher e Patrocinadores aparecem corretamente.
  2. Valide o endpoint da API V1 de Journal/Issue e verifique se o campo publisher e sponsor retorna a lista de nomes esperada.
  3. Execute o comando de indexação do Solr e verifique se não há erros no template article_compile.txt.

Copilot AI review requested due to automatic review settings February 1, 2026 22:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors how organization data (Publishers, Owners, Sponsors, and Copyright Holders) is accessed throughout the application by introducing standardized property methods in the Journal model. The goal is to centralize and simplify the logic for retrieving organization names, supporting both the new Organization model and legacy Institution model.

Changes:

  • Added four property methods to the Journal model (owner_names, publisher_names, sponsors, copyright_holders) to encapsulate organization name retrieval logic
  • Updated API serializers (v1) to use the new properties, simplifying the payload structure
  • Refactored ArticleMeta formatters for both Journal and Issue to consume the new organization properties
  • Updated templates across journalpage and search indexes to use the flattened organization name properties

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
journal/models.py Adds property methods to Journal model for accessing organization names in a standardized way
journal/api/v1/serializers.py Updates serializers to use new property methods and adds helper for formatting institution names
issue/api/v1/serializers.py Simplifies publisher name retrieval in Issue serializer using journal's new property
journal/formats/articlemeta_format.py Refactors ArticleMeta formatter to use new organization properties instead of directly querying history
issue/formats/articlemeta_format.py Updates institution info formatting to use journal's standardized properties
journalpage/templates/journalpage/about.html Simplifies template to iterate over flattened organization names
journalpage/templates/journalpage/includes/journal_info.html Updates journal info display to use new publisher_names property
core/templates/home/include/list_journal_page/journals_table_by_publisher.html Updates publisher table to use institution_name property
article/templates/search/indexes/article/article_compile.txt Attempts to use publisher_names property on Article instead of Article.journal

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<i>{{ publisher.institution.institution.institution_identification.name}}</i>
{% endif %}
{% for publisher in journal.publisher_names %}
<i>{{ publisher }}</i>
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

When iterating over multiple publisher names, the template doesn't include any separator between them. If a journal has multiple publishers, they will be concatenated without spaces or commas, making them difficult to read. Consider adding a separator (e.g., using the {% if not forloop.last %}, {% endif %} pattern) or using the join filter.

Suggested change
<i>{{ publisher }}</i>
<i>{{ publisher }}</i>{% if not forloop.last %}, {% endif %}

Copilot uses AI. Check for mistakes.
Comment on lines 748 to 786
@property
def owner_names(self):
items = []
for item in self.owner_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items

@property
def publisher_names(self):
items = []
for item in self.publisher_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items

@property
def sponsors(self):
items = []
for item in self.sponsor_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items

@property
def copyright_holders(self):
items = []
for item in self.copyright_holder_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The property implementations don't handle exceptions when accessing item.organization.name. If accessing the organization name raises an exception (e.g., due to database issues or unexpected state), it will propagate and potentially break the calling code. Consider wrapping the organization name access in a try-except block similar to how institution_name property handles errors in the BaseHistoryItem class.

Copilot uses AI. Check for mistakes.
Comment on lines +135 to +136
if names:
return [{"name": name} for name in names]
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The format_institution_names method returns None implicitly when names is empty or falsy, which means the API response will have null values for publisher/owner/sponsor/copyright fields when there are no items. This is inconsistent with REST API conventions where empty collections should typically return empty arrays [] rather than null. Consider returning an empty list explicitly when names is empty to maintain consistency.

Suggested change
if names:
return [{"name": name} for name in names]
if not names:
return []
return [{"name": name} for name in names]

Copilot uses AI. Check for mistakes.
<element name="publisher">
<element name="none">
<field name="value">{{ object.publisher.institution.institution_identification.name }}</field>
{% for publisher_name in object.publisher_names %}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The template is trying to access object.publisher_names directly on the Article model, but publisher_names is a property of the Journal model. This will result in an AttributeError since Article doesn't have a publisher_names property. The correct access should be object.journal.publisher_names to access the publisher names through the article's journal relationship.

Suggested change
{% for publisher_name in object.publisher_names %}
{% for publisher_name in object.journal.publisher_names %}

Copilot uses AI. Check for mistakes.
Comment on lines 194 to +184
def _format_publisher_info(self):
"""Informações do publisher"""
publishers = list(self.publisher_history)
"""Informações do owner"""
try:
# Deixa preparado para tornar obsoleto o owner_history no modelo Journal
owner_data = self.obj.owner_data
except AttributeError:
owner_data = {}
owners = list(self.obj.owner_history.select_related(
'institution__institution', 'institution__institution__location'
).all())
for p in owners:
owner_data["country_acronym"] = p.institution_country_acronym
owner_data["state_acronym"] = p.institution_state_acronym
owner_data["city_name"] = p.institution_city_name
break
add_items("v310", [owner_data.get("country_acronym")], self.result)
add_items("v320", [owner_data.get("state_acronym")], self.result)
add_items("v480", self.obj.owner_names, self.result)
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

There is an inconsistency in how v480 is populated between the journal and issue formatters. In the journal formatter (line 184), v480 is populated with owner_names, but in the issue formatter (issue/formats/articlemeta_format.py:205), v480 is populated with publisher_names. This discrepancy could lead to inconsistent data in the ArticleMeta export format. The method name _format_publisher_info also suggests it should handle publisher data, not owner data. Either both should use publisher_names or both should use owner_names for v480, and the method name should match its purpose.

Copilot uses AI. Check for mistakes.
Comment on lines 748 to 786
@property
def owner_names(self):
items = []
for item in self.owner_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items

@property
def publisher_names(self):
items = []
for item in self.publisher_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items

@property
def sponsors(self):
items = []
for item in self.sponsor_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items

@property
def copyright_holders(self):
items = []
for item in self.copyright_holder_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The property methods access related objects through .all() and check item.organization for each item, which could lead to N+1 query problems when these properties are accessed frequently. Consider using select_related('organization', 'institution__institution__institution_identification') to prefetch related data, or alternatively, implement these as methods that accept an already-prefetched queryset to give callers control over query optimization. This is especially important since these properties are used in API serializers and formatters that may process multiple journals.

Copilot uses AI. Check for mistakes.
</h1>
<span class="publisher">
{% trans 'Publicação de' %}: <strong class="namePlublisher">{% for publisher in journal.publisher_history.all %}{{ publisher.institution.institution.institution_identification.name}}{% endfor %}</strong>
{% trans 'Publicação de' %}: <strong class="namePlublisher">{% for publisher in journal.publisher_names %}{{ publisher }}{% endfor %}</strong>
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

When iterating over multiple publisher names, the template doesn't include any separator between them. If a journal has multiple publishers, they will be concatenated without spaces or commas, making them difficult to read. Consider adding a separator (e.g., using the {% if not forloop.last %}, {% endif %} pattern) or using the join filter.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +749 to +756
def owner_names(self):
items = []
for item in self.owner_history.all():
if item.organization:
items.append(item.organization.name)
else:
items.append(item.institution_name)
return items
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The owner_names property is missing select_related optimization for the 'organization' foreign key relationship. When accessing item.organization.name, this will cause an N+1 query problem. The other similar properties (publisher_names, sponsors, copyright_holders) all use select_related which improves performance. Consider adding select_related('organization', 'institution__institution', 'institution__institution__location') to the queryset on line 751.

Copilot uses AI. Check for mistakes.
</h1>
<span class="publisher">
{% trans 'Publicação de' %}: <strong class="namePlublisher">{% for publisher in journal.publisher_history.all %}{{ publisher.institution.institution.institution_identification.name}}{% endfor %}</strong>
{% trans 'Publicação de' %}: <strong class="namePlublisher">{% for publisher in journal.publisher_names %}{{ publisher }}{% endfor %}</strong>
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

Multiple publishers will be concatenated without any separator (comma, semicolon, etc.), making them hard to read. For example, if there are two publishers "Publisher A" and "Publisher B", they will display as "Publisher APublisher B". Consider adding a separator between items, such as using the join filter: {% for publisher in journal.publisher_names %}{{ publisher }}{% if not forloop.last %}, {% endif %}{% endfor %}

Suggested change
{% trans 'Publicação de' %}: <strong class="namePlublisher">{% for publisher in journal.publisher_names %}{{ publisher }}{% endfor %}</strong>
{% trans 'Publicação de' %}: <strong class="namePlublisher">{% for publisher in journal.publisher_names %}{{ publisher }}{% if not forloop.last %}, {% endif %}{% endfor %}</strong>

Copilot uses AI. Check for mistakes.
Comment on lines +123 to 125
{% for publisher in journal.publisher_names %}
<i>{{ publisher }}</i>
{% endfor %}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

Multiple publishers will be concatenated without any separator when rendered inside the italic tags. If there are multiple publishers, they will appear as a single run-on text without commas or other delimiters. Consider adding a separator between items, or using a separate list item for each publisher.

Suggested change
{% for publisher in journal.publisher_names %}
<i>{{ publisher }}</i>
{% endfor %}
<i>
{% for publisher in journal.publisher_names %}
{% if not forloop.first %}, {% endif %}
{{ publisher }}
{% endfor %}
</i>

Copilot uses AI. Check for mistakes.
Comment on lines 167 to 186
def _format_publisher_info(self):
"""Informações do publisher"""
publishers = list(self.publisher_history)
"""Informações do owner"""
try:
# Deixa preparado para tornar obsoleto o owner_history no modelo Journal
owner_data = self.obj.owner_data
except AttributeError:
owner_data = {}
owners = list(self.obj.owner_history.select_related(
'institution__institution', 'institution__institution__location'
).all())
for p in owners:
owner_data["country_acronym"] = p.institution_country_acronym
owner_data["state_acronym"] = p.institution_state_acronym
owner_data["city_name"] = p.institution_city_name
break
add_items("v310", [owner_data.get("country_acronym")], self.result)
add_items("v320", [owner_data.get("state_acronym")], self.result)
add_items("v480", self.obj.owner_names, self.result)
add_items("v490", [owner_data.get("city_name")], self.result)

Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The function name '_format_publisher_info' is misleading as it actually formats owner information (not publisher information). The comment correctly states "Informações do owner" but the function name should be updated to '_format_owner_info' to match its actual purpose. This inconsistency could confuse future maintainers.

Copilot uses AI. Check for mistakes.
Comment on lines +92 to +94
{% for publisher_name in object.publisher_names %}
<field name="value">{{ publisher_name }}</field>
{% endfor %}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The template is trying to access object.publisher_names, but the Article model doesn't have this property. This will fail at runtime when the search index is built. The template should access this through the journal relationship instead: object.journal.publisher_names. Additionally, there should be a check to ensure object.journal exists before accessing this property.

Suggested change
{% for publisher_name in object.publisher_names %}
<field name="value">{{ publisher_name }}</field>
{% endfor %}
{% if object.journal %}
{% for publisher_name in object.journal.publisher_names %}
<field name="value">{{ publisher_name }}</field>
{% endfor %}
{% endif %}

Copilot uses AI. Check for mistakes.
@robertatakenaka robertatakenaka merged commit 259f646 into scieloorg:main Feb 1, 2026
9 of 11 checks passed
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.

1 participant