Corrige a possível causa da duplicação de Article by robertatakenaka · Pull Request #952 · scieloorg/scms-upload

robertatakenaka · 2026-04-30T20:25:36Z

O que esse PR faz?

Corrige a lógica de identificação e deduplicação de artigos, substituindo pid_v3 por pid_v2 como chave de busca em Article.create_or_update, e substituindo a validação de pid_v2 por sufixo de order por uma consulta direta à tabela MigratedArticle.

Problemas que resolve:

Duplicação de artigos: ao usar pid_v3 como chave de busca, artigos que mudavam de pid_v3 mas mantinham o mesmo pid_v2 eram criados em duplicata. Agora a busca é feita por pid_v2, que é estável para o mesmo artigo.
Validação frágil de pid_v2: o método has_valid_pid_v2 comparava os últimos 5 dígitos do pid_v2 com o campo order do documento migrado, uma heurística que podia falhar silenciosamente. Agora a validação é feita consultando diretamente se o pid_v2 existe na tabela MigratedArticle (método MigratedArticle.valid_pid).
Queries N+1: adiciona select_related('sps_pkg', 'pp_xml') na consulta de artigos em exclude_articles_with_invalid_pid_v2.

Onde a revisão poderia começar?

migration/models.py — novo método MigratedArticle.valid_pid
article/models.py — alterações em create_or_update e exclude_articles_with_invalid_pid_v2

Como este poderia ser testado manualmente?

Deduplicação (create_or_update):
- Ter um artigo já cadastrado com um dado pid_v2
- Submeter um pacote SPS com o mesmo pid_v2 mas pid_v3 diferente
- Verificar que o artigo existente é atualizado (não duplicado)
Validação de pid_v2 (exclude_articles_with_invalid_pid_v2):
- Ter artigos com pid_v2 que existem em MigratedArticle e artigos com pid_v2 que não existem
- Executar Article.exclude_articles_with_invalid_pid_v2() para um issue
- Verificar que apenas artigos cujo pid_v2 é encontrado em MigratedArticle são marcados para exclusão
Validações de entrada:
- Submeter sps_pkg sem xml_with_pre → deve lançar ValueError
- Submeter sps_pkg cujo xml_with_pre.v2 é None → deve lançar ValueError

Algum cenário de contexto que queira dar?

O sistema de migração, em alguns casos, gerou pid_v2 aleatórios em vez de preservar o pid_v2 original do artigo. A validação anterior (comparação de sufixo com order) não detectava todos esses casos. Com a nova abordagem, verificamos diretamente se o pid_v2 consta na tabela de artigos migrados — se não consta, é um pid_v2 inválido gerado erroneamente.

Além disso, usar pid_v3 como chave de busca em create_or_update causava duplicação quando o mesmo artigo recebia um novo pid_v3 em reprocessamento, mas o pid_v2 permanecia o mesmo.

Screenshots

N/A

Quais são tickets relevantes?

#950

Referências

n/a

- Adiciona classmethod valid_pid que verifica se um pid existe na tabela MigratedArticle - Retorna True se encontrar o registro (inclusive para MultipleObjectsReturned) - Retorna False se não encontrar (DoesNotExist) - Será usado por Article.exclude_articles_with_invalid_pid_v2 para validar pid_v2

… e valida pid_v2 via MigratedArticle - Altera create_or_update para buscar artigo por pid_v2 em vez de pid_v3, evitando duplicação quando pid_v3 muda mas pid_v2 é o mesmo - Adiciona validações de xml_with_pre e pid_v2 antes de prosseguir - Adiciona import de MigratedArticle - Remove método estático has_valid_pid_v2 (validação por sufixo de order) - Refatora exclude_articles_with_invalid_pid_v2: - Adiciona filtro pid_v2__isnull=False - Adiciona select_related('sps_pkg', 'pp_xml') para otimizar queries - Substitui validação por sufixo de order por MigratedArticle.valid_pid - Simplifica lógica removendo dependência de migrated_data.document.order - Troca logging.exception por events.append para erros de verificação

