Enable sorting of organisation codelists

[tomodwyer]

Part of #3058 - this PR adds the functionality to sort codelists and stores the state in query params.

Published codelist tests cover:

• sort=name&direction=asc
• sort=name&direction=desc
• sort=updated_at&direction=asc
• sort=updated_at&direction=desc

Under-review tests cover:

• sort=name&direction=asc
• sort=name&direction=desc
• sort=created_at&direction=asc
• sort=created_at&direction=desc

Screenshots

Sorted by name (asc) [default]

Sorted by name (desc)

Sorted by last updated (asc)

Sorted by last updated (desc)

Sorted by name (desc) with search

[mikerkelly]

Thanks, Thomas. This seems to mostly do the right thing on local testing and the UI looks good. I like the URL parameter design which is clear and functional.

I think that the overall idea of separating out the organisation list views from the index but with some shared code is a very good one, same for the partial templates. Much of the functionality is along the right lines, but I think we can make some simplifications which will make this much easier to develop and maintain. I can also see that some of the issues I am having with this come from the index code that this was split off from and are longstanding.

I have been finding this module hard to understand and review and so I am
going to make some fairly extensive review comments. Given all that, I am
very happy to assist however I can, whether that is futher review, discussion,
pairing, or doing some or all of the work I am asking for. Let me know what you
think, and feel free to push back. I definitely think we can leave many of these
thoughts for the future if at all. I still have not looked in detail at the
templates or testing.

Probably I have left too many comments, but did not want to lose any thoughts. Sorry if there is too much text and it is unstructured and repetitive.

Some general comments below, more inline.

Correctness of template use of pagination

The Pagination object seems to have been set up right but the context has and template tries to use information about First and Last buttons and page numbers. I never see these, even on an organisation with many pages so I think something may be wrong in the logic in one of those but have not had time to look into that. Could you investigate, or let me know where I can see it working if you have.

I tried going to http://localhost:7000/codelist/opensafely/ which has 12 pages
that I went through and never see the links.

Code reuse and organisation

It seems like the index and organisation codelist pages do most of the sorting logic in Python whereas #3029 was written in JavaScript. Is there a reason for the difference? We now have two separate implementations of sorting a list of codelists for different pages. I think I like the Python implementation for this but could we share code for listing, sorting, and filtering codelists in future PR(s)?

Within views/organisations.py, the module is structured as two separate view functions sharing a lot of private utility functions to handle the differences between the views for published and under-review codelists. This does the job, but it introduces a lot of indirection that I am finding makes it quite hard to follow and reason about the differences.

I think for this PR we should worry less about good query performance and DRY code and more about making the QuerySet the view makes easy to understand and maintain. We can do some refactoring to add custom QuerySets and share code if we keep developing this.

I would like us to try and reduce the number of private helpers down so that there is less indirection and the top-level flow in the view is clearer. Each view is extracting and validating some URL params, making a QuerySet, and constructing a context including a Paginator. Probably those should be the top level helpers, if any. Inline review comments expand on this theme.

All of the QuerySet filtering and sorting for Codelist, CodelistVersion, and Handle get quite complicated here and in index. Probably this should not be in the views at all but somewhere in the models as a model queryset or manager, or in a separate queries module that the view can then call out to with the necessary parameters. I don't necessarily think that we should do anything about that in this PR.

I think it will be much easier for this PR to reason about the QuerySet if we construct one QuerySet with the generally in-scope handles (right org, current, not private, published version exists) then compose that with search_handles. And the part that does the composition should ideally all be done in one place so it can be understood, then detail looked at if required, not split across the organisation_review, _get_organisation_handles, _handles_with_status, and _under_review_versions functions. I think I am finding it too hard to understand and reason about the way it is currently organised.

I have a comment near the start of organisation_published show what that could look like.

