diff --git a/TODO.md b/TODO.md index 80531e5..382af72 100644 --- a/TODO.md +++ b/TODO.md @@ -24,10 +24,10 @@ ### `wagtail-localize-smartling` issues -- [ ] Duplicate jobs get created under some circumstances: +- [x] Duplicate jobs get created under some circumstances: - submitting parent pages including subtree when child pages are already translated - translating a page, converting it back to an alias, submitting it for translation again -- [ ] It's possible for multiple Smartling jobs to end up with identical content +- [x] It's possible for multiple Smartling jobs to end up with identical content if translations are submitted/updated more than once in between runs of `sync_smartling`. This seems to be OK for a single target language. Is it OK for multiple target languages? diff --git a/src/wagtail_localize_smartling/templates/wagtail_localize_smartling/admin/resubmit_job.html b/src/wagtail_localize_smartling/templates/wagtail_localize_smartling/admin/resubmit_job.html new file mode 100644 index 0000000..a227d98 --- /dev/null +++ b/src/wagtail_localize_smartling/templates/wagtail_localize_smartling/admin/resubmit_job.html @@ -0,0 +1,16 @@ +{% extends "wagtailadmin/generic/base.html" %} +{% load i18n %} + +{% block main_content %} +
+

+ {{ view.confirmation_message }} + {% if view_url %}{{ view_url }}{% endif %} +

