Feat: Cria campos no formulário de Pages para referênciar páginas e navegação entre páginas Pais e Filhas#353
Conversation
|
@samuelveigarangel este PR não resolve o 335. Verifique qual é o issue correspondente e corrija a descrição |
There was a problem hiding this comment.
Pull request overview
Este PR adiciona navegação hierárquica para páginas “Sobre o SciELO” (pai/filhas), breadcrumbs e melhora a exibição de idiomas “fora do trio” (EN/PT/ES) nos menus de artigo.
Changes:
- Cria rotas e templates para listar páginas “Sobre” em árvore e exibir detalhe com breadcrumbs/relacionadas.
- Adiciona helpers (breadcrumbs/ordenação) e queries de controller para páginas raiz e “free pages”.
- Ajusta rótulos de idioma em menus de artigo para melhor fallback/normalização.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| opac/webapp/utils/utils.py | Adiciona helpers para breadcrumbs e ordenação de filhos. |
| opac/webapp/main/views.py | Nova rota de detalhe /about/<slug> + listagem /about/ e rota /free/<slug>. |
| opac/webapp/controllers.py | Adiciona queries para páginas raiz “about” e páginas do tipo “free”. |
| opac/webapp/config/lang_names.py | Adiciona dicionário e função de fallback para nomes de idiomas. |
| opac/webapp/templates/collection/includes/levelMenu.html | Inclui breadcrumbs no menu de nível da coleção. |
| opac/webapp/templates/collection/includes/_page_tree.html | Novo include recursivo para renderizar árvore de páginas. |
| opac/webapp/templates/collection/about.html | Passa a renderizar a lista de páginas via árvore (_page_tree). |
| opac/webapp/templates/collection/about_detail.html | Novo template de detalhe com páginas relacionadas (ancestors/descendants). |
| opac/webapp/templates/article/includes/levelMenu_texts.html | Exibe o fallback de idioma em uppercase. |
| opac/webapp/templates/article/includes/levelMenu_pdf.html | Melhora label de PDF para idiomas “outros”. |
| opac/webapp/templates/article/includes/levelMenu_abstracts.html | Exibe idioma do resumo em uppercase. |
| opac/webapp/templates/journal/includes/levelMenu.html | Alterações nos links do menu do periódico (atualmente quebram HTML/navegação). |
| opac/webapp/templates/journal/detail.html | Alterações nos links para o último número e âncoras do sumário (atualmente quebram navegação). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <h3>{% trans %}Sumário{% endtrans %}</h3> | ||
| <ul> | ||
| {% for section in sections %} | ||
| <li><a href="{{ url_for('.issue_toc', url_seg=journal.url_segment, url_seg_issue=last_issue.url_segment) }}#{{ section|upper }}">{{ section }}</a></li> | ||
| <li><a href="##{{ section|upper }}">{{ section }}</a></li> | ||
| {% endfor %} |
There was a problem hiding this comment.
Os links do sumário foram alterados para href="##{{ section|upper }}" (duplo #). Isso gera âncoras inválidas/inesperadas. Use apenas #{{ section|upper }} ou mantenha a URL completa do issue_toc + fragmento como estava antes.
| <h3>{% trans %}Sumário{% endtrans %}</h3> | ||
| <ul> | ||
| {% for section in sections %} | ||
| <li><a href="{{ url_for('.issue_toc', url_seg=journal.url_segment, url_seg_issue=last_issue.url_segment) }}#{{ section|upper }}">{{ section }}</a></li> | ||
| <li><a href="##{{ section|upper }}">{{ section }}</a></li> | ||
| {% endfor %} |
There was a problem hiding this comment.
Os links da seção "Sumário" foram alterados para href="##{{ section|upper }}" (duplo #). Isso cria fragmentos inválidos/inesperados. Use apenas #{{ section|upper }} ou mantenha a URL completa do issue_toc + fragmento.
| @main.route("/free/<string:slug_name>", methods=["GET"]) | ||
| def free_page(slug_name): | ||
| language = session.get("lang", get_locale()) | ||
| page = controllers.get_free_page_by_slug(slug_name, lang=language, is_draft=False) | ||
|
|
There was a problem hiding this comment.
A chamada controllers.get_free_page_by_slug(..., is_draft=False) vai falhar porque o controller não aceita o parâmetro is_draft. Alinhe a assinatura do controller com o uso aqui (ou remova o argumento) e garanta que a função retorne uma única página.
| @main.route("/about/", methods=["GET"]) | ||
| @main.route("/about/<string:slug_name>", methods=["GET"]) | ||
| @cache.cached(key_prefix=cache_key_with_lang_with_qs) | ||
| def about_collection(slug_name=None): | ||
| def about_collection(): |
There was a problem hiding this comment.
Ao remover a rota /about/<slug_name> do endpoint about_collection (agora só /about/), chamadas existentes como url_for('main.about_collection', slug_name=...) deixam de funcionar (há testes e possivelmente links externos usando isso). Considere manter compatibilidade (rota antiga com redirect para page_about_detail) ou atualizar todas as referências.
| {% set child_nodes = node.get_children() %} | ||
| {% if child_nodes %} | ||
| {% set nodes = child_nodes %} | ||
| {% set ul_class = "networkList--children" %} | ||
| {% include "collection/includes/_page_tree.html" %} |
There was a problem hiding this comment.
Aqui o template sobrescreve nodes/ul_class antes do {% include %}. Como o include compartilha o contexto, isso pode vazar para as próximas iterações do loop e renderizar a árvore incorretamente. Prefira {% with nodes=child_nodes, ul_class='networkList--children' %}{% include ... %}{% endwith %} ou um loop recursive.
| {% if journal.last_issue.cover_url %} | ||
| <a href="{{ url_for('.issue_toc', url_seg=journal.url_segment, url_seg_issue=last_issue.url_segment) }}"> | ||
| <a href="#"> | ||
| <img src="{{ latest_issue.cover_url }}" class="image" alt="{% trans %}Capa do número mais recente{% endtrans %}" /> <!-- a imagem é opcional --> | ||
| </a> |
There was a problem hiding this comment.
Neste segundo bloco do template, o link da capa do número mais recente foi alterado para href="#", quebrando a navegação para o sumário. Reponha o url_for('.issue_toc', ...) como no comportamento anterior.
| {% if not next_item and last_issue %} | ||
| <a title="{% trans %}número atual{% endtrans %}" href="{{ url_for('.issue_toc', url_seg=journal.url_segment, url_seg_issue=last_issue.url_segment) }}" class="btn {{ mmenu.endpoint_selected(view_name='main.issue_grid') }} {% if not 'grid' in request.url_rule.rule %} active {% else %} unselected {% endif %}{% if mmenu.endpoint_selected(view_name='main.journal_detail') %} unselected {% endif %}"> | ||
|
|
||
| {% trans %}Número atual{% endtrans %} | ||
| </a> | ||
| {% else %} |
There was a problem hiding this comment.
No bloco {% if not next_item and last_issue %} o <a ...> de "Número atual" foi removido, mas o </a> permaneceu. Isso gera HTML inválido e quebra o link do número atual. Restaure a tag de abertura (com href=url_for('.issue_toc', ...) e classes) ou ajuste o bloco para não renderizar </a> sem <a>.
| breadcrumbs = utils.build_breadcrumbs(page) | ||
| children = page.get_children() | ||
| context = { | ||
| "children": children, | ||
| "breadcrumbs": breadcrumbs, |
There was a problem hiding this comment.
children = page.get_children() é colocado no contexto, mas não é usado no template collection/about_detail.html. Isso pode gerar trabalho/query desnecessário. Remova children do view ou passe a usar esse valor no template em vez de chamar métodos do modelo repetidamente.
opac/webapp/controllers.py
Outdated
| def get_free_page_by_slug(slug, lang): | ||
| return Pages.objects(slug_name=slug, is_draft=False, page_type="free", language=lang) |
There was a problem hiding this comment.
get_free_page_by_slug retorna um QuerySet (Pages.objects(...)) e não uma instância única (.first()), e também não aceita is_draft apesar do uso no view. Padronize para retornar uma página única (ex.: .first()) e alinhe a assinatura com get_page_by_slug_name (ex.: is_draft como parâmetro).
| {% if page.get_descendants() or page.get_ancestors() %} | ||
| <section class="child-pages"> | ||
| <h2>{% trans %} Páginas relacionadas {% endtrans %}</h2> | ||
| <ul> | ||
| {% if page.parent_page %} |
There was a problem hiding this comment.
page.get_descendants() e page.get_ancestors() são chamados no if e depois novamente nos for. Se esses métodos consultarem o banco, isso duplica trabalho. Armazene em variáveis ({% set ancestors = ... %}, {% set descendants = ... %}) e reutilize.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% set nodes = child_nodes %} | ||
| {% set ul_class = "networkList--children" %} | ||
| {% include "collection/includes/_page_tree.html" %} |
There was a problem hiding this comment.
node.get_children() result is assigned back into the nodes loop variable ({% set nodes = child_nodes %}) before including the same template. In Jinja this leaks to the outer scope and can lead to incorrect rendering (siblings may be rendered with the wrong nodes/ul_class) and makes the recursion hard to reason about. Prefer a proper recursive loop/macro, or include with explicit local context (e.g., pass nodes=child_nodes and ul_class=... without overwriting the outer variables).
| {% set nodes = child_nodes %} | |
| {% set ul_class = "networkList--children" %} | |
| {% include "collection/includes/_page_tree.html" %} | |
| {% include "collection/includes/_page_tree.html" with nodes=child_nodes, ul_class="networkList--children" %} |
| {% if page %} | ||
|
|
||
| {% endif %} |
There was a problem hiding this comment.
The template ends with an empty {% if page %} ... {% endif %} block. This is dead code and makes the template harder to maintain; remove the redundant conditional (or add the intended content).
| {% if page %} | |
| {% endif %} | |
opac/webapp/config/lang_names.py
Outdated
|
|
||
| def get_original_lang_name(code): | ||
| return LANG_NAMES.get(code, (None, code))[0] | ||
|
|
||
|
|
||
| def display_lang_name_fallback(locale, code): | ||
| lang = None | ||
| try: | ||
| lang = LANGS_FALLBACK.get(locale, {}).get(str(code).lower()) | ||
| except: | ||
| return code | ||
| finally: | ||
| return lang or code | ||
|
|
There was a problem hiding this comment.
display_lang_name_fallback() lowercases code (str(code).lower()), but LANGS_FALLBACK contains mixed-case keys like zh-Hans/zh-Hant. As a result those mappings will never be hit and the UI will fall back to the raw code. Normalize the dict keys to lowercase (and use lowercase lookups), or stop lowercasing the incoming code and instead normalize it consistently (e.g., BCP47 canonicalization).
| def get_original_lang_name(code): | |
| return LANG_NAMES.get(code, (None, code))[0] | |
| def display_lang_name_fallback(locale, code): | |
| lang = None | |
| try: | |
| lang = LANGS_FALLBACK.get(locale, {}).get(str(code).lower()) | |
| except: | |
| return code | |
| finally: | |
| return lang or code | |
| LANGS_FALLBACK_NORMALIZED = { | |
| str(locale).lower(): {str(k).lower(): v for k, v in mapping.items()} | |
| for locale, mapping in LANGS_FALLBACK.items() | |
| } | |
| def get_original_lang_name(code): | |
| return LANG_NAMES.get(code, (None, code))[0] | |
| def display_lang_name_fallback(locale, code): | |
| locale_key = str(locale).lower() if locale is not None else "" | |
| code_key = str(code).lower() if code is not None else "" | |
| lang = LANGS_FALLBACK_NORMALIZED.get(locale_key, {}).get(code_key) | |
| return lang or code | |
opac/webapp/config/lang_names.py
Outdated
| lang = None | ||
| try: | ||
| lang = LANGS_FALLBACK.get(locale, {}).get(str(code).lower()) | ||
| except: | ||
| return code | ||
| finally: | ||
| return lang or code |
There was a problem hiding this comment.
The try/except/finally structure here is misleading: the return in finally will always run and overrides the return in except, making the except block effectively dead. Also, the bare except: will hide unexpected errors. Simplify to a straight lookup and only catch specific exceptions if truly needed.
| lang = None | |
| try: | |
| lang = LANGS_FALLBACK.get(locale, {}).get(str(code).lower()) | |
| except: | |
| return code | |
| finally: | |
| return lang or code | |
| lang = LANGS_FALLBACK.get(locale, {}).get(str(code).lower()) | |
| return lang or code |
| @main.route("/about/<path:slug_name>", methods=["GET"]) | ||
| def page_about_detail(slug_name): | ||
| language = session.get("lang", get_locale()) | ||
| page = controllers.get_page_by_slug_name(slug_name, lang=language, is_draft=False) | ||
|
|
||
| if not page: | ||
| abort(404, _("Página não encontrada")) | ||
| breadcrumbs = utils.build_breadcrumbs(page) | ||
| context = { | ||
| "breadcrumbs": breadcrumbs, | ||
| "page": page, | ||
| } | ||
| return render_template("collection/about_detail.html", **context) |
There was a problem hiding this comment.
The PR description mentions adding new fields to the Pages admin form (parent/child navigation), but this diff doesn't include any changes under opac/webapp/admin/ or the admin templates to expose those fields. If the form changes are required for this feature, they seem to be missing from the PR; otherwise, the PR description should be updated to match what’s actually shipped.
| @main.route("/about/", methods=["GET"]) | ||
| @main.route("/about/<string:slug_name>", methods=["GET"]) | ||
| @cache.cached(key_prefix=cache_key_with_lang_with_qs) | ||
| def about_collection(slug_name=None): | ||
| def about_collection(): | ||
| language = session.get("lang", get_locale()) | ||
|
|
||
| context = {} | ||
| page = None | ||
| if slug_name: | ||
| # caso seja uma página | ||
| page = controllers.get_page_by_slug_name(slug_name, language) | ||
| if not page: | ||
| abort(404, _("Página não encontrada")) | ||
| context["page"] = page | ||
| else: | ||
| # caso não seja uma página é uma lista | ||
| pages = controllers.get_pages_by_lang(language) | ||
| context["pages"] = pages | ||
| pages = controllers.get_page_about_root(language) | ||
|
|
||
| context["pages"] = pages.order_by("order") | ||
| return render_template("collection/about.html", **context) |
There was a problem hiding this comment.
about_collection() now always renders the list view (it no longer accepts a slug_name and never provides page in the template context). However, collection/about.html still contains a full {% if page %} rendering branch, which is now unreachable and can cause confusion during future changes. Consider removing the unused template branch or reintroducing page rendering via a dedicated route/template (as done for page_about_detail).
O que esse PR faz?
Onde a revisão poderia começar?
Pelos commits
Como este poderia ser testado manualmente?
Algum cenário de contexto que queira dar?
Essa PR depende de scieloorg/opac_schema#116
scieloorg/opac-airflow#493
e da atualizacao das dependencias: https://github.com/scieloorg/docker-airflow
Screenshots
Quais são tickets relevantes?
CLOSE 335 (este número está errado)
Referências