Add organization view UI filters

[agjohnson]

This filters are used for the new dashboard UI, they are not API filters.

These were a challenge to create, as django-filter is really opinionated
about the way it should work, and doesn't quite agree with use cases
that need to use filtered querysets (such as limiting the field choices
to objects the organization is related to).

I went through many permutations of this code, trying to find a pattern
that was not obtuse or awful. Unfortunately, I don't quite like the
patterns here, but because all of the django-filter magic happens at the
class level instead of at instantiation time, every direction required
hacks to obtain something reasonable.

Given the use we have for filters in our UI, I wouldn't mind making
these into a more generalized filter solution.

I think I'd like to replace some of the existing filters that currently
hit the API with frontend code and replace those with proper filters
too. These are mostly the project/version listing elements.

[agjohnson]

I'm currently looking for some input on whether any of this is a bad approach. Do note that the patterns here are not very nice, but I tried many different approaches and nothing worked. The main problem is that django-filter does a lot at the class creation time and provides little support for use cases that need to tune filter fields at view time (filtering teams that are only related to a single organization).

Here's what it looks like in the UI currently:

Screencast.from.2023-10-18.19-59-55.webm

[stsewd]

I'm guessing you are passing both querysets, so they can be re-used in the context of the old templates. I like more the idea of passing the object that the querysets need to be filtered by, like in

readthedocs.org/readthedocs/organizations/filters.py

Line 275 in e795e57

self.organization = organization

The filterset class could expose those querysets like OrganizationProjectListFilterSet.get_teams, so it can be re-used in the context. But not sure if that will execute the query twice (maybe cache the method if that happens).

Just an idea, not sure if this will end up being more tedious than just passing the queryset.

[agjohnson]

I also leaned this way and would consider passing in organization instead of the extra queryset instance(s) to be a bit cleaner.

I started this direction too, but did realize that this would be confusing as all of these querysets are already defined and evaluated at the view level. There is potential to duplicate queries though, yeah.

Duplicate queries might not even be a concern though. The filters aren't used on the current dashboard and I don't think all of the view queries will be used by the views once the new dashboard is the only dashboard.

I'm solidly impartial on changing back to this at this point. But, if folks feel it's a better design to define these querysets at the FilterSet level, I'm also 👍

[humitos]

but because all of the django-filter magic happens at the
class level instead of at instantiation time

It seems what you want here is overriding the .qs property of your filters: https://django-filter.readthedocs.io/en/stable/guide/usage.html#filtering-the-primary-qs. That will allow you filter the results based on whatever the user has selected from the UI and their permissions on the objects.

Then inside the .qs property, I'd put all the logic you have inside get_context_data to get the Team/Project querysets based on users permissions as you are already doing.

This approach seems to be clearer to me since it's will be self-contained in the filter without "contaminating" the view, but the logic would be similar to what you already have, I think.

[humitos]

Thinking completely differently here: I've never used django-filter outside Django REST Framework and I'm not sure it's going to be useful outside that context even. I didn't find good examples in their documentation for this (the only example they have is this one: https://django-filter.readthedocs.io/en/stable/guide/usage.html#the-view). What are the benefits of using django-filter in the way you are using it here?

I'd probably say that we should create our own class to perform these queries in the same way we are doing with AdminPermission (

readthedocs.org/readthedocs/core/permissions.py

Line 10 in 00d188e

class AdminPermissionBase:

) following the same pattern and having consistency across our code base and avoiding these hacks that don't feel good for anybody 😄

[agjohnson]

It seems what you want here is overriding the .qs property of your filters

Thanks for the suggestion, but this pattern doesn't quite solve the problem we have with django-filter. I know of the qs property, as well as passing queryset as a function, but neither helped to filter the individual filter field querysets. Overall, defining the main queryset used in the FilterSet is not a burden. What you are describing could be a way to refactor our view/mixin and filter querysets, but I felt this was a later project.

The primary problem, which I describe here, is that the individual filter fields in the filterset will query the models separately to populate form choices. For example, the default queryset used to populate choices for the list of teams filter field is Team.objects.all() where we need it to be Team.objects.member(self.request.user, organization=org). It's important to note that this queryset is not the same queryset defined by FilterSet.queryset/FilterSet.qs/etc.

Redefining the qs property method is helpful in a different way. We could use this if we want to move the queryset definitions out of the underlying views/mixins and into the FilterSet. But this does not solve the problem above.

