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

Fix uncaught exception "translation" from Jinja #1126

Conversation

ahmad-alkadri
Copy link
Contributor

This (draft) PR potentially can fix Issues #1122 and (maybe) #1100.

Location of the bug found at the app/routes.py and app/templates/error.html.

Basically instead of querying the key from the translation dictionary at the template-level we should do it at the python func-level.

Please click on the links to see the details of the commits.

Also modified one of the testing script.

Open to feedback.

@ahmad-alkadri
Copy link
Contributor Author

Also just realized that the test code failed at this part where:

    def test_ddg_bang(client):
        # Bang at beginning of query
        rv = client.get(f'/{Endpoint.search}?q=!gh%20whoogle')
>       assert rv._status_code == 302
E       assert 200 == 302
E        +  where 200 = <WrapperTestResponse streamed [200 OK]>._status_code

So basically the endpoint responded with 200 (status ok successful) instead of 302. Maybe we should modify the test script to include more scenarios. What do you think @benbusby?

@benbusby
Copy link
Owner

benbusby commented Mar 6, 2024

@ahmad-alkadri the test error you saw should be fixed now -- it was due to DuckDuckGo changing their bang URL, so it needed to be updated to not use the version number anymore.

As far as the translation errors, it would probably be simpler to do something like the following:

- <input type="submit" id="search-submit" value="{{ translation['search'] }}">
+ <input type="submit" id="search-submit" value="{{ translation.get('search', 'Search') }}">

Calling the function directly in the html template means we could still just pass in the translation dict as before, but switch to using the .get(key, default) function in the template. With that approach, there would be little to no changes in the python code.

Also worth noting that the default value should always be a non-empty string, maybe just the default English translation of whichever element?

@benbusby benbusby linked an issue Mar 6, 2024 that may be closed by this pull request
8 tasks
@benbusby
Copy link
Owner

benbusby commented Mar 6, 2024

@ahmad-alkadri Actually never mind, I looked into this a bit further and it looks like the primary issue is that the error html template renders farside redirects when they aren't actually needed. I'm going to push a separate fix to make the redirect section only render if a query is present, otherwise it should just display the error and allow the user to return back to the home page.

@benbusby
Copy link
Owner

benbusby commented Mar 6, 2024

Just pushed the change to app/templates/error.html, which resolves the original issue. Feel free to reopen this PR if you see anything that you think should still be refactored with the new approach!

@benbusby benbusby closed this Mar 6, 2024
@ahmad-alkadri
Copy link
Contributor Author

Hi @benbusby

the primary issue is that the error html template renders farside redirects

that is correct

when they aren't actually needed. I'm going to push a separate fix to make the redirect section only render if a query is present, otherwise it should just display the error and allow the user to return back to the home page.

no problem with me. I've just pulled the main branch with your fixes and I saw that my issue's gone also 😄 Thanks!

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.

[BUG] Translation uncaught exception
2 participants