Skip to content

Conversation

@Rossi-Luciano
Copy link
Collaborator

O que esse PR faz?

Este PR expande o sistema de validação de contribuidores (article_contribs) com três objetivos principais:

  1. Corrige bugs críticos: Bug de lógica invertida em validação de afiliações e lista incorreta de valores para specific-use

  2. Adiciona 5 novas validações obrigatórias:

    • validate_contrib_type() - Valida atributo obrigatório @contrib-type
    • Detecção de URLs em ORCID (ao invés de identificador alfanumérico)
    • CollabGroupValidation - Valida estrutura completa de grupos (nomes, afiliações e ORCIDs)
    • DocumentCreditConsistencyValidation - Valida regra "tudo ou nada" da taxonomia CRediT
    • SubArticleCollabIDValidation - Valida IDs únicos entre article e sub-articles
  3. Implementa internacionalização completa: Adiciona suporte i18n (advice_text e advice_params) em todas as 17 validações, preparando 35 mensagens para tradução em pt_BR, en e es

Resultado: Aumenta a cobertura de regras SciELO de 37% (7/19) para 79% (15/19).


Onde a revisão poderia começar?

Ordem recomendada de revisão:

  1. RESUMO_IMPLEMENTACOES.md - Visão geral completa das mudanças (recomendado iniciar aqui)

  2. packtools/sps/validation/article_contribs_rules.json

    • Linhas 17-19: Lista specific-use corrigida
    • Linhas 45-58: Encoding UTF-8 corrigido nos termos CRediT
    • Novos parâmetros de erro adicionados
  3. packtools/sps/validation/article_contribs.py

    • Linhas 48-154: Nova validação validate_contrib_type()
    • Linhas 221-245: Detecção de URL em ORCID
    • Linhas 376-385: BUG CORRIGIDO - validate_affiliations() (linha 383)
    • Linhas 587-693: Nova classe CollabGroupValidation
    • Linhas 695-838: Nova classe DocumentCreditConsistencyValidation
    • Linhas 840-1002: Nova classe SubArticleCollabIDValidation
    • i18n: Todas as validações agora têm advice_text e advice_params
  4. tests/sps/validation/test_article_contribs.py

    • Linhas 68-94: Novos testes para validate_contrib_type
    • Linhas 132-143: Novo teste para detecção de URL em ORCID
    • Linhas 298-354: Novos testes para CollabGroupValidation (3 testes)
    • Linhas 357-469: Novos testes para DocumentCreditConsistencyValidation (3 testes)
    • Linhas 472-683: Novos testes para SubArticleCollabIDValidation (3 testes)

Como este poderia ser testado manualmente?

1. Executar a suite de testes completa:

cd packtools
python -m unittest tests.sps.validation.test_article_contribs -v

Resultado esperado: Ran 27 tests ... OK (100% passando)

2. Testar validações específicas com XMLs reais:

Teste A - Validar @contrib-type obrigatório:

from lxml import etree
from packtools.sps.validation.article_contribs import ContribValidation

# XML sem @contrib-type
contrib = {"contrib_full_name": "Silva, João"}
validator = ContribValidation(contrib, {})
results = list(validator.validate_contrib_type())
# Deve retornar erro: "@contrib-type attribute"

Teste B - Validar detecção de URL em ORCID:

# Contrib com URL ao invés de identificador
contrib = {
    "contrib_full_name": "Silva, João",
    "contrib_ids": {"orcid": "https://orcid.org/0000-0001-2345-6789"}
}
validator = ContribValidation(contrib, {})
results = list(validator.validate_orcid_format())
# Deve retornar erro: "ORCID format - URL detected"

Teste C - Validar consistência CRediT:

<!-- XML com uso inconsistente de CRediT (deve falhar) -->
<article>
  <contrib-group>
    <contrib>
      <role content-type="https://credit.niso.org/...">Writing</role>
    </contrib>
    <contrib>
      <role>Methodology</role> <!-- Sem CRediT! -->
    </contrib>
  </contrib-group>
</article>

3. Testar internacionalização:

# Verificar estrutura de resposta com i18n
result = next(validator.validate_orcid_format())
assert "advice_text" in result  # Template i18n
assert "advice_params" in result  # Parâmetros i18n