For the future, if we want to be more DRY and have shared queryset functions like "saved" filters and order_bys, I think an idiomatic way to do this is to extend the QuerySet class with some custom methods that also return QuerySets, and use that as the model manager. That approach is explained in the documentation here and a nice example of what that looks like in this Julia Evans post.

Sorting

This PR sorts a QuerySet of Django model instances into an ordered list for display. Django QuerySets have a composable order_by method that should be used for ordering queryest as required. We should only do the sort ourselves in Python as a last resort, if there is a good reason not to do in with order_by. Using QuerySet methods lets us use the nice API the framework provides, write less code ourselves, and will do the work in the database when required, which will probably be orders of magnitude more performant.

https://docs.djangoproject.com/en/6.0/ref/models/querysets/#django.db.models.query.QuerySet.order_by

On performance: It took me 13s to open http://localhost:7000/codelist/opensafely/under-review/ on first visit, of which only 4ms was database queries. After that visits (such as changing sort order) took about 75-150ms, of which almost all was CPU time. http://localhost:7000/codelist/opensafely/ was similar. I am unsure if the initial visit was my local database warming up or populating caches or just a blip. ~100ms delay is something we could ignore as not materially affecting UX if we wanted, but we can do better that that and should use QuerySet for consistency reasons anyway.

Back on dc76451 the page took about 50-60ms to load, about 3ms on database queries. An extra 50-100ms is not a dealbreaker as performance is not very important to us, but hints how expensive unpacking into Python lists and sorting in Python can be, compared to doing it in the database. And we should be careful about compounding such performance hits.

[mikerkelly]

Suggestion, optional, terminology: I think field and order are more idiomatic and informative terms to qualify a sort than sort and direction. Those are the words used in Django QuerySet, so it probably makes sense to mirror that in our code.

https://docs.djangoproject.com/en/6.0/ref/models/querysets/#order-by

[mikerkelly]

Suggestion, optional: Python has Enum to represent a set of names bound to values. Since Python 3.11, we have StrEnum, which is slightly nicer when just working with string values, like we do here.

https://docs.python.org/3/library/enum.html#enum.StrEnum

[mikerkelly]

Suggestion, optional: We know that later we will want to select valid sort fields by status. So instead of defining SORT_FOR_PUBLISHED and SORT_FOR_UNDER_REVIEW, you could define a dict keyed on Status, mapping to the allowed values. Then the other code can change behaviour by keying into this dictionary based on status rather than having to define explicitly which to use. This can make code drastically more expressive and concise, which makes it easier to follow and validate.

[mikerkelly]

Putting those together could could look something like:

from enum import StrEnum, auto


class SortOrder(StrEnum):
    ASC = auto()  # automatically "asc" ...
    DESC = auto()


class SortField(StrEnum):
    CREATED_AT = auto()
    NAME = auto()
    UPDATED_AT = auto()


ALLOWED_SORT_FIELDS = {
    Status.PUBLISHED: {SortField.NAME, SortField.UPDATED_AT},
    Status.UNDER_REVIEW: {SortField.NAME, SortField.CREATED_AT},
}

For example, that could simplify this:

sort_options_by_status = {
    Status.PUBLISHED: SORT_FOR_PUBLISHED,
    Status.UNDER_REVIEW: SORT_FOR_UNDER_REVIEW,
}
sort_options = sort_options_by_status.get(status, {SORT_BY_NAME})

query_sort_by = request.GET.get("sort")
if query_sort_by in sort_options:
    sort_by = query_sort_by
else:
    sort_by = SORT_BY_NAME

to this (combining with ternary operator):

query_sort = request.GET.get("sort")
sort_by = query_sort if query_sort in ALLOWED_SORT_FIELDS[status] else SortField.NAME

[mikerkelly]

Earlier I suggest that this can be simplified like:

query_sort = request.GET.get("sort")
sort_by = query_sort if query_sort in ALLOWED_SORT_FIELDS[status] else SortField.NAME

