Record counts for review queues, including detail per promoted class

[diox]

Fixes mozilla/addons#15173

Testing

manage.py cron record_reviewer_queues_counts should populate the database with QueueCount instances for the current date. There should be one instance per day per queue, plus one per promoted class. Re-running the cron for a given day should have no effect.

[KevinMind]

Few comments.

[diox]

This is the only change in that file, rest is re-ordering alphabetically

[KevinMind]

nit: is there a reason not to simply define the 3 relevant properties as args and let the view pass in the arguments from else and the cron pass in the hard coded values. IMO that makes it super clear at the call site what is going on, whereas now both are kind of obfuscated.. If there is a good reason to keep the request in context then fine, but otherwise that seems like the better way to go, don't you think?

[diox]

I had a version with this idea that I didn't push: I had renamed get_queryset() as get_base_queryset(), the views would call get_queryset(request) which would then call get_base_queryset() with the right arguments, the cron would directly call get_base_queryset() (with no arguments, the default values were what we needed).

But ultimately I scrapped it because it was very verbose (each table needed to have those 2 methods, even when they didn't implement anything special) and I found the extra layer more confusing when re-reading the code.

[KevinMind]

I'm not sure why it needs to be 2 methods. it could just be one method with the defined arguments.

get_queryset(
    admin_reviewer=False,
    show_temporarily_delayed = True,
    show_only_upcoming = False,
)

Then in the view we can pull the arguments from the request.

admin_reviewer = is_admin_reviewer(request.user)
show_temporarily_delayed = acl.action_allowed_for(
    request.user, amo.permissions.ADDONS_TRIAGE_DELAYED
)
show_only_upcoming = upcoming_due_date_focus and not acl.action_allowed_for(
    request.user, amo.permissions.ADDONS_ALL_DUE_DATES
)

table = TheClass.get_queryset(
    admin_reviewer=admin_reviewer,
    show_temporarily_delayed=show_temporarily_delayed,
    show_only_upcoming=show_only_upcoming,
)

... do stuff with table

And in the cron we just directly set whatever arguments we want... would something like this not work and achieve my goal of keeping the control logic at the call site? You can say yes and also not implement it, but wondering if this would work in your view?

[diox]

Something like that would work, yes, but I was trying to avoid more duplication of code. The code to check for show_temporarily_delayed / admin_reviewer is needed in the queue view... but also the dashboard, which fetches the counts (personalized for the current user according to what they'd be able to see). The single get_queryset() method avoids that duplication currently.

[KevinMind]

Okay. I won't die on this hill but I think that kind of code duplication is totally okay and arguably better than including additional (unnecessary) context into the "service" layer. Keeping that layer args only not only simplifies testing (just pass all combination of arguments and verify the results, no need to mock requests) it also exposes the actual expectations each caller has for the service, since the code is defined and readable at the point of dependency. This is totally worth sacrificing one unit of DRY imo.

I will verify and approve. Speaking of... how should I verify the counts themselves are correct. Are we expecting the length of the queue in reviewer tools should match that of the QueueCount instance? How would one verify this using the admin?

[diox]

I will verify and approve. Speaking of... how should I verify the counts themselves are correct. Are we expecting the length of the queue in reviewer tools should match that of the QueueCount instance? How would one verify this using the admin?

At the most basic level, you need a bunch of add-ons in the queues, which can be achieved by:

• Submitting themes / theme versions, for the theme queue
• Submitting extensions / extensions versions and forcing their versions not to be auto-approved and marked as needing human review, for the manual review queue (quickest way is probably to add NeedsHumanReview instance for each version - reason doesn't matter, as long as is_active stays True - and don't auto-approve them)

Then if you are a superuser (so you have all permissions) the count in the dashboard for each queue should match the value in the corresponding QueueCount for that day, as well as the number of visible items in the queue page itself (including pagination if needed)

(You can't verify this in the admin, have to open a django shell)

There are a lot more combinations since depending on permissions you should not see the same contents in some queues (but the QueueCount should stay the same) and more queues than that, but I think this would be a lot easier to fully verify on dev (which has all the relevant data) if the logic looks ok. Existing unit tests should also ensure I've not regressed the queues in my refactor.

[KevinMind]

Agree, this requires a lot of preparation data already covered in the tests so let's verify on dev.