Skip to content

Conversation

@Rossi-Luciano
Copy link
Collaborator

O que esse PR faz?

Implementa validações completas para <app-group> e <app> conforme regras SciELO Brasil, aumentando conformidade de 10% para 95%.

Adiciona 4 novas validações obrigatórias:

  • @id obrigatório em <app> (CRITICAL)
  • <label> obrigatório em <app> (CRITICAL)
  • <app-group> como wrapper obrigatório, mesmo para um único <app> (CRITICAL)
  • Acessibilidade em <media>: alt-text/long-desc e referência a transcrição (ERROR/WARNING)

Melhorias adicionais:

  • Internacionalização completa (advice_text/advice_params)
  • Detecta <app> órfão (fora de <app-group>)
  • Detecta múltiplos <app-group> (máximo 1 permitido)
  • 35 novos testes (+1750% cobertura)

Onde a revisão poderia começar?

  1. packtools/sps/validation/app_group.py linha 63 - validate_app_id()
    Nova validação: verifica @id obrigatório em cada <app>

  2. packtools/sps/validation/app_group.py linha 88 - validate_app_label()
    Nova validação: verifica <label> obrigatório em cada <app>

  3. packtools/sps/validation/app_group.py linha 113 - validate_app_group_wrapper()
    Nova validação: detecta <app> órfão e múltiplos <app-group>

  4. tests/sps/validation/test_app_group.py linha 64 - TestAppIdValidation
    Novos testes para @id obrigatório

Como este poderia ser testado manualmente?

Teste 1: Validação de @id obrigatório

cat > test_app_id.xml << 'EOF'
<article xmlns:xlink="http://www.w3.org/1999/xlink">
    <back>
        <app-group>
            <app>  <!-- Sem @id - deve gerar erro -->
                <label>Appendix</label>
            </app>
        </app-group>
    </back>
</article>
EOF

python -c "
from lxml import etree
from packtools.sps.validation.app_group import AppValidation

xml = etree.parse('test_app_id.xml')
params = {
    'app_existence_error_level': 'WARNING',
    'app_id_error_level': 'CRITICAL',
    'app_label_error_level': 'CRITICAL',
    'app_group_wrapper_error_level': 'CRITICAL',
    'app_group_occurrence_error_level': 'CRITICAL',
    'media_alt_text_error_level': 'ERROR',
    'media_transcript_error_level': 'WARNING',
}
results = list(AppValidation(xml, params).validate_app_id())
id_errors = [r for r in results if '@id' in r.get('title', '') and r['response'] == 'CRITICAL']
print(f'Erros @id: {len(id_errors)}')  # Esperado: 1
print(f'Mensagem: {id_errors[0][\"advice\"]}')
"
# Esperado: "Add @id attribute to <app>. Example: <app id=\"app1\">"

Teste 2: Validação de <app> órfão

cat > test_orphan.xml << 'EOF'
<article xmlns:xlink="http://www.w3.org/1999/xlink">
    <back>
        <app id="app1">  <!-- Sem <app-group> wrapper - deve gerar erro -->
            <label>Appendix</label>
        </app>
    </back>
</article>
EOF

python -c "
from lxml import etree
from packtools.sps.validation.app_group import AppValidation

xml = etree.parse('test_orphan.xml')
params = {
    'app_existence_error_level': 'WARNING',
    'app_id_error_level': 'CRITICAL',
    'app_label_error_level': 'CRITICAL',
    'app_group_wrapper_error_level': 'CRITICAL',
    'app_group_occurrence_error_level': 'CRITICAL',
    'media_alt_text_error_level': 'ERROR',
    'media_transcript_error_level': 'WARNING',
}
results = list(AppValidation(xml, params).validate_app_group_wrapper())
wrapper_errors = [r for r in results if 'wrapper' in r.get('title', '').lower() and r['response'] == 'CRITICAL']
print(f'Erros wrapper: {len(wrapper_errors)}')  # Esperado: 1
"

Teste 3: Suite completa

python -m unittest tests.sps.validation.test_app_group -v

# Esperado: Ran 37 tests in X.XXXs - OK

Algum cenário de contexto que queira dar?

Elementos <app> (apêndices/anexos) são elementos pós-textuais opcionais em artigos científicos. Segundo SciELO:

  • Apêndice: texto elaborado pelo autor para complementar argumentação
  • Anexo: texto não elaborado pelo autor para fundamentação/comprovação

A documentação SciELO tem regras estruturais rigorosas:

  1. @id obrigatório em cada <app> (para referenciamento)
  2. <label> obrigatório (título do apêndice/anexo)
  3. <app-group> sempre necessário como wrapper, mesmo com um único <app>
  4. Máximo 1 <app-group> por artigo
  5. Para mídias (vídeo/áudio): recomenda-se alt-text/long-desc + transcrição

Problemas encontrados na implementação original:

  • Validava apenas existência de <app> (~10% conformidade)
  • Não validava @id obrigatório
  • Não validava <label> obrigatório
  • Não detectava <app> órfão (fora de <app-group>)
  • Não detectava múltiplos <app-group>
  • Não validava acessibilidade em mídias
  • Sem internacionalização

Impacto das mudanças:

  • XMLs com <app> sem @id agora serão rejeitados (CRITICAL)
  • XMLs com <app> sem <label> serão rejeitados (CRITICAL)
  • XMLs com <app> fora de <app-group> serão rejeitados (CRITICAL)
  • XMLs com múltiplos <app-group> serão rejeitados (CRITICAL)
  • XMLs com mídia sem alt-text gerarão erro (ERROR)
  • Editores devem ser notificados para adequar XMLs existentes

Exemplo de XML inválido detectado pelas novas validações:

<back>
    <app>  <!-- ERRO: órfão, sem app-group -->
        <!-- ERRO: sem @id -->
        <!-- ERRO: sem <label> -->
        <p>Conteúdo do apêndice</p>
    </app>
</back>

Exemplo de XML válido:

<back>
    <app-group>
        <app id="app1">
            <label>Appendix 1</label>
            <p>Conteúdo do apêndice</p>
        </app>
    </app-group>
</back>

Screenshots

N.A. - Validação backend sem interface gráfica

Quais são tickets relevantes?

N.A. - Implementação baseada em análise de conformidade com documentação SciELO Brasil

Referências

  1. Documentação SPS - <app-group>: Apêndice e Anexo

    • Regra: "Atributo obrigatório para <app>: @id"
    • Regra: "<app> exigem o elemento <label> como título"
    • Regra: "<app-group> deve sempre ser usado como agrupador, mesmo se houver somente uma ocorrência"
    • Regra: "Aparece em <back>: Zero ou uma vez"
    • Regra: "Para acessibilidade recomenda-se que vídeos e áudios venham com descrição"
  2. JATS (Journal Article Tag Suite)

    • Elemento <app>: apêndices e anexos pós-textuais
    • Elemento <app-group>: agrupador de apêndices
  3. Padrão i18n implementado em packtools

    • Arquivo: packtools/sps/validation/aff.py (referência)
    • Arquivo: packtools/sps/validation/alternatives.py (referência)
    • Função: build_response() em utils.py
  4. Modelo de dados

    • packtools/sps/models/app_group.py: classe XmlAppGroup
    • packtools/sps/models/media.py: classe Media para acessibilidade

…orme SciELO

Novas validações obrigatórias:
- validate_app_id(): valida @id obrigatório em <app> (CRITICAL)
- validate_app_label(): valida <label> obrigatório em <app> (CRITICAL)
- validate_app_group_wrapper(): valida estrutura <app-group> e
ocorrência única (CRITICAL)
- validate_media_accessibility(): valida alt-text/long-desc e
transcrição em <media> (ERROR/WARNING)

Melhorias estruturais:
- Implementa internacionalização completa (advice_text e advice_params)
- Detecta <app> órfão (fora de <app-group>)
- Detecta múltiplos <app-group> em <back> (máximo 1 permitido)
- Valida acessibilidade em elementos <media> dentro de <app>
- Usa build_response() ao invés de format_response() para suporte i18n
Testes existentes mantidos:
- test_app_validation_no_app_elements: valida ausência de <app>
- test_app_validation_with_app_elements: valida presença de <app> válido

Novas classes de teste (+35 testes):
- TestAppIdValidation: 3 testes para @id obrigatório
- TestAppLabelValidation: 2 testes para <label> obrigatório
- TestAppGroupWrapperValidation: 4 testes para estrutura <app-group>
  - Valida <app> órfão (fora de <app-group>)
  - Valida múltiplos <app-group> (máximo 1)
  - Valida necessidade de wrapper mesmo com 1 <app>
- TestMediaAccessibilityValidation: 4 testes para acessibilidade em
mídia
  - Valida presença de alt-text ou long-desc
  - Valida referência a transcrição
- TestI18nSupport: 2 testes para internacionalização
- TestCompleteValidation: 1 teste de validação completa
…app-group

