Optimize number of queries made by devhub version list view & display block status #22789
Conversation
diox
force-pushed
the
devhub-expose-blocks-in-version-list
branch
from
63cbfc0
to
2264666
Compare
| @@ -87,7 +97,7 @@ | |||
| data-is-current="{{ (version == addon.current_version)|int }}">x</a> | |||
| </td> | |||
| </tr> | |||
| {% if full_info and version.channel == amo.CHANNEL_LISTED and (can_request_review or can_cancel) and check_addon_ownership(request.user, addon, allow_developer=True) %} | |||
There was a problem hiding this comment.
The view is already decorated by dev_required which calls check_addon_ownership(request.user, addon, allow_developer=True, allow_mozilla_disabled_addon). can_cancel and can_request_review will never be True if the add-on is disabled, so this extra check is not needed.
| @@ -148,9 +158,8 @@ <h2>{{ addon.name }}</h2> | |||
| <h3>{{ distro_tag(amo.CHANNEL_LISTED)}} {{ _('Listing visibility') }}</h3> | |||
| <div class="item" id="addon-current-state"> | |||
| <div class="item_wrapper"> | |||
| {% set owner = check_addon_ownership(request.user, addon, allow_developer=True) %} | |||
There was a problem hiding this comment.
As above, since the view is already decorated with the appropriate check all we need to do is check for addon.status == amo.STATUS_DISABLED instead.
| @@ -212,7 +221,7 @@ <h3>{{ _('All versions') }}</h3> | |||
| <div class="item" id="version-list" | |||
| data-stats="{{ url('devhub.versions.stats', addon.slug) }}"> | |||
| <div class="item_wrapper"> | |||
| {% if check_addon_ownership(request.user, addon, allow_developer=True) and addon.status != amo.STATUS_DISABLED %}{# i.e. Admin disabled #} | |||
| {% if can_submit %} | |||
There was a problem hiding this comment.
As above, since the view is already decorated with the appropriate check and can_submit already means the add-on is not disabled, we can simplify this.
| BLOCK_TYPE_END_USERS_CHOICES = Choices( | ||
| ('BLOCKED', 0, _('Blocked')), | ||
| ('SOFT_BLOCKED', 1, _('Restricted')), | ||
| ) |
There was a problem hiding this comment.
😐
I'm not a fan of the way we effectively have 5 different ways of indicating soft-blocked...
SOFT_BLOCKED (the constant name);
1 (the constant integer value);
True (BlockVersion.soft value);
'Restricted' the user facing name;
'Soft-Blocked' the internal name.
I'm not saying this needs to be addressed in this pr but it's not ideal.
There was a problem hiding this comment.
I agree. As much as I don't like switching the field to an integer, we could do it, that would solve the most confusing part. But I don't see a way to solve the strings, since UX want us to say "Restricted" to end-users, but internally "Restricted" is such a loaded term...
There was a problem hiding this comment.
Re: the label we expose to internal vs end users, we could use the constant name (i.e. SOFT_BLOCKED) but I'm not sure that's a great solution either 🤷
There was a problem hiding this comment.
I could also not make the end-user version a Choice object - we don't need to manipulate it other than for display...
| versions, which are fetched without a select_related('blockversion') #} | ||
| {{ version.blockversion.get_user_facing_block_type_display() }} | ||
| {% else %} | ||
| {{ version.file.get_status_display() }} |
There was a problem hiding this comment.
I see we did this already, but I wonder why we weren't using version.get_review_status_display here?
There was a problem hiding this comment.
Good question. I think get_review_status_display was meant for reviewers only. It can return Disabled by Developer which is technically more correct that what we do now (atm in devhub if the developer disables the version, we say "Disabled by Mozilla") but at the same time a bit weird to display to a developer maybe ?
There was a problem hiding this comment.
We have a thread with UX about the various statuses and what we show to developers, lets wait to see how that shakes out as it will probably require more changes/its own issue.
Fixes mozilla/addons#15037
Fixes mozilla/addons#15017
Description
This replaces "Disabled by Mozilla" by "Blocked"/"Restricted" in devhub versions list, and optimizes queries made by that view as a drive-by (I was already in the neighborhood and was changing the test anyway...)
Testing
Unit test, or play with blocks on versions that you have locally. Note that technically one can force a block without having the version disabled, but I figured this was an edge case not worth worrying about, that saves us a couple database queries that way because we are re-using the macro for various versions that are fetched without fetching the blockversion as well.