Skip to content

Agregar markup_doc y fixtures DOCX#58

Open
eduranm wants to merge 3 commits intoscieloorg:mainfrom
eduranm:issue-02
Open

Agregar markup_doc y fixtures DOCX#58
eduranm wants to merge 3 commits intoscieloorg:mainfrom
eduranm:issue-02

Conversation

@eduranm
Copy link
Copy Markdown
Contributor

@eduranm eduranm commented Apr 20, 2026

O que esse PR faz?

Agrega la base del módulo markup_doc e incorpora fixtures DOCX para pruebas

Incluye:

  • registro de markup_doc;
  • incorporación del directorio markup_doc/;
  • incorporación del directorio fixtures/;

Onde a revisão poderia começar?

Por commits

Como este poderia ser testado manualmente?

Levantar el entorno;
Verificar que markup_doc esté registrada;
Probar el flujo base con alguno de los archivos en fixtures/.

Algum cenário de contexto que queira dar?

Deja preparada la base para continuar con las siguientes funcionalidades de markup_doc.

Screenshots

N/A

Quais são tickets relevantes?

#57

Referências

  • N/A

Copy link
Copy Markdown
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 introduces the initial markup_doc Django app to support a base DOCX markup flow in MarkAPI, including Wagtail admin registration and a sample DOCX fixture for manual testing.

Changes:

  • Added markup_doc app with initial models, Wagtail hooks/viewsets, and DB migrations.
  • Added external catalog sync helpers and a Celery task for journal synchronization.
  • Added a DOCX fixture under fixtures/ and registered markup_doc in project settings.

Reviewed changes

Copilot reviewed 11 out of 17 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
config/settings/base.py Registers markup_doc in INSTALLED_APPS.
markup_doc/apps.py Adds the Django AppConfig for the new app.
markup_doc/models.py Introduces core models/proxies and Wagtail edit handlers for DOCX + markup data.
markup_doc/wagtail_hooks.py Registers Wagtail snippet viewsets/group and adds preconditions around collection/journal availability.
markup_doc/sync_api.py Implements external API sync for collections/journals.
markup_doc/tasks.py Adds a Celery task wrapper for journal sync.
markup_doc/migrations/0001_initial.py Creates initial schema for the new app.
markup_doc/migrations/0002_alter_articledocx_estatus_and_more.py Aligns estatus field choices/defaults.
markup_doc/forms.py Placeholder forms module (currently only an import).
markup_doc/choices.py Adds label/ordering constants used by Wagtail blocks.
markup_doc/tests.py Placeholder test module.
fixtures/e14790.docx Adds a sample DOCX file for manual testing.

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

Comment thread markup_doc/models.py
Comment on lines +81 to +89
def update(cls, title, estatus):
try:
obj = cls.get(title=title)
except (cls.DoesNotExist, ValueError):
pass

obj.estatus = estatus
obj.save()
return obj
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

ArticleDocx.update() swallows DoesNotExist/ValueError and then uses obj anyway, which will raise UnboundLocalError when the record doesn't exist. Decide on the intended behavior (re-raise, return None, or create a new object) and implement it explicitly.

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/models.py
Comment on lines +428 to +436
def update(cls, title, estatus):
try:
obj = cls.get(title=title)
except (cls.DoesNotExist, ValueError):
pass

obj.estatus = estatus
obj.save()
return obj
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

ArticleDocxMarkup.update() has the same issue as ArticleDocx.update(): if the object doesn't exist, the exception is swallowed and obj is still referenced, causing UnboundLocalError. Implement explicit behavior for the not-found case.

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/models.py
return str(self)

def __str__(self):
return self.title
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

JournalModel.__str__() returns self.title, but title is nullable; returning None from __str__ raises TypeError. Ensure __str__ always returns a string (e.g., fallback to an empty string or another identifier).

Suggested change
return self.title
return self.title or ""

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/models.py
Comment on lines +1 to +25
import os
import sys
import requests

from django.db import models
from django.utils.translation import gettext_lazy as _
from django import forms
from django.utils.html import format_html
from django.urls import reverse

from modelcluster.fields import ParentalKey
from modelcluster.models import ClusterableModel
from wagtail.admin.panels import FieldPanel, InlinePanel, ObjectList, TabbedInterface
from wagtailautocomplete.edit_handlers import AutocompletePanel
from wagtail.documents.models import Document

from core.forms import CoreAdminModelForm
from core.choices import LANGUAGE
from core.models import (
CommonControlField,
Language,
TextWithLang
)
from wagtail.fields import StreamField
from wagtail.blocks import StructBlock, TextBlock, CharBlock, ChoiceBlock, ListBlock, StreamBlock
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

This module currently has several unused imports (e.g., os, sys, requests, ParentalKey, InlinePanel, Document, Language, TextWithLang, CharBlock, ListBlock). With flake8 enabled, these will fail CI (F401). Remove unused imports or start using them in the implementation.

