Fix Bug 1578057: update locale's latest_translation for visible projects only #1366
Conversation
|
Hi @mathjazz, can you suggest me a suitable reviewer, please? |
pontoon/base/models.py
Outdated
| instances = [project, locale, translatedresource] | ||
| instances = [translatedresource, project] | ||
|
|
||
| if Project.objects.visible().filter(pk=project.pk).first(): |
There was a problem hiding this comment.
Is there a better way to do this in Django? I am not that familiar with the DB layer, but it seems to me there are three SELECTs to the database first (TranslatedResource.objects., Project.objects. and utils.get_object_or_none) to decide whether to update the three tables or not.
Maybe it could be rewritten to something that issues just three UPDATE statements instead, with the proper WHERE clause to ensure the right rows are (not) updated.
There was a problem hiding this comment.
It's enough to check if the project isn't a system project here like this:
if not project.system_project:
That's also what we do in adjust_all_stats() to make sure we don't update stats on /teams when system projects change.
The Project.visible() method would also check if the project is available (i.e. has been synced for the first time), but we don't need that since in this case the Translation.save() method (which calls update_latest_translation()) would not be callable.
There was a problem hiding this comment.
Nice, that's one SELECT down.
Do you think I should also get rid of the get on TranslatedResource and ProjectLocale? There wouldn't be the for-loop, just X.objects.filter(...).update(...).
There was a problem hiding this comment.
You should be able to save another DB request by getting rid of TranslatedResource.objects.get() if you pass translatedresource to self.update_latest_translation() in save().
I'm not sure about the ProjectLocale.
You can use this patch to debug SQL performance and compare the number of DB calls:
https://gist.github.com/mathjazz/92d2f6a36c968e8c1f6496f5e5aa0318
There was a problem hiding this comment.
I have tried to do it via QuerySet update() API. update_latest_translation is sometimes called from a context, where the translated resource is not present, or at least not directly.
Thanks for filing a bug and creating a PR! I'll be happy to review! |
pontoon/base/models.py
Outdated
| instances = [project, locale, translatedresource] | ||
| instances = [translatedresource, project] | ||
|
|
||
| if Project.objects.visible().filter(pk=project.pk).first(): |
There was a problem hiding this comment.
It's enough to check if the project isn't a system project here like this:
if not project.system_project:
That's also what we do in adjust_all_stats() to make sure we don't update stats on /teams when system projects change.
The Project.visible() method would also check if the project is available (i.e. has been synced for the first time), but we don't need that since in this case the Translation.save() method (which calls update_latest_translation()) would not be callable.
|
Thanks for the review. Please also check the last two commits. If you like them and CI is green, I intend no more changes. EDIT: Interesting, the same tests failed as without the last commit, but I cannot reproduced it locally. Even the build on the branch in my fork passed. |
As of now, translation changes to invisible projects (e.g. system project like tutorial) do update the latest activity column shown on all teams dashboard. That's kind of confusing, since you cannot click through to that system project from any dashboards. As the name suggests, the project is invisible there.
This change omits updating
latest_translationfor a locale if the translation is being saved for a system project. Note, thatlatest_translationis still updated in all other tables, includingproject_locale, because the dashboard of an invisible project is still accessible via direct link or from its translation view and the latest activity should be displayed there.I have written a small unit test for the behaviour. Also I have tested the patch manually in local setup. Exact steps to reproduce can be found in the bug, though in the local setup you need to add at least one visible project, so the teams dashboard can show something interesting.