-
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) | |
| } | |
| ) |
| 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.
| """ | ||
| 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" |
| 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 ....
| 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
| # 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?
| 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 ?)
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