-
Notifications
You must be signed in to change notification settings - Fork 24
Implementa validação completa de <app-group> para conformidade SPS #1068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implementa validação completa de <app-group> para conformidade SPS #1068
Conversation
…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>
There was a problem hiding this 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(), andvalidate_media_accessibility() - Migrated from
format_responsetobuild_responsewith 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.
| 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) | ||
| } | ||
| ) | ||
|
|
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
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:
- Check for orphan apps (apps not inside app-group)
- 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.
| 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) | |
| } | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A verificação de múltiplos <app-group> foi movido para fora do bloco if/else e executa independentemente da existência de apps órfãos.
| 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") | ||
|
|
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adicionado teste test_orphan_app_and_multiple_app_groups() que valida ambos os erros simultaneamente.
| """ | ||
| 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" |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
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"
| 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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrigido. Ajustada concordância verbal na docstring do método validate_app_label().
| advice_text=None, | ||
| advice_params=None |
There was a problem hiding this comment.
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 ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertatakenaka esse caso se refere ao "sucesso" da validação, ou seja, existe <app> e serão geradas respostas identificando cada ocorrência da tag.
| 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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementado. Método validate_app_label() usa variáveis app_id_or_placeholder e app_id_or_example_fix para evitar repetição.
| # Buscar <app> que estão diretamente em <back>, sem <app-group> | ||
| orphan_apps = self.xmltree.xpath(".//back/app") | ||
|
|
||
| if orphan_apps: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refatorado. validate_app_group_wrapper() agora delega para validate_orphan_apps() e validate_multiple_app_groups(), garantindo separação de responsabilidades.
| for app in self.apps: | ||
| media_list = app.get("media", []) | ||
|
|
||
| for media in media_list: |
There was a problem hiding this comment.
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 ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementado. Removido validate_media_accessibility() pois validações de acessibilidade já são tratadas por AccessibilityDataValidation (via VisualResourceBaseValidation). Removidos também os parâmetros media_alt_text_error_level e media_transcript_error_level de app_group_rules.json.
| advice="Consider adding an <app> element to include additional content such as appendices.", | ||
| data=None, | ||
| error_level=self.params["app_existence_error_level"], | ||
| advice_text="Consider adding an <app> element to include additional content such as appendices.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rossi-Luciano adicionar a marca de tradução
| advice='Add @id attribute to <app>. Example: <app id="app1">', | ||
| data=app, | ||
| error_level=self.params["app_id_error_level"], | ||
| advice_text='Add @id attribute to <app>. Example: <app id="app1">', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rossi-Luciano marca de tradução
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:
<app>(CRITICAL)<label>obrigatório em<app>(CRITICAL)<app-group>como wrapper obrigatório, mesmo para um único<app>(CRITICAL)<media>: alt-text/long-desc e referência a transcrição (ERROR/WARNING)Melhorias adicionais:
<app>órfão (fora de<app-group>)<app-group>(máximo 1 permitido)Onde a revisão poderia começar?
packtools/sps/validation/app_group.py linha 63 - validate_app_id()
Nova validação: verifica @id obrigatório em cada
<app>packtools/sps/validation/app_group.py linha 88 - validate_app_label()
Nova validação: verifica
<label>obrigatório em cada<app>packtools/sps/validation/app_group.py linha 113 - validate_app_group_wrapper()
Nova validação: detecta
<app>órfão e múltiplos<app-group>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
Teste 2: Validação de
<app>órfãoTeste 3: Suite completa
python -m unittest tests.sps.validation.test_app_group -v # Esperado: Ran 37 tests in X.XXXs - OKAlgum 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:A documentação SciELO tem regras estruturais rigorosas:
<app>(para referenciamento)<label>obrigatório (título do apêndice/anexo)<app-group>sempre necessário como wrapper, mesmo com um único<app><app-group>por artigoProblemas encontrados na implementação original:
<app>(~10% conformidade)<label>obrigatório<app>órfão (fora de<app-group>)<app-group>Impacto das mudanças:
<app>sem @id agora serão rejeitados (CRITICAL)<app>sem<label>serão rejeitados (CRITICAL)<app>fora de<app-group>serão rejeitados (CRITICAL)<app-group>serão rejeitados (CRITICAL)Exemplo de XML inválido detectado pelas novas validações:
Exemplo de XML válido:
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
Documentação SPS -
<app-group>: Apêndice e Anexo<app>: @id"<app>exigem o elemento<label>como título"<app-group>deve sempre ser usado como agrupador, mesmo se houver somente uma ocorrência"<back>: Zero ou uma vez"JATS (Journal Article Tag Suite)
<app>: apêndices e anexos pós-textuais<app-group>: agrupador de apêndicesPadrão i18n implementado em packtools
Modelo de dados