[mikerkelly]

Separation of concerns: I think that we should do the extraction of all of the query parameters first, in the view or in a function dedicated to that. Currently the view is passing the request to many of the utility functions, and it is not obvious why until you hunt down which specific parameters they later pluck out of the object.

It seems surprising that a function called _get_organisation_handles is returning something called q and also the organisation, as well as the handles. And that not all organisation handles are returned. Maybe these should be reorganised?

[mikerkelly]

Naming: Please let's not use variable names that are just initial letters of what we really mean. Otherwise the reader has to work that out for themselves. Obvious things are not obvious, and there are often more than one relevant concepts starting with a letter. So q should be query, probably, or maybe search_term would be even better. Here and throughout. Possibly also in the URL query parameters.

I know that this is how the index code does it. Probably it should change there, too. Actually, that module provides a great example of why this is problematic, as search_handles includes both a parameter called q and many references to Django Q objects, which are a totally different thing (and also have a bad name).

[mikerkelly]

So something like:

def organisation_published(request: HttpRequest, organisation_slug: str) -> HttpResponse:
    status = Status.PUBLISHED
    search_term, page, sort_order, sort_field = _parse_query_params(request)
    organisation = get_object_or_404(Organisation, slug=organisation_slug)
    handles = (
        Handle.objects.filter(organisation=organisation)
        .filter(is_current=True, codelist__is_private=False)
        .filter(
            Exists(
                CodelistVersion.objects.filter(
                    codelist=OuterRef("codelist"), status=Status.PUBLISHED  # change this in the other view 
                )
            )
        )
        .select_related("codelist")
        .order_by(Lower("name"))  # can branch or have a look to add the right parameter here
    )

    # compose with `search_handles` if search term set.    

    paginator =  Paginator(handles, PAGE_SIZE).get_page(request.GET.get("page"))

[mikerkelly]

We can pass the handles queryset directly to the paginator, we don't need to convert it into a list. The templates can access {{handle.codelist}} fine.

[mikerkelly]

I don't ever see first, last, or page number links no matter how many search results I have or which page I am on. Could you please check that this is working for you and tell me what parameters you were using to see that?

[mikerkelly]

This is almost identical between the two view functions. Maybe this can be a shared function?

[mikerkelly]

Why do we have sort_options the context as a sub-dict but also have each sort option as a top-level entry in the context dict?

[mikerkelly]

I think it will be simpler to use the freezer pytest extension fixture to move time before creating each codelist.

If you want to do it declaratively without freezer then the required datetime should be a parameter to create_codelist so that functionality can be shared between tests, not special a helper just for this module.

[mikerkelly]

Maybe worth a brief comment explanation that the creation order is BaC but the case-insenstive lexicographic order is aBC. That makes it really obvious why these are called what they are.

You can also add id parameters to pytest test parametrisation arguments to hint as to the purpose of each parametrisation.

[mikerkelly]

Question: do we really need to use the test client fixture in these tests? Would it be sufficient to use the rf request factory fixture instead and call the view code directly?

I ask because using the client makes a full real HTTP request to the test server, which is much slower than rf.

Using client makes this more like what in Job Server we call an integration test, of high level behaviour, with separate rf tests for the more numerous and detailed unit tests.

[mikerkelly]

I think it would be quite a bit cleaner to extend Paginator to either overload its methods or add new ones, if we want to do this logic in the view Python code. Don't have time to expand now, sorry.

[mikerkelly]

If we use order_by to do QuerySet sorting as suggested elsewhere, this dict could be at the module level and take a (SortField, SortOrder) tuple as key, and map to the argument needed to order_by. Then function is not needed, we can do something like

ordering_params = {
    (SortField.NAME, SortOrder.ASC): "name__iexact"
    (SortField.NAME, SortOrder.DESC): "-name__iexact"
    ...
}
...
.order_by(ordering_params[(sort_field, sort_order)])