Suggested change
import os
import sys
import requests
from django.db import models
from django.utils.translation import gettext_lazy as _
from django import forms
from django.utils.html import format_html
from django.urls import reverse
from modelcluster.fields import ParentalKey
from modelcluster.models import ClusterableModel
from wagtail.admin.panels import FieldPanel, InlinePanel, ObjectList, TabbedInterface
from wagtailautocomplete.edit_handlers import AutocompletePanel
from wagtail.documents.models import Document
from core.forms import CoreAdminModelForm
from core.choices import LANGUAGE
from core.models import (
CommonControlField,
Language,
TextWithLang
)
from wagtail.fields import StreamField
from wagtail.blocks import StructBlock, TextBlock, CharBlock, ChoiceBlock, ListBlock, StreamBlock
from django.db import models
from django.utils.translation import gettext_lazy as _
from django import forms
from django.utils.html import format_html
from django.urls import reverse
from modelcluster.models import ClusterableModel
from wagtail.admin.panels import FieldPanel, ObjectList, TabbedInterface
from wagtailautocomplete.edit_handlers import AutocompletePanel
from core.forms import CoreAdminModelForm
from core.choices import LANGUAGE
from core.models import CommonControlField
from wagtail.fields import StreamField
from wagtail.blocks import StructBlock, TextBlock, ChoiceBlock, StreamBlock

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/forms.py
@@ -0,0 +1 @@
from wagtail.admin.forms.models import WagtailAdminModelForm
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

markup_doc/forms.py only imports WagtailAdminModelForm but doesn't use or export it, which will trigger flake8 unused-import failures. Either remove this file/import or define the intended form subclass here and use it from the models/viewsets.

Suggested change
from wagtail.admin.forms.models import WagtailAdminModelForm
"""Form definitions for the markup_doc app."""

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/models.py
Comment on lines +414 to +417
def create(cls, title, doi):
obj = cls()
obj.title = title
obj.doi = doi
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

ArticleDocxMarkup.create() assigns obj.doi, but ArticleDocxMarkup doesn't define a doi field. This will raise AttributeError at runtime. Either add the missing field or remove/update the factory method signature to match the actual model fields.

Suggested change
def create(cls, title, doi):
obj = cls()
obj.title = title
obj.doi = doi
def create(cls, title):
obj = cls()
obj.title = title

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/models.py
]

def __str__(self):
return f"{self.collection.acron.upper()} - {self.collection.acron}"
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

CollectionModel.__str__() assumes self.collection is always set, but the FK is nullable/blank. If a row exists without a selected collection, this will raise AttributeError when rendered in admin. Guard for None (and consider displaying the collection name instead of repeating the acronym twice).

Suggested change
return f"{self.collection.acron.upper()} - {self.collection.acron}"
if self.collection is None:
return "No collection selected"
return str(self.collection)

Copilot uses AI. Check for mistakes.
Comment thread markup_doc/models.py
Comment on lines +35 to +42
class ReadOnlyFileWidget(forms.Widget):
def render(self, name, value, attrs=None, renderer=None):
if value:
# Muestra el archivo como un enlace de descarga
#return format_html('<a href="{}" target="_blank" download>{}</a>', value.url, value.name.split('/')[-1])
instance = value.instance
url = reverse('generate_xml', args=[instance.pk])
return format_html('<a href="{}" target="_blank" download>Download XML</a>', url)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

ReadOnlyFileWidget calls reverse('generate_xml', ...), but there is no URL pattern named generate_xml in the repository (searching the codebase only finds this reference). This will raise NoReverseMatch when the admin form renders. Add the missing URL/view or change this to point at an existing route (or fall back to value.url).

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +33
from django.http import HttpResponseRedirect
from django.utils.translation import gettext_lazy as _
from django.contrib import messages
from django.template.response import TemplateResponse
from wagtail_modeladmin.options import ModelAdmin

from wagtail.snippets.views.snippets import (
CreateView,
EditView,
SnippetViewSet,
SnippetViewSetGroup
)

from markup_doc.models import (
ArticleDocx,
ArticleDocxMarkup,
UploadDocx,
MarkupXML,
CollectionModel,
JournalModel,
ProcessStatus
)

from config.menu import get_menu_order
from markup_doc.tasks import task_sync_journals_from_api
from django.urls import path, reverse
from django.utils.html import format_html
from wagtail.admin import messages
from wagtail.admin.views import generic

from django.shortcuts import redirect, get_object_or_404
from django.views import View

Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

The import section has multiple unused / duplicated imports (e.g., importing messages from both django.contrib and wagtail.admin, plus several unused Wagtail/Django utilities). This will trigger flake8 F401 and the messages name is currently shadowed, which is error-prone. Remove unused imports and keep a single messages import (or alias one of them) to avoid shadowing.

Copilot uses AI. Check for mistakes.
class JournalModelCreateView(CreateView):
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
task_sync_journals_from_api
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

JournalModelCreateView.get_context_data() references task_sync_journals_from_api but never calls it, so no sync will actually run when this view is hit. Call the task (e.g., .delay()) or remove this method if it isn't needed.

Suggested change
task_sync_journals_from_api
task_sync_journals_from_api.delay()

Copilot uses AI. Check for mistakes.
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