Copilot

Pull request overview

Ajusta a lógica de deduplicação/identificação de Article para evitar criação de duplicatas em reprocessamentos de migração, passando a usar pid_v2 como chave estável e mudando a validação de pid_v2 para consultar diretamente MigratedArticle.

Changes:

Adiciona MigratedArticle.valid_pid() para validar se um pid existe na tabela migrada.
Atualiza Article.create_or_update() para buscar por pid_v2 em vez de pid_v3.
Altera Article.exclude_articles_with_invalid_pid_v2() para validar via MigratedArticle e ajusta a consulta de artigos com select_related.

Reviewed changes

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

File	Description
migration/models.py	Adiciona helper para validar existência de PID em `MigratedArticle`.
article/models.py	Troca chave de dedup para `pid_v2` e altera estratégia de validação/exclusão de artigos migrados.

Comments suppressed due to low confidence (1)

article/models.py:495

O docstring desta função ainda descreve a validação por sufixo do pid_v2 vs order (v121), mas a lógica foi trocada para consulta em MigratedArticle. Atualize o docstring para refletir o novo critério de invalidez (pid_v2 ausente em MigratedArticle).

        """
        Find and delete migrated articles whose pid_v2 last 5 digits
        don't match the order (v121) from MigratedArticle.document.order.
        Uses ArticleProc.migrated_data to access the migration data.
        Only applies to migrated articles.
        """

Copilot · 2026-04-30T20:30:50Z

        try:
-            obj = cls.get(sps_pkg.pid_v3)
+            # usa pid_v2 para evitar duplicação por usar o pid_v3 como chave única, e o pid_v2 é o mesmo para artigos duplicados
+            obj = cls.objects.get(pid_v2=pid_v2)
            obj.updated_by = user
        except cls.DoesNotExist:
            obj = cls()
            obj.pid_v3 = sps_pkg.pid_v3
+            obj.pid_v2 = pid_v2


A busca por pid_v2 usa .get() sem tratar MultipleObjectsReturned. Como o problema que este PR resolve envolve justamente múltiplos Articles com o mesmo pid_v2, isso pode lançar exceção e interromper o fluxo. Considere aplicar a mesma estratégia de dedup usada em Article.get (selecionar o mais recente e remover os demais) e, ao atualizar, também sincronizar obj.pid_v3 com sps_pkg.pid_v3 para manter consistência e evitar que add_pp_xml() continue usando um pid_v3 antigo.