Overriding qs would be a code organization sort of change, as it would not alter our need for filtering the model choices. I did contemplate this change, but don't feel this is worth the effort, and feel the code is clearer with the querysets defined at one place: the view/mixin. Given we can't move this logic without affecting the current dashboard, I do still lean towards leaving the querysets at the view/mixin unless we have a very strong reason to move these querysets.

To clarify, the queryset for teams comes from here:

readthedocs.org/readthedocs/organizations/views/base.py

Lines 105 to 114 in 9d55392

	def get_team_queryset(self):
	"""
	Return teams visible to request user.
	This will either be team the user is a member of, or teams where the
	user is an owner of the organization.
	"""
	return Team.objects.member(self.request.user).filter(
	organization=self.get_organization(),
	).order_by('name')

This queryset is set at FilterSet.__init__ as all of the configuration of the FilterSet happens at class definition time, and we need to pass in view-specific variables like the organization that the views are describing.

I hope that makes sense.

Thinking completely differently here: I've never used django-filter outside Django REST Framework and I'm not sure it's going to be useful outside that context even.

It is still very useful, and creating UI forms for the view is still standard usage of the library. The main problem is that FilterSet filters only support customizing querysets with request and not customizing querysets with view specific variables. Django-filter doesn't explicitly say how to integrate at the template level, but it just like any other form: {{ filter|crispy }}.

What django-filter provides is:

• A form object for submitting filters. This is rendered directly by Crispy.
• A form field for each filter dropdown in the filter UI
• Filter dropdowns are populated with choices, either manually or ModelChoiceField choices are populated with a queryset. This is not the same queryset as FilterSet.qs.
• Validation on form choices
• Processing of URL params into field filter choices, including validation

I'd probably say that we should create our own class

I would say that this is not a good direction. We are already using djagno-filter for filter UI for one, and I don't think mixing patterns is great. But more importantly, what you're describing is basically recreating django-filter in our own code. Django-filter does not quite support this use case, but it does still do all of the lifting that we need otherwise, and would be a lot of code to recreate in our own application.

I do think this is an opportunity to improve django-filter though, and would probably focus my efforts on putting in a patch to django-filter.

[humitos]

Thanks for the detailed explanation 💯

Django-filter does not quite support this use case

Maybe it's worth opening an issue on the project explaining this situation to see if they have already considered this.

I do think this is an opportunity to improve django-filter though, and would probably focus my efforts on putting in a patch to django-filter.

This is a good outcome 😉

[agjohnson]

Well, in trying to open an issue on django-filter, and thinking about how to describe the issue and what I was hoping to accomplish, I might have come up with a cleaner solution 😄

I think what I wanted here all along was something comparable to the ChoiceField.method attribute:

https://django-filter.readthedocs.io/en/stable/ref/filters.html#method

This attribute allows passing the queryset to a method on the parent FilterSet instance. I tried this approach here and think it works a bit better: 1ccdfd3

This does move the queryset definition into the FilterSet. I've waffled on this, and we talked about above, but hopefully this feels cleaner. Does this seem like a better approach?

[agjohnson]

I went ahead with the solution above and I added a whole bunch of tests. Like, probably too many 💪

I'm feeling good about the solution now.

One thing that I don't quite like, and would like to eventually revisit, is that if you give the filter an invalid choice (you manually alter the URL to set a ?team__slug=doesntexist for example), the queryset returned is unaffected by the filter instead of returning an empty queryset. I'm not worried about this though, and the answer is probably really basic. But this technically makes sense, as it is probably throwing a form validation error.

[agjohnson]

if you give the filter an invalid choice the queryset returned is unaffected by the filter instead of returning an empty queryset

This was bugging me enough that I fixed it. It was an addition at the view layer, checking whether the filterset was valid or not. I got the UI and tests working for this as well.

I also found some of the view code from django-filters that plugs in fairly well with CBV. I didn't want to get too deep into restructuring our views though, so customized the mixin to fit our usage.

That is now the last I'll touch this, I'm happy with where this is now and feel way more comfortable with tests and a few bugs sorted out.

[humitos]

This looks a lot nicer than the previous approach. I didn't do an extensive review on permissions and tests, but I like this pattern more.

[agjohnson]

I've added some release notes to check these filters in production/beta after release. Thanks for the eyes on this @humitos and @stsewd !