Parâmetros adicionados (6 novos):
- app_id_error_level: CRITICAL - valida @id obrigatório em <app>
- app_label_error_level: CRITICAL - valida <label> obrigatório em <app>
- app_group_wrapper_error_level: CRITICAL - valida estrutura <app-group>
- app_group_occurrence_error_level: CRITICAL - valida ocorrência única
- media_alt_text_error_level: ERROR - valida descrição em <media>
- media_transcript_error_level: WARNING - valida transcrição em <media>

Parâmetro mantido:
- app_existence_error_level: WARNING - informa presença/ausência de
<app>
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 implements comprehensive validation for <app-group> and <app> elements according to SciELO Brasil guidelines, significantly increasing validation coverage from 10% to 95%. The implementation adds four critical validations: mandatory @id attribute in <app>, mandatory <label> element in <app>, required <app-group> wrapper (even for single apps), and media accessibility requirements (alt-text/long-desc and transcript references).

Changes:

  • Added four new validation methods: validate_app_id(), validate_app_label(), validate_app_group_wrapper(), and validate_media_accessibility()
  • Migrated from format_response to build_response with full i18n support (advice_text/advice_params)
  • Added 35 new test cases covering all new validations and edge cases
  • Updated validation rules configuration with 6 new error level parameters

Reviewed changes

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

File Description
packtools/sps/validation/app_group.py Implements four new validation methods for app-group compliance (id, label, wrapper, media accessibility) with i18n support
packtools/sps/validation_rules/app_group_rules.json Adds configuration for six new error level parameters (id, label, wrapper, occurrence, media alt-text, media transcript)
tests/sps/validation/test_app_group.py Adds 35 new test cases organized in five test classes covering all validation scenarios and i18n support

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

Comment on lines +181 to +208
else:
# Verificar se há múltiplos <app-group> (deve ser 0 ou 1)
app_groups = self.xmltree.xpath(".//back/app-group")

if len(app_groups) > 1:
yield build_response(
title="Single <app-group> allowed",
parent={
"parent": "article",
"parent_id": None,
"parent_article_type": self.xmltree.get("article-type"),
"parent_lang": self.xmltree.get("{http://www.w3.org/XML/1998/namespace}lang"),
},
item="back",
sub_item="app-group",
validation_type="occurrence",
is_valid=False,
expected="Zero or one <app-group> in <back>",
obtained=f"{len(app_groups)} <app-group> elements found",
advice=f'Merge all {len(app_groups)} <app-group> elements into a single <app-group>.',
data={"count": len(app_groups)},
error_level=self.params["app_group_occurrence_error_level"],
advice_text='Merge all {count} <app-group> elements into a single <app-group>.',
advice_params={
"count": len(app_groups)
}
)

Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

The check for multiple app-group elements is nested inside the else block (line 181), which means it only executes when there are no orphan apps. This creates a logic error where if there are orphan apps, the multiple app-group validation is skipped entirely.

Both validations should run independently:

  1. Check for orphan apps (apps not inside app-group)
  2. Check for multiple app-group elements (maximum of 1 allowed)

The multiple app-group check should be moved outside the if-else block so it always executes, regardless of whether orphan apps exist.

Suggested change
else:
# Verificar se há múltiplos <app-group> (deve ser 0 ou 1)
app_groups = self.xmltree.xpath(".//back/app-group")
if len(app_groups) > 1:
yield build_response(
title="Single <app-group> allowed",
parent={
"parent": "article",
"parent_id": None,
"parent_article_type": self.xmltree.get("article-type"),
"parent_lang": self.xmltree.get("{http://www.w3.org/XML/1998/namespace}lang"),
},
item="back",
sub_item="app-group",
validation_type="occurrence",
is_valid=False,
expected="Zero or one <app-group> in <back>",
obtained=f"{len(app_groups)} <app-group> elements found",
advice=f'Merge all {len(app_groups)} <app-group> elements into a single <app-group>.',
data={"count": len(app_groups)},
error_level=self.params["app_group_occurrence_error_level"],
advice_text='Merge all {count} <app-group> elements into a single <app-group>.',
advice_params={
"count": len(app_groups)
}
)
# Verificar se há múltiplos <app-group> (deve ser 0 ou 1)
app_groups = self.xmltree.xpath(".//back/app-group")
if len(app_groups) > 1:
yield build_response(
title="Single <app-group> allowed",
parent={
"parent": "article",
"parent_id": None,
"parent_article_type": self.xmltree.get("article-type"),
"parent_lang": self.xmltree.get("{http://www.w3.org/XML/1998/namespace}lang"),
},
item="back",
sub_item="app-group",
validation_type="occurrence",
is_valid=False,
expected="Zero or one <app-group> in <back>",
obtained=f"{len(app_groups)} <app-group> elements found",
advice=f'Merge all {len(app_groups)} <app-group> elements into a single <app-group>.',
data={"count": len(app_groups)},
error_level=self.params["app_group_occurrence_error_level"],
advice_text='Merge all {count} <app-group> elements into a single <app-group>.',
advice_params={
"count": len(app_groups)
}
)