Copilot · 2026-04-30T20:30:50Z

    @classmethod
    def exclude_articles_with_invalid_pid_v2(cls, issue=None):
        """


A remoção de has_valid_pid_v2 vai quebrar os testes existentes que ainda chamam Article.has_valid_pid_v2 (ex.: article/test_models.py). Se a heurística antiga foi substituída, ajuste/remova esses testes junto com esta alteração para manter o CI passando.

Copilot · 2026-04-30T20:30:50Z

        filters = {
+            "pid_v2__isnull": False,
            "migrated_data__isnull": False,
            "sps_pkg__isnull": False,
        }


filters é aplicado em ArticleProc.objects.filter(**filters), mas ArticleProc não tem campo pid_v2. Isso deve resultar em FieldError em runtime. Se a intenção é filtrar pelo pid_v2 do Article ou do pacote, ajuste para o caminho correto (ex.: via relação existente) ou remova esse filtro daqui.

Copilot · 2026-04-30T20:30:51Z

-                        f"expected suffix={expected_suffix}, "
-                        f"actual suffix={article.pid_v2[-5:]})"
-                    )
+                if MigratedArticle.valid_pid(article.pid_v2):


A condição parece invertida: se MigratedArticle.valid_pid(article.pid_v2) retorna True, isso indica que o pid_v2 existe em MigratedArticle e, portanto, deveria ser considerado válido. Do jeito que está, o código marca para exclusão justamente os artigos com pid_v2 válido (e o comentário também sugere o oposto). Inverta a condição para excluir apenas quando o pid_v2 não existir em MigratedArticle.

Suggested change

if MigratedArticle.valid_pid(article.pid_v2):

if not MigratedArticle.valid_pid(article.pid_v2):

Copilot · 2026-04-30T20:30:51Z

        for article_proc in article_procs:
            try:
                article = articles_by_sps_pkg.get(article_proc.sps_pkg_id)
                if not article or not article.pid_v2:
                    continue
-
-                order = article_proc.migrated_data.document.order
-                if not order:
-                    continue
-
-                if not cls.has_valid_pid_v2(article.pid_v2, order):
-                    try:
-                        expected_suffix = str(int(str(order).strip())).zfill(5)
-                    except (TypeError, ValueError):
-                        expected_suffix = str(order)
-                    events.append(
-                        f"Invalid pid_v2: {article.pid_v2} "
-                        f"(order={order}, "
-                        f"expected suffix={expected_suffix}, "
-                        f"actual suffix={article.pid_v2[-5:]})"
-                    )
+                if MigratedArticle.valid_pid(article.pid_v2):
+                    # houve um erro o sistema de migração que criou pid_v2 aleatoriamente no lugar de usar o pid_v2 original
                    article_ids.append(article.id)


Do jeito que está, MigratedArticle.valid_pid() é chamado dentro do loop e faz query por item, reintroduzindo um padrão N+1 (agora em MigratedArticle). Para reduzir carga no banco, prefira coletar todos os pid_v2 dos artigos e fazer uma única query MigratedArticle.objects.filter(pid__in=...).values_list('pid', flat=True) para validar em memória.

Copilot · 2026-04-30T20:30:51Z

+                events.append(
                    f"Error checking pid_v2 for ArticleProc {article_proc}: {e}"


A troca de logging.exception(...) por apenas adicionar uma string em events remove o traceback e dificulta diagnosticar falhas em produção. Se a intenção é manter o método resiliente, considere registrar a exceção (ou incluir traceback.format_exc() no evento) para não perder contexto de debugging.

Suggested change

events.append(

f"Error checking pid_v2 for ArticleProc {article_proc}: {e}"

logging.exception(

"Error checking pid_v2 for ArticleProc %s", article_proc

)

events.append(

f"Error checking pid_v2 for ArticleProc {article_proc}: {e}\n"

f"{traceback.format_exc()}"

Copilot · 2026-04-30T20:30:52Z

+        try:
+            MigratedArticle.objects.get(pid=pid)
+            return True
+        except MigratedArticle.DoesNotExist:
+            return False
+        except MigratedArticle.MultipleObjectsReturned:
+            # ok, pois significa que é válido
+            return True


valid_pid faz uma query via .get() e usa exceptions como controle de fluxo. Para apenas checar existência, isso é mais caro e adiciona overhead em cenários em lote; prefira cls.objects.filter(pid=pid).exists() (e trate pid vazio/None retornando False).

Suggested change

try:

MigratedArticle.objects.get(pid=pid)

return True

except MigratedArticle.DoesNotExist:

return False

except MigratedArticle.MultipleObjectsReturned:

# ok, pois significa que é válido

return True

if not pid:

return False

return cls.objects.filter(pid=pid).exists()

robertatakenaka · 2026-04-30T20:36:16Z

@copilot apply changes based on the comments in this thread

…exists - substitui try/except com get() por verificação direta com filter().exists() - adiciona guarda para pid falsy ou com tamanho diferente de 23 caracteres - elimina tratamento desnecessário de MultipleObjectsReturned

- trata MultipleObjectsReturned em get_or_create: mantém o mais recente, remove duplicatas - adiciona override de delete() que remove SPSPkg e PidProviderXML associados (FKs SET_NULL não seriam limpos pelo CASCADE) - corrige filtro pid_v2__isnull para pid__isnull em exclude_articles_with_invalid_pid_v2 - inverte condição de MigratedArticle.valid_pid: artigos com pid inválido devem ser excluídos, não os válidos - altera parâmetro de journal para issue no filtro de exclusão

- substitui testes de Article.has_valid_pid_v2 por MigratedArticle.valid_pid - testa validação por tamanho (23 chars) e existência no banco - atualiza mocks de exclude_articles_with_invalid_pid_v2 para refletir: - select_related na chain de consulta - filtro por issue em vez de journal - inversão da condição valid_pid - remove parâmetro order dos helpers de teste (não mais necessário)

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

article/models.py:529

O docstring de exclude_articles_with_invalid_pid_v2 ainda descreve validação por sufixo/order do v121, mas a implementação agora valida existência em MigratedArticle. Isso pode confundir quem der manutenção e quem consome os eventos. Sugestão: atualizar o docstring para refletir o novo critério (PID v2 de 23 chars presente em MigratedArticle).

        """
        Find and delete migrated articles whose pid_v2 last 5 digits
        don't match the order (v121) from MigratedArticle.document.order.
        Uses ArticleProc.migrated_data to access the migration data.
        Only applies to migrated articles.
        """

Copilot · 2026-04-30T22:23:39Z

+    def delete(self, using=None, keep_parents=False):
+        """
+        Remove o artigo e seus objetos auxiliares exclusivos.
+
+        Antes de excluir o registro de Article, preserva referências para
+        SPSPkg e PidProviderXML (cujos FKs são SET_NULL e não seriam
+        removidos automaticamente). Após a exclusão do Article — que
+        aciona CASCADE em ArticleDOIWithLang, ArticleTitle, RelatedItem e
+        RequestArticleChange — os objetos auxiliares são deletados.
+
+        Args:
+            using: Alias do banco de dados a utilizar (repassado ao super).
+            keep_parents: Se True, mantém registros pai em herança multi-tabela.
+        """
+        # Preserva referências antes de a exclusão zerá-las (SET_NULL)
+        sps_pkg = self.sps_pkg
+        pp_xml = self.pp_xml
+
+        with transaction.atomic():
+            # Exclui o Article e todos os relacionamentos CASCADE
+            super().delete(using=using, keep_parents=keep_parents)
+
+            # Remove SPSPkg e PidProviderXML que pertenciam exclusivamente
+            # a este artigo e não seriam limpos pelo CASCADE
+            if sps_pkg:
+                sps_pkg.delete()
+            if pp_xml:
+                pp_xml.delete()
+


O override de Article.delete() está deletando SPSPkg e PidProviderXML sempre que um Article é removido. Como esses FKs são SET_NULL, isso muda a semântica global de exclusão e pode apagar objetos ainda referenciados por outros registros (ou úteis para auditoria), causando perda de dados. Sugestão: não sobrescrever delete(); crie um método/serviço de cleanup usado apenas pelos fluxos de manutenção e, se for deletar auxiliares, só o faça após verificar que não há outras referências (ex.: article_set.exclude(pk=self.pk).exists()).

Suggested change

def delete(self, using=None, keep_parents=False):

"""