+
+ {% csrf_token %} + + {% trans "No, don't resubmit" %} +
+
+{% endblock %} diff --git a/src/wagtail_localize_smartling/templatetags/wagtail_localize_smartling_admin_tags.py b/src/wagtail_localize_smartling/templatetags/wagtail_localize_smartling_admin_tags.py index 39572ed..62b8e1f 100644 --- a/src/wagtail_localize_smartling/templatetags/wagtail_localize_smartling_admin_tags.py +++ b/src/wagtail_localize_smartling/templatetags/wagtail_localize_smartling_admin_tags.py @@ -1,8 +1,10 @@ from typing import Any from django import template +from django.urls import reverse from django.utils.translation import gettext as _ from wagtail.admin import messages as admin_messages +from wagtail.admin.utils import set_query_params from wagtail_localize_smartling.constants import UNTRANSLATED_STATUSES from wagtail_localize_smartling.models import Job @@ -23,7 +25,7 @@ def smartling_edit_translation_message(context): } translation = context["translation"] - jobs = translation.smartling_jobs.exclude(status__in=UNTRANSLATED_STATUSES) + jobs = translation.smartling_jobs.all() jobs_exists = bool(jobs) inclusion_context["show_message"] = jobs_exists @@ -35,20 +37,38 @@ def smartling_edit_translation_message(context): # then pk descending. So we choose the first in the list. This may mean the job # status will show as unsynced in the message below latest_job = list(jobs)[0] - message = _( - "This translation is managed by Smartling. Changes made here will be lost the " - "next time translations are imported from Smartling. " - f"Job status: {latest_job.get_status_display()}" - ) buttons = [] - if smartling_url := format_smartling_job_url(latest_job): + + if latest_job.status in UNTRANSLATED_STATUSES: + message = _( + "The latest Smartling job for this translation " + f"was {latest_job.get_status_display().lower()}." + ) + resubmit_url = set_query_params( + reverse("wagtail_localize_smartling_retry_job", args=(latest_job.pk,)), + {"next": context["request"].path}, + ) buttons.append( admin_messages.button( - smartling_url, - _("View job in Smartling"), + resubmit_url, + _("Resubmit to Smartling"), new_window=True, ), ) + else: + message = _( + "This translation is managed by Smartling. Changes made here will be lost " + "the next time translations are imported from Smartling. " + f"Job status: {latest_job.get_status_display()}" + ) + if smartling_url := format_smartling_job_url(latest_job): + buttons.append( + admin_messages.button( + smartling_url, + _("View job in Smartling"), + new_window=True, + ), + ) inclusion_context["message"] = admin_messages.render( message, diff --git a/src/wagtail_localize_smartling/views.py b/src/wagtail_localize_smartling/views.py index f1aaa89..5e18568 100644 --- a/src/wagtail_localize_smartling/views.py +++ b/src/wagtail_localize_smartling/views.py @@ -2,13 +2,24 @@ from typing import Any +from django.shortcuts import get_object_or_404, redirect from django.urls import reverse +from django.utils import timezone +from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ from django.views.generic import TemplateView -from wagtail.admin.views.generic import WagtailAdminTemplateMixin +from wagtail.admin.auth import permission_denied +from wagtail.admin.utils import get_valid_next_url_from_request +from wagtail.admin.views.generic import ( + PermissionCheckedMixin, + WagtailAdminTemplateMixin, +) from wagtail.models import Locale +from wagtail.permission_policies import ModelPermissionPolicy -from .models import Project +from .constants import UNTRANSLATED_STATUSES +from .models import Job, Project +from .templatetags.wagtail_localize_smartling_admin_tags import smartling_job_url from .utils import ( format_smartling_project_url, get_wagtail_source_locale, @@ -74,3 +85,82 @@ def get_context_data(self, **kwargs): context["suggested_source_locale_exists"] = False return super().get_context_data(**context) + + +class SmartlingResubmitJobView( # pyright: ignore[reportIncompatibleMethodOverride] + PermissionCheckedMixin, WagtailAdminTemplateMixin, TemplateView +): + _show_breadcrumbs = True + page_title = _("Resubmit to Smartling") + template_name = "wagtail_localize_smartling/admin/resubmit_job.html" + header_icon = "wagtail-localize-language" + object: Job + permission_policy = ModelPermissionPolicy(Job) + permission_required = "view" + jobs_report_url: str = "" + + def get_breadcrumbs_items(self): + # TODO: link to the report view + return super().get_breadcrumbs_items() + [ + {"url": self.jobs_report_url, "label": _("Smartling jobs")}, + {"url": "", "label": self.page_title}, + ] + + def get_object(self): + return get_object_or_404(Job, pk=self.kwargs.get("job_id")) + + def dispatch(self, request, *args, **kwargs): + self.object = self.get_object() + if self.object.status not in UNTRANSLATED_STATUSES: + return permission_denied(request) + + # Check that the given Job is the latest. We can use any of its translations + translation = self.object.translations.first() + if translation is None: + # should cover the case where the translation was converted back to an alias + return permission_denied(request) + + # Jobs are ordered by `first_synced_at`, with null values coming at the top + # then pk descending. + latest_job = translation.smartling_jobs.values_list("pk", flat=True)[0] # pyright: ignore[reportAttributeAccessIssue] + if latest_job != self.object.pk: + return permission_denied(request) + + self.jobs_report_url = reverse("wagtail_localize_smartling_jobs:index") + return super().dispatch(request, *args, **kwargs) + + def get(self, request, *args, **kwargs): + context = self.get_context_data(object=self.object) + return self.render_to_response(context) + + def post(self, request, *args, **kwargs): + if self.object: + due_date = self.object.due_date + Job.get_or_create_from_source_and_translation_data( + self.object.translation_source, + self.object.translations.all(), + user=request.user, + due_date=due_date if due_date and due_date >= timezone.now() else None, + ) + + return redirect(self.get_success_url()) + + def get_success_url(self): + return get_valid_next_url_from_request(self.request) or self.jobs_report_url + + @property + def confirmation_message(self): + return _("Are you sure you want to resubmit this job?") + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context["view"] = self + if smartling_url := smartling_job_url(self.object): + context["view_url"] = format_html( + '{}', + smartling_url, + self.object.reference_number, + _("View job in Smartling"), + ) + + return context diff --git a/src/wagtail_localize_smartling/viewsets.py b/src/wagtail_localize_smartling/viewsets.py index e2ff38a..ad3a92f 100644 --- a/src/wagtail_localize_smartling/viewsets.py +++ b/src/wagtail_localize_smartling/viewsets.py @@ -203,5 +203,9 @@ class JobViewSet(ModelViewSet): def permission_policy(self): return JobPermissionPolicy(self.model) + @cached_property + def url_namespace(self): # # pyright: ignore[reportIncompatibleVariableOverride] + return "wagtail_localize_smartling_jobs" + smartling_job_viewset = JobViewSet("smartling-jobs") diff --git a/src/wagtail_localize_smartling/wagtail_hooks.py b/src/wagtail_localize_smartling/wagtail_hooks.py index acfa510..9b22624 100644 --- a/src/wagtail_localize_smartling/wagtail_hooks.py +++ b/src/wagtail_localize_smartling/wagtail_hooks.py @@ -4,12 +4,20 @@ from wagtail.admin.menu import MenuItem from . import admin_urls +from .views import SmartlingResubmitJobView from .viewsets import smartling_job_viewset @hooks.register("register_admin_urls") # pyright: ignore[reportOptionalCall] def register_admin_urls(): - return [path("smartling/", include(admin_urls))] + return [ + path("smartling/", include(admin_urls)), + path( + "smartling-jobs/resubmit//", + SmartlingResubmitJobView.as_view(), + name="wagtail_localize_smartling_retry_job", + ), + ] @hooks.register("register_settings_menu_item") # pyright: ignore[reportOptionalCall] diff --git a/tests/conftest.py b/tests/conftest.py index 1285862..bc7add0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,7 @@ import pytest +from django.contrib.auth.models import Permission from wagtail.coreutils import get_supported_content_language_variant from wagtail.models import Locale, Page from wagtail_localize.models import LocaleSynchronization @@ -55,6 +56,25 @@ def superuser(): return UserFactory(is_superuser=True) +@pytest.fixture() +def regular_user(): + user = UserFactory(is_superuser=False) + user.user_permissions.add(Permission.objects.get(codename="access_admin")) + return user + + +@pytest.fixture() +def smartling_reporter(): + user = UserFactory(is_superuser=False) + user.user_permissions.add(Permission.objects.get(codename="access_admin")) + user.user_permissions.add( + Permission.objects.get( + content_type__app_label="wagtail_localize_smartling", codename="view_job" + ) + ) + return user + + @pytest.fixture() def smartling_auth(responses): # Mock API response for authentication diff --git a/tests/translation_components/test_submit_translations.py b/tests/translation_components/test_submit_translations.py index 20cdb90..bf7c455 100644 --- a/tests/translation_components/test_submit_translations.py +++ b/tests/translation_components/test_submit_translations.py @@ -104,19 +104,29 @@ def test_submitting_for_translation_with_no_existing_jobs_no_child_pages( assert job.due_date is None +MESSAGE_MANAGED = ( + "This translation is managed by Smartling. Changes made here will be " + "lost the next time translations are imported from Smartling. " + "Job status: STATUS_PLACEHOLDER" +) +MESSAGE_RESUBMIT = ( + "The latest Smartling job for this translation was STATUS_PLACEHOLDER_LOWER" +) + + @pytest.mark.parametrize( - "status,show_message", + "status,message", [ - (JobStatus.UNSYNCED, True), - (JobStatus.AWAITING_AUTHORIZATION, True), - (JobStatus.IN_PROGRESS, True), - (JobStatus.COMPLETED, True), - (JobStatus.CANCELLED, False), - (JobStatus.DELETED, False), + (JobStatus.UNSYNCED, MESSAGE_MANAGED), + (JobStatus.AWAITING_AUTHORIZATION, MESSAGE_MANAGED), + (JobStatus.IN_PROGRESS, MESSAGE_MANAGED), + (JobStatus.COMPLETED, MESSAGE_MANAGED), + (JobStatus.CANCELLED, MESSAGE_RESUBMIT), + (JobStatus.DELETED, MESSAGE_RESUBMIT), ], ) def test_managed_by_message_on_translation( - client, root_page, superuser, smartling_project, status, show_message + client, root_page, superuser, smartling_project, status, message ): client.force_login(superuser) @@ -175,15 +185,11 @@ def test_managed_by_message_on_translation( job.save(update_fields=update_fields) response = client.get(redirect_url) - expected = ( - "This translation is managed by Smartling. Changes made here will be " - "lost the next time translations are imported from Smartling. " - f"Job status: {job.get_status_display()}" # pyright: ignore[reportAttributeAccessIssue] - ) - if show_message: - assert expected in response.content.decode("utf-8") - else: - assert expected not in response.content.decode("utf-8") + status_display = job.get_status_display() # pyright: ignore[reportAttributeAccessIssue] + expected_message = message.replace( + "STATUS_PLACEHOLDER_LOWER", status_display.lower() + ).replace("STATUS_PLACEHOLDER", status_display) + assert expected_message in response.content.decode("utf-8") # TODO test existing jobs in various states diff --git a/tests/views/test_resubmit_job_view.py b/tests/views/test_resubmit_job_view.py new file mode 100644 index 0000000..538ec21 --- /dev/null +++ b/tests/views/test_resubmit_job_view.py @@ -0,0 +1,150 @@ +import pytest + +from django.urls import reverse +from django.utils import timezone +from pytest_django.asserts import assertRedirects +from wagtail.models import Locale +from wagtail_localize.models import Translation, TranslationSource + +from wagtail_localize_smartling.api.types import JobStatus +from wagtail_localize_smartling.models import Job +from wagtail_localize_smartling.templatetags.wagtail_localize_smartling_admin_tags import ( # noqa: E501 + smartling_job_url, +) +from wagtail_localize_smartling.utils import compute_content_hash + +from testapp.factories import InfoPageFactory + + +pytestmark = pytest.mark.django_db + + +WAGTAIL_ADMIN_HOME = reverse("wagtailadmin_home") + + +@pytest.fixture +def smartling_job(smartling_project, superuser, root_page): + page = InfoPageFactory(parent=root_page, title="Component test page") + translation_source, created = TranslationSource.get_or_create_from_instance(page) + page_translation = Translation.objects.create( + source=translation_source, + target_locale=Locale.objects.get(language_code="fr"), + ) + + now = timezone.now() + job = Job.objects.create( + project=smartling_project, + translation_source=translation_source, + user=superuser, + name=Job.get_default_name(translation_source, [page_translation]), + description=Job.get_default_description(translation_source, [page_translation]), + reference_number=Job.get_default_reference_number( + translation_source, [page_translation] + ), + content_hash=compute_content_hash(translation_source.export_po()), + first_synced_at=now, + last_synced_at=now, + status=JobStatus.DRAFT, + translation_job_uid="job_to_be_cancelled", + ) + job.translations.set([page_translation]) + + return job + + +@pytest.fixture +def cancelled_smartling_job(smartling_job): + smartling_job.status = JobStatus.CANCELLED + smartling_job.save(update_fields=["status"]) + return smartling_job + + +def test_get_resubmit_job_view_denied_if_job_not_in_untranslated_status( + client, superuser, smartling_job +): + client.force_login(superuser) + url = reverse("wagtail_localize_smartling_retry_job", args=[smartling_job.id]) + response = client.get(url) + assertRedirects(response, WAGTAIL_ADMIN_HOME) + + +def test_get_resubmit_job_denied_without_appropriate_user_permission( + client, regular_user, cancelled_smartling_job +): + client.force_login(regular_user) + url = reverse( + "wagtail_localize_smartling_retry_job", args=[cancelled_smartling_job.id] + ) + response = client.get(url) + assertRedirects(response, WAGTAIL_ADMIN_HOME) + + +def test_get_resubmit_job_denied_if_newer_job_exists( + client, regular_user, cancelled_smartling_job +): + new_job = cancelled_smartling_job + new_job.id = None + new_job.status = JobStatus.DRAFT + new_job.save() + + client.force_login(regular_user) + url = reverse( + "wagtail_localize_smartling_retry_job", args=[cancelled_smartling_job.id] + ) + response = client.get(url) + assertRedirects(response, WAGTAIL_ADMIN_HOME) + + +def test_get_resubmit_job_with_allowed_user_permission_succeeds( + client, smartling_reporter, cancelled_smartling_job +): + client.force_login(smartling_reporter) + url = reverse( + "wagtail_localize_smartling_retry_job", args=[cancelled_smartling_job.id] + ) + response = client.get(url) + assert response.status_code == 200 + + assert "Are you sure you want to resubmit this job" in response.content.decode() + + +def test_get_resubmit_job_view(client, superuser, cancelled_smartling_job): + client.force_login(superuser) + url = reverse( + "wagtail_localize_smartling_retry_job", args=[cancelled_smartling_job.id] + ) + response = client.get(url) + assert response.status_code == 200 + + content = response.content.decode() + assert "Are you sure you want to resubmit this job" in content + assert smartling_job_url(cancelled_smartling_job) in content + + +def test_post_resubmit_job_view_creates_a_new_job( + client, superuser, cancelled_smartling_job +): + assert Job.objects.count() == 1 + + client.force_login(superuser) + url = reverse( + "wagtail_localize_smartling_retry_job", args=[cancelled_smartling_job.id] + ) + response = client.post(url) + assert response.status_code == 302 + + cancelled_smartling_job.refresh_from_db() + assert cancelled_smartling_job.status == JobStatus.CANCELLED.name + + assert Job.objects.count() == 2 + assert Job.objects.order_by("pk").last().status == JobStatus.UNSYNCED.name # pyright: ignore[reportOptionalMemberAccess] + + +def test_resubmit_non_existent_job_404s(client, superuser): + client.force_login(superuser) + url = reverse("wagtail_localize_smartling_retry_job", args=[999999]) + response = client.get(url) + assert response.status_code == 302 + # bit of a hack, but in this case, we raise a 404 + # but the i18n setup tries with the default language prefixed + assert response.url == f"/en{url}" diff --git a/tests/status/test_status_view.py b/tests/views/test_status_view.py similarity index 100% rename from tests/status/test_status_view.py rename to tests/views/test_status_view.py