WeblateOrg / weblate

Web based localization tool with tight version control integration.
https://weblate.org/
GNU General Public License v3.0
4.52k stars 998 forks source link

Resetting translations in a component to upstream gives Page Not Found(404) error (Without push enabled) #4105

Open 55abhilash opened 4 years ago

55abhilash commented 4 years ago

Describe the bug

To reset all changes in a language in a component, I go to repository Manage -> Repository Maintainence I press "Reset" button. It leads me to a url of the following format: localhost:8000/projects/project_name/component_name/language_code/#repository which is a URL no longer bound to any view, if all the translations of a particular language are reset.

Although if I go back to the project, it does show me the message that "All changes have been reset", so actually Reset is working fine, just that if after resetting, there is nothing left, it should redirect to the project url, instead of language url.

To Reproduce

Steps to reproduce the behavior:

  1. Go to Project -> Component
  2. Start New Translation. Choose language and continue.
  3. Go to the translations and translate any one string.
  4. Go to language -> Manage -> Repository Maintenance
  5. Reset

Expected behavior

If after resetting, all the translations of a language are gone (hence the language itself is removed from the component), it should redirect to project URL.

Server configuration and status

55abhilash commented 4 years ago

The code which leads up to the redirection is very generic. The following function is called for multiple purposes; pull, push, reset,etc. (in argument 'call') for a language, component or project (in argument 'obj')

def execute_locked(request, obj, message, call, *args, **kwargs):
    """Helper function to catch possible lock exception."""
    try:
        result = call(*args, **kwargs) 
        # With False the call is supposed to show errors on its own
        if result is None or result:
            messages.success(request, message)
    except Timeout:
        messages.error(
            request,
            _("Failed to lock the repository, another operation is in progress."),
        )
        report_error()
    return redirect_param(obj, "#repository")

We want to redirect to project when language has no strings left after reset. But there is no way to do it, except doing some kind of hack like:

    #If no translations of a language are left after reset, redirect to component url
    if call.__name__ == "do_reset" \
        and type(obj) == type(Translation()) \
        and len(Translation.objects.filter(component=obj.component, language=obj.language)) == 0: #There is no Translation object for this component and language combination
        obj = obj.component
   redirect_param(obj, "#repository")

In this case, since obj = obj.component, it will redirect to URL of component. Is there any other way to do it within the constraints put by the generic function?

nijel commented 4 years ago

The very same situation can happen with update as well - when the translation has been removed in the upstream repo, so I don't think there is need to special case for reset only.

The similar thing can happen on component level as well - the files are removed and the discovery can remove no longer existing component (in normal setup this would happen in background, but still the component could be removed before the redirect is loaded).

I can see few approaches to this:

I favor the second approach as it provides graceful handling of such URLs for all users, not only for the one triggering this, but it is way more complex code change.

Related middleware which handles redirection for renamed or mistyped translations:

https://github.com/WeblateOrg/weblate/blob/a98e1f161e31c4522ef2bbdee890ad55ec8a3024/weblate/middleware.py#L68-L155

55abhilash commented 4 years ago

Properly log all such removals in history

I can see ACTION_REMOVE_TRANSLATION and ACTION_REMOVE_COMPONENT in Change model. That means it is already logging on removal of translation or component?

You can assign this to me btw.

nijel commented 4 years ago

Yes, it's there, but right now it is AFAIK generated only for manually triggered removal now. Also the middleware doesn't look for these, it looks only for renaming.

55abhilash commented 4 years ago

Do you think it is better to add seperate method to handle in case of a 404 error? Or should the existing methods (fixup_component, fixup_language) be enhanced?

nijel commented 4 years ago

Probably separate, because those only look into existing objects, they won't handle redirection to parent object.

55abhilash commented 4 years ago

Ok. So now I found out that it is 'Http404' exception in case of both renaming and deletion. Now how to differentiate whether the exception is due to renaming or it is due to deletion? We can use the use the location where exception occurs maybe. Eg. in case of a translation getting deleted, the exception occurs at views.basic.show_translation. So we redirect to parent if exception comes from here. What do you think?

nijel commented 4 years ago

The error handler currently doesn't care for a cause, it just looks if there is a renaming entry in the history. The similar could be done for the removal - look for removal of the matching object and redirect to parent.