Corrige document licenca de uso e issue publication date #133
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Comenta funções pids_and_their_records e get_acron_db_path - Mantém código comentado para referência futura - Prepara para refatoração completa do módulo controller
- Comenta imports e funções do módulo migration - Mantém código comentado para preservar lógica de migração - Prepara terreno para nova arquitetura de migração de dados
- Adiciona validação explícita do tamanho do article_pid - Lança FileNotFoundError com mensagem descritiva quando arquivo ID não existe - Lança ValueError quando article_pid tem tamanho inválido - Mantém imports de Document, Issue, Journal com comentário explicativo - Melhora rastreabilidade de erros na busca de arquivos de artigos
- Remove logging.info redundante que poluía os logs - Mantém funcionalidade de conversão de registros ISIS intacta
- Usa h_record.issue_publication_date ao invés de self.issue.issue_publication_date - Refatora license_texts para processar lista de licenças ao invés de dict - Adiciona fallback para license_code quando issue_licenses não existe - Melhora extração de URL e texto das licenças usando ElementTree - Adiciona tratamento de exceções ao processar HTML de licenças - Garante que sempre retorna dict com license_code quando disponível
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request improves error handling in the SciELo Classic Website migration system and prepares the codebase for future refactoring. The main focus is on fixing data source issues for licenses and publication dates while making error conditions more explicit.
- Fixes the source for
issue_publication_dateto use the correcth_recordproperty instead of the non-existentself.issueproperty - Refactors license text processing to correctly handle list-based data structure and add fallback logic for missing license information
- Enhances error reporting in file path resolution with explicit exceptions instead of silent failures
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scielo_classic_website/models/document.py | Corrects publication date source and refactors license processing to handle list structure with proper fallbacks |
| scielo_classic_website/classic_ws.py | Adds explicit error handling with FileNotFoundError and ValueError for invalid article PIDs |
| scielo_classic_website/iid2json/id2json3.py | Removes redundant logging statement to reduce log noise |
| scielo_classic_website/migration.py | Comments out unused migration functions in preparation for refactoring |
| scielo_classic_website/controller.py | Comments out unused controller functions in preparation for refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| item["code"] = license_code | ||
| try: | ||
| html = ET.fromstring(f"<root>{item['html']}</root>") | ||
| item["url"] = html.find(".//a").get("href") |
There was a problem hiding this comment.
The use of html.find(".//a").get("href") can raise an AttributeError if no anchor tag is found in the HTML. When find() returns None, calling .get("href") on it will fail. Consider adding a None check or using a safer pattern like html.find(".//a") followed by a conditional check before accessing the href attribute.
Suggested change
| item["url"] = html.find(".//a").get("href") | |
| anchor = html.find(".//a") | |
| if anchor is not None: | |
| item["url"] = anchor.get("href") |
Comment on lines
+316
to
+317
| except Exception as e: | ||
| logging.exception(e) |
There was a problem hiding this comment.
The broad Exception catch makes it difficult to identify and debug specific issues with HTML parsing or attribute access. Consider catching more specific exceptions (e.g., AttributeError, ET.ParseError) to provide better error context and handling.
Suggested change
| except Exception as e: | |
| logging.exception(e) | |
| except (AttributeError, KeyError, ET.ParseError) as e: | |
| logging.exception(f"Error parsing license HTML for language={language}: {e}") |
|
|
||
| from scielo_classic_website.iid2json import id2json3 | ||
| from scielo_classic_website.isisdb.isis_cmd import ISISCommader | ||
| # manter Document, Issue, Journal imports para evitar quebra em outras partes do sistema |
There was a problem hiding this comment.
The comment uses Portuguese ("manter Document, Issue, Journal imports para evitar quebra em outras partes do sistema"). Consider translating to English for consistency with the codebase, or verify if Portuguese is the standard for this project.
Suggested change
| # manter Document, Issue, Journal imports para evitar quebra em outras partes do sistema | |
| # Keep Document, Issue, and Journal imports to avoid breaking other parts of the system |
| f"ID file not found for article pid={article_pid} at {path}" | ||
| ) | ||
| else: | ||
| raise ValueError(f"Found article_pid={article_pid} invalid size") |
There was a problem hiding this comment.
The error message format "Found article_pid={article_pid} invalid size" is grammatically awkward. Consider rephrasing to "Invalid article_pid size: {article_pid} (expected 23 characters)" or "article_pid must be 23 characters, got {len(article_pid) if article_pid else 'None'}" for better clarity.
Suggested change
| raise ValueError(f"Found article_pid={article_pid} invalid size") | |
| raise ValueError( | |
| f"article_pid must be 23 characters, got {len(article_pid) if article_pid else 'None'}" | |
| ) |
Comment on lines
+6
to
+31
| # def pids_and_their_records(source_file_path, db_type): | ||
| # if not source_file_path: | ||
| # return [] | ||
|
|
||
| id_function = id2json3.article_id | ||
| if db_type == "title": | ||
| id_function = id2json3.journal_id | ||
| elif db_type == "issue": | ||
| id_function = id2json3.issue_id | ||
| # id_function = id2json3.article_id | ||
| # if db_type == "title": | ||
| # id_function = id2json3.journal_id | ||
| # elif db_type == "issue": | ||
| # id_function = id2json3.issue_id | ||
|
|
||
| # get id_file_path | ||
| id_file_path = isis_cmd.get_id_file_path(source_file_path) | ||
| return id2json3.pids_and_their_records(id_file_path, id_function) | ||
| # # get id_file_path | ||
| # id_file_path = isis_cmd.get_id_file_path(source_file_path) | ||
| # return id2json3.pids_and_their_records(id_file_path, id_function) | ||
|
|
||
|
|
||
| def get_acron_db_path(acron, id_folder_path=None): | ||
| config.check_migration_sources() | ||
| # def get_acron_db_path(acron, id_folder_path=None): | ||
| # config.check_migration_sources() | ||
|
|
||
| db_path = config.get_bases_acron(acron) | ||
| print("db:", db_path) | ||
| if id_folder_path: | ||
| id_file_path = os.path.join(id_folder_path, f"{acron}.id") | ||
| id_file_path = isis_cmd.create_id_file(db_path, id_file_path) | ||
| db_path = id_file_path | ||
| print(f"{id_file_path} - size: {size(id_file_path)} bytes") | ||
| return db_path | ||
| # db_path = config.get_bases_acron(acron) | ||
| # print("db:", db_path) | ||
| # if id_folder_path: | ||
| # id_file_path = os.path.join(id_folder_path, f"{acron}.id") | ||
| # id_file_path = isis_cmd.create_id_file(db_path, id_file_path) | ||
| # db_path = id_file_path | ||
| # print(f"{id_file_path} - size: {size(id_file_path)} bytes") | ||
| # return db_path |
There was a problem hiding this comment.
This comment appears to contain commented-out code.
Comment on lines
+5
to
+25
| # def get_document_pids(from_date, to_date): | ||
| # return controller.isis_cmd.get_document_pids(from_date, to_date) | ||
|
|
||
| def get_document_pids(from_date, to_date): | ||
| return controller.isis_cmd.get_document_pids(from_date, to_date) | ||
|
|
||
| # def get_paragraphs_records(pid): | ||
| # id_file_path = config.get_paragraphs_id_file_path(pid) | ||
| # return controller.pids_and_their_records(id_file_path, "artigo") | ||
|
|
||
| def get_paragraphs_records(pid): | ||
| id_file_path = config.get_paragraphs_id_file_path(pid) | ||
| return controller.pids_and_their_records(id_file_path, "artigo") | ||
|
|
||
| # def get_records_by_pid(pid): | ||
| # source_path = controller.isis_cmd.get_document_isis_db(pid) | ||
| # return controller.pids_and_their_records(source_path, "artigo") | ||
|
|
||
| def get_records_by_pid(pid): | ||
| source_path = controller.isis_cmd.get_document_isis_db(pid) | ||
| return controller.pids_and_their_records(source_path, "artigo") | ||
|
|
||
| # def get_records_by_source_path(db_type, source_path): | ||
| # return controller.pids_and_their_records(source_path, db_type) | ||
|
|
||
| def get_records_by_acron(acron, id_folder_path=None): | ||
| source_path = controller.get_acron_db_path(acron, id_folder_path) | ||
| return controller.pids_and_their_records(source_path, "artigo") | ||
|
|
||
|
|
||
| def get_records_by_source_path(db_type, source_path): | ||
| return controller.pids_and_their_records(source_path, db_type) | ||
|
|
||
|
|
||
| def get_issue_files(acron, issue_folder): | ||
| issue_files = IssueFiles(acron, issue_folder) | ||
| return issue_files.files | ||
| # def get_issue_files(acron, issue_folder): | ||
| # issue_files = IssueFiles(acron, issue_folder) | ||
| # return issue_files.files |
There was a problem hiding this comment.
This comment appears to contain commented-out code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request: Melhorias no tratamento de erros e preparação para refatoração
Descrição
Este PR implementa melhorias críticas no tratamento de erros e prepara o terreno para uma refatoração maior do sistema de migração, focando em tornar o código mais robusto e manutenível.
Mudanças Principais
🔧 Tratamento de Erros Aprimorado
article_pidcom mensagens de erro descritivasFileNotFoundErrorquando arquivo ID não existeValueErrorpara PIDs com tamanho inválido🐛 Correções Importantes
issue_publication_datelicense_code🧹 Limpeza de Código
Impacto
Testes
As mudanças mantêm compatibilidade total com o código existente. Todos os testes existentes continuam passando.
Próximos Passos
Este PR prepara o terreno para: