-
Notifications
You must be signed in to change notification settings - Fork 530
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
Optimize tags dashboards #3101
Optimize tags dashboards #3101
Conversation
bc3f4f4
to
884c6c0
Compare
884c6c0
to
6f701c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one minor stylistic quibble inline about the code, but I have two larger concerns:
- The tests for the old implementation are removed, but I don't see any corresponding tests being added for the new
class Tags
. Is that really covered by the remaining prior tests? - The file paths and naming is messed up.
TagsTool
is now only used for the admin form, and it's intags/utils/tags.py
.Tags
is the presumably more common non-admin part, and it's intags/utils/utils.py
. Could/should the former class and file be renamed asTagsAdmin
or something similar, andTags
defined intags.py
?
pontoon/tags/utils/utils.py
Outdated
if tr["approved_date"] is not None and tr["approved_date"] > tr["date"]: | ||
date = "approved_date" | ||
else: | ||
date = "date" | ||
|
||
dates[tr[date]] = tr[group_by] | ||
prefix = "entity__" if group_by == "resource__tag" else "" | ||
|
||
# Find translations with matching date and tag/locale | ||
translations |= Translation.objects.filter( | ||
Q(**{date: tr[date], f"{prefix}{group_by}": tr[group_by]}) | ||
).prefetch_related("user", "approved_user") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if tr["approved_date"] is not None and tr["approved_date"] > tr["date"]: | |
date = "approved_date" | |
else: | |
date = "date" | |
dates[tr[date]] = tr[group_by] | |
prefix = "entity__" if group_by == "resource__tag" else "" | |
# Find translations with matching date and tag/locale | |
translations |= Translation.objects.filter( | |
Q(**{date: tr[date], f"{prefix}{group_by}": tr[group_by]}) | |
).prefetch_related("user", "approved_user") | |
date = max(tr["date"], tr["approved_date"] or 0) | |
dates[date] = tr[group_by] | |
prefix = "entity__" if group_by == "resource__tag" else "" | |
# Find translations with matching date and tag/locale | |
translations |= Translation.objects.filter( | |
Q(**{date: date, f"{prefix}{group_by}": tr[group_by]}) | |
).prefetch_related("user", "approved_user") |
Codecov says the coverage of the utils.py is above 80% (although I can't currenty access the results page). Still, I should add a test or two for that.
OK, I'll move things around a little. Maybe rename I didn't want to touch the old code before wiping it out the way I did with the rest of the code. I should file a bug for that. I could also add a README file to /pontoon/tags/utils explaining that everything but the utils.py is legacy code that should not be touched. |
…from tags/utils to tags/admin
Applied the suggested change, move Admin code to /admin and filed #3108. |
pontoon/tags/admin/resources.py
Outdated
from pontoon.tags.admin.exceptions import InvalidProjectError | ||
|
||
from .base import TagsDataTool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from pontoon.tags.admin.exceptions import InvalidProjectError | |
from .base import TagsDataTool | |
from .base import TagsDataTool | |
from .exceptions import InvalidProjectError |
The functionality should remain identical, but the views should be significantly faster. In the case of Firefox Tag dashboards, the load times were in the 20-30 second range (or timing out), now these pages take max. 2-3 seconds to load. The patch also: * adds an extra DB index, which significantly impacts Tags tab performance * drops the unused code (which represents the vast majority of changes in this patch) * moves remaining files from tags/utils to tags/admin
This reverts commit 1dcd738.
Fix #2264.
I rewrote the code for retrieving data used on all three Tags dashboards:
I also:
The functionality should remain identical, but the views should be significantly faster. In the case of Firefox Tag dashboards, the load times were in the 20-30 second range (or timing out), now these pages take max. 2-3 seconds to load.
Deployed to stage.