Algum cenário de contexto que queira dar?

N.A.

Screenshots

N.A.


Quais são tickets relevantes?

N.A.


Referências

N.A.

- Corrige encoding UTF-8 dos termos CRediT (– vs â€")
- Atualiza lista specific-use para apenas reviewer/editor
- Adiciona níveis de erro para novas validações
- Adiciona contrib_type_list com valores válidos
- Adiciona 5 novas validações (contrib_type, URL, grupos, CRediT,
sub-articles)
- Corrige bug crítico em validate_affiliations (lógica invertida)
- Adiciona internacionalização em todas as 17 validações (35 mensagens)
- Aumenta cobertura de regras SciELO de 37% para 79%
- Adiciona 15 novos testes (contrib_type, URL, grupos, CRediT,
sub-articles)
- Corrige encoding em testes existentes (termos CRediT UTF-8)
- Corrige expectativas de specific-use (reviewer/editor)
- Total: 27 testes, 100% passando
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

Expande as validações de contribuidores (article_contribs) para maior conformidade SciELO, incluindo novas regras (contrib-type, ORCID URL, completude de collab-list, consistência CRediT e unicidade entre sub-articles) e suporte a i18n via advice_text/advice_params.

Changes:

  • Adiciona novas validações em article_contribs.py (incluindo classes novas e integração no fluxo de validação).
  • Atualiza regras em article_contribs_rules.json (novos níveis de erro, listas e termos/URLs CRediT).
  • Amplia a suite de testes com casos novos para contrib-type, ORCID URL e novas validações.

Reviewed changes

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

File Description
tests/sps/validation/test_article_contribs.py Adiciona testes para novas validações e ajusta fixtures/expectativas.
packtools/sps/validation_rules/article_contribs_rules.json Atualiza parâmetros e listas (ex.: contrib-type, specific-use, CRediT URLs/termos).
packtools/sps/validation/article_contribs.py Implementa validações novas e adiciona i18n em build_response (adv_text/adv_params).

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

validation_type="consistency",
is_valid=False,
expected="consistent taxonomy (all CRediT or all non-CRediT)",
obtained=f"mixed taxonomy in contributors: {', '.join(mixed_contribs)}",
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

mixed_contribs.append(contrib.get("contrib_full_name")) can append None for non-person contributors (e.g., <collab> without <name>). When that happens, ', '.join(mixed_contribs) will raise a TypeError. Use a non-null identifier (e.g., fall back to collab/<unknown>) or filter out falsy values before joining.

Suggested change
obtained=f"mixed taxonomy in contributors: {', '.join(mixed_contribs)}",
obtained=(
"mixed taxonomy in contributors: "
f"{', '.join(str(c) for c in mixed_contribs if c)}"
),

Copilot uses AI. Check for mistakes.
Comment on lines +1214 to +1224
valid = specific_use in expected if specific_use else True

if specific_use and not valid:
advice = f'{self.info} replace {specific_use} in <role specific-use="{specific_use}"> with {expected}'
advice_text = _('{info} replace {specific_use} in <role specific-use="{specific_use}"> with {expected}')
advice_params = {"info": self.info, "specific_use": specific_use, "expected": ", ".join(expected)}
else:
advice = f'{self.info} add contributor role type <contrib><role specific-use=""></role></contrib> with {expected}'
advice_text = _('{info} add contributor role type <contrib><role specific-use=""></role></contrib> with {expected}')
advice_params = {"info": self.info, "expected": ", ".join(expected)}

Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

validate_role_specific_use() currently treats a missing specific-use as valid (... else True), which effectively disables validation when the attribute is absent. Given the function’s purpose and the advice text prompting users to add the attribute, this likely should be an error when specific-use is missing (at least when contrib_role_specific_use_list is non-empty). Consider making specific-use presence part of the validation or splitting into an exist check + value in list check.

Suggested change
valid = specific_use in expected if specific_use else True
if specific_use and not valid:
advice = f'{self.info} replace {specific_use} in <role specific-use="{specific_use}"> with {expected}'
advice_text = _('{info} replace {specific_use} in <role specific-use="{specific_use}"> with {expected}')
advice_params = {"info": self.info, "specific_use": specific_use, "expected": ", ".join(expected)}
else:
advice = f'{self.info} add contributor role type <contrib><role specific-use=""></role></contrib> with {expected}'
advice_text = _('{info} add contributor role type <contrib><role specific-use=""></role></contrib> with {expected}')
advice_params = {"info": self.info, "expected": ", ".join(expected)}
# When there is a configured list of allowed values, require the
# presence of @specific-use and that its value be in the list.
if expected:
if specific_use is None:
valid = False
else:
valid = specific_use in expected
else:
# No configured allowed values: do not enforce validation.
valid = True
if expected:
if specific_use is None:
advice = f'{self.info} add contributor role type <contrib><role specific-use=""></role></contrib> with {expected}'
advice_text = _('{info} add contributor role type <contrib><role specific-use=""></role></contrib> with {expected}')
advice_params = {"info": self.info, "expected": ", ".join(expected)}
elif not valid:
advice = f'{self.info} replace {specific_use} in <role specific-use="{specific_use}"> with {expected}'
advice_text = _('{info} replace {specific_use} in <role specific-use="{specific_use}"> with {expected}')
advice_params = {"info": self.info, "specific_use": specific_use, "expected": ", ".join(expected)}
else:
# Valid and in list: no corrective advice needed.
advice = ""
advice_text = ""
advice_params = {}
else:
# No constraints to apply: no advice.
advice = ""
advice_text = ""
advice_params = {}

Copilot uses AI. Check for mistakes.
Comment on lines +380 to +387
def test_validate_collab_members_complete(self):
"""Test validate_collab_members_completeness with complete member info"""
validator = CollabGroupValidation(self.xmltree.find("."), {})
results = list(validator.validate_collab_members_completeness())
errors = [r for r in results if r['response'] != 'OK']
# Sem afiliações completas no XML, esperamos erros
# Este teste precisa de XMLAffiliations mock para passar
self.assertGreaterEqual(len(errors), 0)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

test_validate_collab_members_complete doesn’t assert any expected behavior (assertGreaterEqual(len(errors), 0) is always true), so it won’t catch regressions. Either assert the exact expected errors for the provided XML (e.g., currently missing affiliation resolution) or adjust the fixture/mocks so the test can assert a truly “complete” case with zero errors.

Copilot uses AI. Check for mistakes.

# Valida afiliação
affs = contrib_data.get("affs") or []
if not affs:
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

validate_collab_members_completeness() checks contrib_data.get("affs"), but ContribGroup.data only provides contrib_xref (and affs is only populated by XMLContribs._add_affs). As a result, collab-list members will be reported as missing affiliation even when <xref ref-type="aff" .../> is present. Consider validating presence of an aff xref (contrib_xref with ref_type == "aff") and/or resolving rid→ via XMLAffiliations/XMLContribs so the rule reflects the actual XML.

Suggested change
if not affs:
# Para contribuições em collab-list, a afiliação pode ser indicada
# apenas via <xref ref-type="aff"> em contrib_xref.
contrib_xref = contrib_data.get("contrib_xref") or []
has_aff_xref = any(
(
xref.get("ref_type") == "aff"
or xref.get("ref-type") == "aff"
)
for xref in contrib_xref
)
has_affiliation = bool(affs) or has_aff_xref
if not has_affiliation:

Copilot uses AI. Check for mistakes.
is_valid_value = contrib_type in valid_values

if not is_valid_value:
advice = f'{self.info} @contrib-type="{contrib_type}" is invalid. Use: {" or ".join(valid_values)}'
Copy link
Member

Choose a reason for hiding this comment

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

@Rossi-Luciano entre os { e }, prefira usar variável para não deixar o código difícil de ler

validation_type="consistency",
is_valid=False,
expected="consistent taxonomy (all CRediT or all non-CRediT)",
obtained=f"mixed taxonomy in contributors: {', '.join(mixed_contribs)}",
Copy link
Member

Choose a reason for hiding this comment

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

@Rossi-Luciano usar variáveis entre { e }.

)


class CollabGroupValidation:
Copy link
Member

Choose a reason for hiding this comment

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

@Rossi-Luciano veja se não está duplicado

Copy link
Member

@robertatakenaka robertatakenaka left a comment

Choose a reason for hiding this comment

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

@Rossi-Luciano verificar

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.

2 participants