Copilot uses AI. Check for mistakes.
Comment on lines +237 to +272
def test_multiple_app_groups_invalid(self):
"""Múltiplos <app-group> devem gerar erro CRITICAL"""
xml = etree.fromstring(
'<article xmlns:xlink="http://www.w3.org/1999/xlink">'
"<back>"
"<app-group>"
'<app id="app1"><label>App 1</label></app>'
"</app-group>"
"<app-group>"
'<app id="app2"><label>App 2</label></app>'
"</app-group>"
"</back>"
"</article>"
)
obtained = list(AppValidation(xml, self.params).validate_app_group_wrapper())

self.assertGreater(len(obtained), 0)
# Deve ter erro sobre múltiplos app-groups
multiple_errors = [r for r in obtained if "Single" in r.get("title", "")]
self.assertGreater(len(multiple_errors), 0)
self.assertEqual(multiple_errors[0]["response"], "CRITICAL")

def test_single_app_needs_app_group(self):
"""Mesmo um único <app> precisa estar em <app-group>"""
xml = etree.fromstring(
'<article xmlns:xlink="http://www.w3.org/1999/xlink">'
"<back>"
'<app id="app1"><label>Single App</label></app>'
"</back>"
"</article>"
)
obtained = list(AppValidation(xml, self.params).validate_app_group_wrapper())

self.assertGreater(len(obtained), 0)
self.assertEqual(obtained[0]["response"], "CRITICAL")

Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

Missing test case for the scenario where both orphan apps and multiple app-group elements exist simultaneously. The current tests only cover each issue separately. Add a test that combines both issues to ensure both validations run independently:

Example test case:

<back>
    <app id="orphan"><label>Orphan</label></app>
    <app-group><app id="app1"><label>App 1</label></app></app-group>
    <app-group><app id="app2"><label>App 2</label></app></app-group>
</back>

This should generate errors for both the orphan app and the multiple app-group elements.

Copilot uses AI. Check for mistakes.
"""
Valida presença obrigatória do elemento <label> em <app>.
Regra SciELO: "<app> exigem o elemento <label> como título do apêndice ou anexo"
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

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

Grammar error in Portuguese docstring. The verb "exigem" (plural) should be "exige" (singular) to agree with the singular subject "". The correct sentence should be: " exige o elemento como título do apêndice ou anexo"

Suggested change
Regra SciELO: "<app> exigem o elemento <label> como título do apêndice ou anexo"
Regra SciELO: "<app> exige o elemento <label> como título do apêndice ou anexo"

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +70
advice_text=None,
advice_params=None
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 advice nunca deveria ser None, sempre tem que ter alguma instrução para o usuário mesmo que seja óbvia, no entanto, se for "óbvia" pode ser melhorada, por exemplo, no lugar de dizer complete com o autor, explique use a tag X para representar o nome do autor e Y para representar o sobrenome, use ... para ....

Comment on lines +126 to +128
advice=f'Add <label> element to <app id="{app.get("id", "?")}">. '
f'Example: <app id="{app.get("id", "app1")}"><label>Appendix 1</label></app>',
data=app,
Copy link
Member

@robertatakenaka robertatakenaka Jan 26, 2026

Choose a reason for hiding this comment

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

@Rossi-Luciano user variáveis no lugar de app.get... para evitar problemas com aspas incompatíveis. Além disso esta prática deixa o código mais enxuto e evita ter app.get repetidas vezes

# Buscar <app> que estão diretamente em <back>, sem <app-group>
orphan_apps = self.xmltree.xpath(".//back/app")

if orphan_apps:
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 acho que uma coisa não anula a outra. Acho que na pior das hipóteses, pode haver órfãs e app dentro de app-group. Que tal dividir em 2 validações separadas e fazer uma chamas às 2 funções?

for app in self.apps:
media_list = app.get("media", [])

for media in media_list:
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 será que não é possível reuso de validação? (no caso MediaValidation ?)

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