wagtail / wagtail-localize

Translation plugin for Wagtail CMS
https://wagtail-localize.org/
Other
216 stars 83 forks source link

Issue reading PO files on windows. #757

Closed Nigel2392 closed 4 months ago

Nigel2392 commented 6 months ago

PO files cannot properly be opened on windows. This is because the file gets locked.

You might receive permission errors like so:

PermissionError at /admin/localize/translate/59/pofile/upload/
[Errno 13] Permission denied: 'C:\\Users\\<NOT_THE_RIGHT_USERPROFILE_FOLDER????>\\AppData\\Local\\Temp\\tmp6o550iu9'

Where NOT_THE_RIGHT_USERPROFILE_FOLDER would be NIGELV~1 when it should be NigelvanGoodAd.

This is due to an issue in opening the tempfile in view/edit_translation.upload_pofile. The temporary file should be opened with delete=False, and then properly unlinked.

The fixed code should be:


@require_POST
def upload_pofile(request, translation_id):
    translation = get_object_or_404(Translation, id=translation_id)

    instance = translation.get_target_instance()
    if not user_can_edit_instance(request.user, instance):
        raise PermissionDenied

    do_import = True

    # Set delete to false. This fixes some windows errors when creating tempfiles.
    # This is due to the Windows OS locking temporary files when open.
    with tempfile.NamedTemporaryFile(delete=False) as f:
        # Note: polib.pofile accepts either a filename or contents. We cannot pass the
        # contents directly into polib.pofile or users could upload a file containing
        # a filename and this will be read by polib!
        f.write(request.FILES["file"].read())
        f.flush()

    # Move indentation back, we should open the file outside of the with statement.
    # Delete must be set to false, otherwise the file will be deleted before we can open it.
    try:
        po = polib.pofile(f.name)

    except (OSError, UnicodeDecodeError):
       # Annoyingly, POLib uses OSError for parser exceptions...
       messages.error(request, _("Please upload a valid PO file."))
       do_import = False

    if do_import:
        translation_id = po.metadata["X-WagtailLocalize-TranslationID"]
        if translation_id != str(translation.uuid):
            messages.error(
                request,
                _(
                    "Cannot import PO file that was created for a different translation."
                ),
            )
            do_import = False

    if do_import:
        translation.import_po(po, user=request.user, tool_name="PO File")
        messages.success(request, _("Successfully imported translations from PO File."))

    # Delete the created tempfile
    try:
        os.unlink(f.name)
    except OSError:
        pass

    # Work out where to redirect to
    next_url = get_valid_next_url_from_request(request)
    if not next_url:
        # Note: You should always provide a next URL when using this view!
        next_url = reverse("wagtailadmin_home")

    return redirect(next_url)
zerolab commented 6 months ago

@Nigel2392 this is like a side effect of none of the localize developers using Windows. Happy to review a PR if you're up for submitting one

Nigel2392 commented 6 months ago

@Nigel2392 this is like a side effect of none of the localize developers using Windows. Happy to review a PR if you're up for submitting one

I don't really understand github and their pull request system, sorry... I've monkeypatched it in my own project, so it won't bother me when developing. Respectfully - I'm only here to bring it to attention. I'm planning to put some time into git's pull system in the future; I'll get back to it then.

zerolab commented 6 months ago

Understood. And thank you again for raising these issues. It is most helpful to get reports from as many real-world users as possible.