Remove o artigo e seus objetos auxiliares exclusivos.

Antes de excluir o registro de Article, preserva referências para

SPSPkg e PidProviderXML (cujos FKs são SET_NULL e não seriam

removidos automaticamente). Após a exclusão do Article — que

aciona CASCADE em ArticleDOIWithLang, ArticleTitle, RelatedItem e

RequestArticleChange — os objetos auxiliares são deletados.

Args:

using: Alias do banco de dados a utilizar (repassado ao super).

keep_parents: Se True, mantém registros pai em herança multi-tabela.

"""

# Preserva referências antes de a exclusão zerá-las (SET_NULL)

sps_pkg = self.sps_pkg

pp_xml = self.pp_xml

with transaction.atomic():

# Exclui o Article e todos os relacionamentos CASCADE

super().delete(using=using, keep_parents=keep_parents)

# Remove SPSPkg e PidProviderXML que pertenciam exclusivamente

# a este artigo e não seriam limpos pelo CASCADE

if sps_pkg:

sps_pkg.delete()

if pp_xml:

pp_xml.delete()

def cleanup_exclusive_auxiliary_objects(self):

"""

Remove objetos auxiliares somente em fluxos explícitos de manutenção.

Este método não faz parte da exclusão padrão do Article. Ele só deve

