carltongibson / neapolitan

Quick CRUD views for Django
https://noumenal.es/neapolitan/
MIT License
413 stars 30 forks source link

Added filterset to list view context. #42

Closed carltongibson closed 3 months ago

carltongibson commented 3 months ago

Closes #12

carltongibson commented 3 months ago

@joshuadavidthomas @jefftriplett This adds the filterset key to the list-view context.

It's the minimal addition here. Maybe at some point we hold a reference to the filterset, but it's not used outside the list view, so this should resolve the narrow issue. (I'll come back for the templates in a second pass, but I'm assuming you have your own project base templates.)

I'm going for a walk. I roll a release later on today.

jefftriplett commented 3 months ago

@carltongibson This is great and saves me a base class. 🎉

Feel free to ignore this, but I wanted to pass it along if this is an artifact of the change. I had to modify how the filterset was handled in the view to avoid the frontend blowing up. (I can't remember if this was doing to using a custom ModelForm or not.)

        if filterset and (
            not filterset.is_bound or filterset.is_valid() or not self.get_strict()
        ):
            queryset = filterset.qs

I don't always understand what django-filters is doing so it's possible we were doing something weird.

Update: I fed my kids and found the context:

https://github.com/westerveltco/neapolitan/blob/main/src/neapolitan/views.py#L336-L341

carltongibson commented 3 months ago

@jefftriplett I saw this in the PR @joshuadavidthomas linked (your one there).

What's the self.get_strict()? 🤔

Behaviour should be that if not bound (if GET is empty) then is_valid() is automatically False. (So that should drop away... 🤔)

From my perspective this looks like a separate issue: happy to dig into it if you want to give me a reproduce with a sample model say? (Neapolitan's tests just are a single folder Django project, so you could add a model/form/view/etc there if that's easier than spinning up something separate.)

jefftriplett commented 3 months ago

From my perspective this looks like a separate issue: happy to dig into it if you want to give me a reproduce with a sample model say? (Neapolitan's tests just are a single folder Django project, so you could add a model/form/view/etc there if that's easier than spinning up something separate.)

I'll try to follow up this week since I'll be touching a project of @joshuadavidthomas's where we first stumbled on it. It would have been form + list view + pagination related so probably easy to recreate.

carltongibson commented 3 months ago

Great. Thanks @jefftriplett 👊