Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
55abhilash opened this issue Jun 29, 2020 · 8 comments
Labels
bug Something is broken.

Comments

@55abhilash
Copy link
Contributor

55abhilash commented Jun 29, 2020

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

  • Weblate: weblate-4.1.1-247-gafcd25bb6f
  • Django: 3.0.7
@55abhilash
Copy link
Contributor Author

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
Copy link
Member

nijel commented Jun 29, 2020

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:

  • Call obj.refresh_from_db() and redirect to parent in case that fails. This would cover the case you've hit, but not the component removal as that happens later and in different database transaction.
  • Properly log all such removals in history and improve redirect handling in middleware to catch removed translations or components as well. Right now it handles only renamed projects/components.

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:

class RedirectMiddleware:
"""
Middleware that handles URL redirecting.
This used for fuzzy lookups of projects, for example case insensitive
or after renaming.
"""
def __init__(self, get_response=None):
self.get_response = get_response
def __call__(self, request):
return self.get_response(request)
def fixup_language(self, lang):
return Language.objects.fuzzy_get(code=lang, strict=True)
def fixup_project(self, slug, request):
try:
project = Project.objects.get(slug__iexact=slug)
except Project.DoesNotExist:
try:
project = (
Change.objects.filter(
action=Change.ACTION_RENAME_PROJECT, target=slug,
)
.order()[0]
.project
)
except IndexError:
return None
request.user.check_access(project)
return project
def fixup_component(self, slug, request, project):
try:
component = Component.objects.get(project=project, slug__iexact=slug)
except Component.DoesNotExist:
try:
component = (
Change.objects.filter(
action=Change.ACTION_RENAME_COMPONENT, target=slug
)
.order()[0]
.component
)
except IndexError:
return None
request.user.check_access_component(component)
return component
def process_exception(self, request, exception):
if not isinstance(exception, Http404):
return None
try:
resolver_match = request.resolver_match
except AttributeError:
return None
resolver_match = request.resolver_match
kwargs = dict(resolver_match.kwargs)
if "lang" in kwargs:
language = self.fixup_language(kwargs["lang"])
if language is None:
return None
kwargs["lang"] = language.code
if "project" in kwargs:
project = self.fixup_project(kwargs["project"], request)
if project is None:
return None
kwargs["project"] = project.slug
if "component" in kwargs:
component = self.fixup_component(kwargs["component"], request, project)
if component is None:
return None
kwargs["component"] = component.slug
if kwargs != resolver_match.kwargs:
return redirect(resolver_match.url_name, **kwargs, permanent=True)
return None

@55abhilash
Copy link
Contributor Author

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
Copy link
Member

nijel commented Jun 30, 2020

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
Copy link
Contributor Author

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
Copy link
Member

nijel commented Jul 12, 2020

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

@55abhilash
Copy link
Contributor Author

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
Copy link
Member

nijel commented Jul 18, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken.
Projects
None yet
Development

No branches or pull requests

2 participants