ser chamado por rotinas administrativas que realmente desejem limpar

registros auxiliares após validar que não há outras referências.

"""

sps_pkg = self.sps_pkg

pp_xml = self.pp_xml

with transaction.atomic():

if sps_pkg and not Article.objects.filter(sps_pkg=sps_pkg).exclude(

pk=self.pk

).exists():

sps_pkg.delete()

if pp_xml and not Article.objects.filter(pp_xml=pp_xml).exclude(

pk=self.pk

).exists():

pp_xml.delete()

def delete(self, using=None, keep_parents=False):

"""

Remove o artigo preservando a semântica dos relacionamentos com

`on_delete=SET_NULL`.

A limpeza de objetos auxiliares deve ser executada apenas por fluxos

explícitos de manutenção, por meio de `cleanup_exclusive_auxiliary_objects`.

Args:

using: Alias do banco de dados a utilizar (repassado ao super).

keep_parents: Se True, mantém registros pai em herança multi-tabela.

"""

return super().delete(using=using, keep_parents=keep_parents)

Copilot · 2026-04-30T22:23:40Z

+        if not sps_pkg:
+            raise ValueError("create_article requires sps_pkg with pid_v2")

+        xml_with_pre = sps_pkg.xml_with_pre
+        if not xml_with_pre:
+            raise ValueError(f"SPSPkg {sps_pkg} is missing xml_with_pre")
+
+        pid_v2 = xml_with_pre.v2
+        if not pid_v2:
+            raise ValueError(f"SPSPkg {sps_pkg} xml_with_pre is missing pid_v2")
        try:
-            obj = cls.get(sps_pkg.pid_v3)
+            # usa pid_v2 para evitar duplicação por usar o pid_v3 como chave única, e o pid_v2 é o mesmo para artigos duplicados
+            obj = cls.objects.get(pid_v2=pid_v2)
            obj.updated_by = user
+        except cls.MultipleObjectsReturned:
+            items = cls.objects.filter(pid_v2=pid_v2).order_by("-updated")
+            obj = items.first()
+            for item in items[1:]:
+                item.delete()
        except cls.DoesNotExist:
            obj = cls()
            obj.pid_v3 = sps_pkg.pid_v3
+            obj.pid_v2 = pid_v2


create_or_update deixou de validar sps_pkg.pid_v3. Se pid_v3 vier None (cenário descrito quando não há conexão com o Core), o fluxo chama add_pp_xml() depois e isso pode executar PidProviderXML.get_by_pid_v3(None) com parâmetros vazios, potencialmente associando um PidProviderXML incorreto. Sugestão: manter a validação/erro para pid_v3 ausente ou, no mínimo, pular qualquer lógica que dependa de pid_v3 até que ele exista.

Copilot · 2026-04-30T22:23:41Z

+            for item in items[1:]:
+                item.delete()
        except cls.DoesNotExist:
            obj = cls()
            obj.pid_v3 = sps_pkg.pid_v3
+            obj.pid_v2 = pid_v2
            obj.creator = user

        obj.sps_pkg = sps_pkg


Ao buscar o Article por pid_v2, o código não atualiza obj.pid_v3 quando o mesmo artigo é reprocessado com um novo pid_v3 (situação que motivou este PR). Isso pode deixar o registro com pid_v3 antigo e impactar add_pp_xml()/URLs/integrações que dependem do pid_v3. Sugestão: atualizar pid_v3 a partir do sps_pkg.pid_v3 no caminho de update (e garantir consistência em casos de MultipleObjectsReturned).

Suggested change

for item in items[1:]:

item.delete()

except cls.DoesNotExist:

obj = cls()

obj.pid_v3 = sps_pkg.pid_v3

obj.pid_v2 = pid_v2

obj.creator = user

obj.sps_pkg = sps_pkg

obj.updated_by = user

for item in items[1:]:

item.delete()

except cls.DoesNotExist:

obj = cls()

obj.pid_v2 = pid_v2

obj.creator = user

obj.sps_pkg = sps_pkg

obj.pid_v3 = sps_pkg.pid_v3

Copilot · 2026-04-30T22:23:41Z

                        sps_pkg_ids.add(article.sps_pkg_id)
                    if article.pp_xml_id:
                        pp_xml_ids.add(article.pp_xml_id)
            except Exception as e:


No except Exception as e o código passou a apenas adicionar uma string em events, removendo logging.exception(...). Isso reduz observabilidade (perde stacktrace no log) e pode dificultar diagnosticar falhas reais na checagem. Sugestão: manter logging.exception(e) (ou registrar o traceback) e também adicionar o evento, se necessário.

Suggested change

except Exception as e:

except Exception as e:

logging.exception(

"Error checking pid_v2 for ArticleProc %s", article_proc

)

Copilot · 2026-04-30T22:23:41Z

+            # usa pid_v2 para evitar duplicação por usar o pid_v3 como chave única, e o pid_v2 é o mesmo para artigos duplicados
+            obj = cls.objects.get(pid_v2=pid_v2)
            obj.updated_by = user


create_or_update agora faz lookup frequente por pid_v2 (objects.get(pid_v2=...)), mas pid_v2 não parece ter índice/unique constraint no modelo. Em ambientes com muitos artigos isso pode degradar performance. Sugestão: adicionar db_index=True ou um models.Index(fields=['pid_v2']) (e considerar uma constraint apropriada se o domínio permitir).

Copilot · 2026-04-30T22:23:41Z

    def create_or_update(cls, user, sps_pkg, issue=None, journal=None, position=None):
-        if not sps_pkg or sps_pkg.pid_v3 is None:
-            raise ValueError("create_article requires sps_pkg with pid_v3")
+        if not sps_pkg:
+            raise ValueError("create_article requires sps_pkg with pid_v2")

+        xml_with_pre = sps_pkg.xml_with_pre
+        if not xml_with_pre:
+            raise ValueError(f"SPSPkg {sps_pkg} is missing xml_with_pre")
+
+        pid_v2 = xml_with_pre.v2
+        if not pid_v2:
+            raise ValueError(f"SPSPkg {sps_pkg} xml_with_pre is missing pid_v2")
        try:
-            obj = cls.get(sps_pkg.pid_v3)
+            # usa pid_v2 para evitar duplicação por usar o pid_v3 como chave única, e o pid_v2 é o mesmo para artigos duplicados
+            obj = cls.objects.get(pid_v2=pid_v2)
            obj.updated_by = user
+        except cls.MultipleObjectsReturned:
+            items = cls.objects.filter(pid_v2=pid_v2).order_by("-updated")
+            obj = items.first()
+            for item in items[1:]:
+                item.delete()
        except cls.DoesNotExist:


As mudanças em Article.create_or_update (chave de busca por pid_v2, novos ValueErrors e deduplicação) não estão cobertas por testes unitários neste módulo, apesar de já existirem testes para outros comportamentos de Article. Sugestão: adicionar testes para: (1) update do mesmo pid_v2 com pid_v3 diferente não criar duplicata, (2) xml_with_pre/pid_v2 ausentes gerarem erro, e (3) dedupe quando existem múltiplos Articles com mesmo pid_v2.

Copilot · 2026-04-30T22:23:41Z

        for article_proc in article_procs:
            try:
                article = articles_by_sps_pkg.get(article_proc.sps_pkg_id)
                if not article or not article.pid_v2:
                    continue
-
-                order = article_proc.migrated_data.document.order
-                if not order:
-                    continue
-
-                if not cls.has_valid_pid_v2(article.pid_v2, order):
-                    try:
-                        expected_suffix = str(int(str(order).strip())).zfill(5)
-                    except (TypeError, ValueError):
-                        expected_suffix = str(order)
-                    events.append(
-                        f"Invalid pid_v2: {article.pid_v2} "
-                        f"(order={order}, "
-                        f"expected suffix={expected_suffix}, "
-                        f"actual suffix={article.pid_v2[-5:]})"
-                    )
+                if not MigratedArticle.valid_pid(article.pid_v2):
+                    # houve um erro o sistema de migração que criou pid_v2 aleatoriamente no lugar de usar o pid_v2 original


exclude_articles_with_invalid_pid_v2 chama MigratedArticle.valid_pid() dentro do loop e cada chamada faz um .exists() no banco. Isso reintroduz um padrão N+1 (agora na tabela MigratedArticle) proporcional ao número de artigos do fascículo. Sugestão: coletar todos os pid_v2 candidatos e fazer uma única query MigratedArticle.objects.filter(pid__in=...).values_list('pid', flat=True) para montar um set e validar por membership.

Copilot · 2026-04-30T22:23:41Z

+        if not pid or len(pid) != 23:
+            return False
+        return MigratedArticle.objects.filter(pid=pid).exists()


MigratedArticle.valid_pid assume que pid é uma string e chama len(pid). Se pid vier como outro tipo (ex.: int/bytes) isso levanta TypeError. Sugestão: validar tipo (isinstance(pid, str)) antes do len, e usar cls.objects em vez de referenciar MigratedArticle.objects diretamente para manter o método mais genérico/testável.

Suggested change

if not pid or len(pid) != 23:

return False

return MigratedArticle.objects.filter(pid=pid).exists()

if not isinstance(pid, str) or not pid or len(pid) != 23:

return False

return cls.objects.filter(pid=pid).exists()

…zado - Atribui updated_by ao usuário corrente ao resolver duplicatas em get_or_create, garantindo rastreabilidade da última edição - Remove override de delete() que excluía manualmente SPSPkg e PidProviderXML após a exclusão do Article; a limpeza desses objetos será tratada por outro mecanismo

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

article/models.py:501

The docstring for exclude_articles_with_invalid_pid_v2 still describes the old heuristic (suffix vs MigratedArticle.document.order). The implementation now validates by querying MigratedArticle.valid_pid, so the docstring is misleading and should be updated to match the new behavior.

    def exclude_articles_with_invalid_pid_v2(cls, issue=None):
        """
        Find and delete migrated articles whose pid_v2 last 5 digits
        don't match the order (v121) from MigratedArticle.document.order.
        Uses ArticleProc.migrated_data to access the migration data.
        Only applies to migrated articles.
        """

Copilot · 2026-04-30T22:39:46Z

+    def valid_pid(cls, pid):
+        if not pid or len(pid) != 23:
+            return False
+        return MigratedArticle.objects.filter(pid=pid).exists()


valid_pid is a classmethod but it hardcodes MigratedArticle.objects instead of using cls.objects, which makes the method less reusable (e.g., for subclasses/proxies) and harder to test/override. Prefer querying via cls.objects.filter(pid=pid).exists().

Suggested change

return MigratedArticle.objects.filter(pid=pid).exists()

return cls.objects.filter(pid=pid).exists()

Copilot · 2026-04-30T22:39:47Z

        try:
-            obj = cls.get(sps_pkg.pid_v3)
+            # usa pid_v2 para evitar duplicação por usar o pid_v3 como chave única, e o pid_v2 é o mesmo para artigos duplicados
+            obj = cls.objects.get(pid_v2=pid_v2)
            obj.updated_by = user
+        except cls.MultipleObjectsReturned:
+            items = cls.objects.filter(pid_v2=pid_v2).order_by("-updated")
+            obj = items.first()
+            obj.updated_by = user
+            for item in items[1:]:
+                item.delete()
        except cls.DoesNotExist:
            obj = cls()
            obj.pid_v3 = sps_pkg.pid_v3
+            obj.pid_v2 = pid_v2
            obj.creator = user

        obj.sps_pkg = sps_pkg
-
-        xml_with_pre = sps_pkg.xml_with_pre
-        obj.pid_v2 = xml_with_pre.v2
+        obj.pid_v2 = pid_v2
        obj.article_type = xml_with_pre.xmltree.find(".").get("article-type")


create_or_update switches lookup to pid_v2, but when an existing Article is found it never updates obj.pid_v3 from the incoming sps_pkg.pid_v3. This can leave the Article pointing to an outdated pid_v3 (and add_pp_xml() depends on self.pid_v3), causing inconsistent identifiers and potentially wrong pp_xml linkage/URLs. Update pid_v3 on every run (and handle any uniqueness conflict/merge if another row already has that pid_v3).

Copilot · 2026-04-30T22:39:47Z

        articles_by_sps_pkg = {}
        if sps_pkg_id_list:
-            for article in Article.objects.filter(
+            for article in Article.objects.select_related("sps_pkg", "pp_xml").filter(


Article.objects.select_related("sps_pkg", "pp_xml") is combined with .only("id", "pid_v2", "sps_pkg_id", "pp_xml_id"), and the code only uses the FK ids (not the related objects). This join work is likely unnecessary and may increase query cost; consider dropping select_related here (or adjust .only() to actually use the related objects if that's the intent).

Suggested change

for article in Article.objects.select_related("sps_pkg", "pp_xml").filter(

for article in Article.objects.filter(

Copilot · 2026-04-30T22:39:47Z

                        sps_pkg_ids.add(article.sps_pkg_id)
                    if article.pp_xml_id:
                        pp_xml_ids.add(article.pp_xml_id)
            except Exception as e:


The exception handler was changed from logging.exception(...) to only appending a short string into events. This drops the stack trace from logs, making production troubleshooting harder (and the event string alone may not be persisted). Prefer keeping logging.exception(...) (and optionally also add an event), or include traceback.format_exc() in the event if you intentionally avoid logging.

Suggested change

except Exception as e:

except Exception as e:

logging.exception(

"Error checking pid_v2 for ArticleProc %s", article_proc

)

robertatakenaka added 2 commits April 30, 2026 17:16

Copilot AI review requested due to automatic review settings April 30, 2026 20:25

Copilot started reviewing on behalf of robertatakenaka April 30, 2026 20:26 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

robertatakenaka added 3 commits April 30, 2026 19:00

robertatakenaka requested a review from Copilot April 30, 2026 22:18

Copilot started reviewing on behalf of robertatakenaka April 30, 2026 22:18 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

robertatakenaka requested a review from Copilot April 30, 2026 22:35

Copilot started reviewing on behalf of robertatakenaka April 30, 2026 22:36 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

robertatakenaka merged commit c282bd2 into scieloorg:main May 2, 2026
7 checks passed

	if MigratedArticle.valid_pid(article.pid_v2):
	if not MigratedArticle.valid_pid(article.pid_v2):

		events.append(
		f"Error checking pid_v2 for ArticleProc {article_proc}: {e}"

-                events.append(
-                    f"Error checking pid_v2 for ArticleProc {article_proc}: {e}"
+                logging.exception(
+                    "Error checking pid_v2 for ArticleProc %s", article_proc
+                )
+                events.append(
+                    f"Error checking pid_v2 for ArticleProc {article_proc}: {e}\n"
+                    f"{traceback.format_exc()}"

	return MigratedArticle.objects.filter(pid=pid).exists()
	return cls.objects.filter(pid=pid).exists()

	for article in Article.objects.select_related("sps_pkg", "pp_xml").filter(
	for article in Article.objects.filter(

Conversation

robertatakenaka commented Apr 30, 2026

O que esse PR faz?

Onde a revisão poderia começar?

Como este poderia ser testado manualmente?

Algum cenário de contexto que queira dar?

Screenshots

Quais são tickets relevantes?

Referências

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

robertatakenaka commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026