Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updates to class-based view for filterable lists #4173

Merged
merged 9 commits into from
Jun 4, 2024
Merged

Updates to class-based view for filterable lists #4173

merged 9 commits into from
Jun 4, 2024

Conversation

joemull
Copy link
Member

@joemull joemull commented May 8, 2024 •
edited
Loading

These changes were made to provide functionality for openlibhums/consortial_billing#65.

They update the filterable class-based view in these ways:

  • Make the methods essentially generic by checking to see if it is a set of articles before doing article-specific things
  • Add filter types integer, search, and boolean
  • Add a simple way to search using Django admin's search functionality

@joemull joemull requested a review from ajrbyers May 8, 2024 19:16
@joemull joemull changed the title Supporter management Updates to filterable list class-based view May 8, 2024
@joemull joemull changed the title Updates to filterable list class-based view Updates to class-based view for filterable lists May 8, 2024
@joemull joemull requested review from mauromsl and removed request for ajrbyers May 21, 2024 17:04
@joemull joemull assigned mauromsl and unassigned ajrbyers May 21, 2024
mauromsl
mauromsl requested changes May 23, 2024
View reviewed changes
Copy link
Member

@mauromsl mauromsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look to be some useful new facets.

I've found a couple minor areas were we can improve the implementation, particularly one around polymorphism

src/core/views.py Outdated Show resolved Hide resolved
src/core/views.py Outdated Show resolved Hide resolved
src/core/views.py Outdated Show resolved Hide resolved
@mauromsl mauromsl assigned joemull and unassigned mauromsl May 23, 2024
@mauromsl mauromsl mentioned this pull request May 24, 2024
Merged
@joemull joemull requested a review from mauromsl May 29, 2024 16:42
@joemull joemull assigned mauromsl and unassigned joemull May 29, 2024
mauromsl
mauromsl approved these changes May 30, 2024
View reviewed changes
Copy link
Member

@mauromsl mauromsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last minor nitpick that I'll flag as optional

src/core/views.py Outdated Show resolved Hide resolved
@mauromsl mauromsl requested a review from ajrbyers May 30, 2024 16:15
@mauromsl mauromsl assigned ajrbyers and unassigned mauromsl May 30, 2024
@joemull
Improvements to filtered list view
a962271
@joemull
Add search using admin search
e8c2805
@joemull
Implement boolean fields on filtered list view
72a95f0
@joemull
Make add user field work if no journal
eda957b
@joemull
Make model utility for searching via admin search
761b007
@joemull
Make FilteredListView properly generic
d950673
@joemull
Remove deprecated null check
6249fab 
The if/else check deleted in this commit was used in an earlier
version of the view to let the user filter by whether data was missing
on a given facet. In other words, we wanted to be able
to send [something]__isnull=True in the query.
This was superceded with another appraoch, where some option representing
missing data was added to choices in a select field, and translated
into the query by means of an annotation.
@joemull
Use set for efficiency #63
057ae67
@joemull joemull force-pushed the cb-63-supporter-management branch from 6b9924f to 057ae67 Compare May 31, 2024 12:03
@joemull
Copy link
Member Author

joemull commented May 31, 2024

@ajrbyers rebasing done.

ajrbyers
ajrbyers approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@ajrbyers ajrbyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment, it doesn't block this PR from being merged but we will need to address it before deployment:

This PR removes FilteredArticlesListView which is in use in the isolinear plugin.

https://github.com/BirkbeckCTP/isolinear/blob/11775ad04b83a36af4f8967536a0ba19e359e6f1/views.py#L248-L253

@mauromsl
Copy link
Member

mauromsl commented Jun 3, 2024

Just one comment, it doesn't block this PR from being merged but we will need to address it before deployment:

This PR removes FilteredArticlesListView which is in use in the isolinear plugin.

https://github.com/BirkbeckCTP/isolinear/blob/11775ad04b83a36af4f8967536a0ba19e359e6f1/views.py#L248-L253

Good Catch, we'll need to still support this with a deprecation warning

Just one comment, it doesn't block this PR from being merged but we will need to address it before deployment:

This PR removes FilteredArticlesListView which is in use in the isolinear plugin.

https://github.com/BirkbeckCTP/isolinear/blob/11775ad04b83a36af4f8967536a0ba19e359e6f1/views.py#L248-L253

Good catch! @joemull can we preserve the name FilteredArticlesListView with a deprecation warning raised on its initialization? We want to avoid betraying semantic versioning

mauromsl
mauromsl requested changes Jun 3, 2024
View reviewed changes
src/core/views.py Show resolved Hide resolved
@mauromsl mauromsl assigned joemull and unassigned ajrbyers Jun 3, 2024
@joemull joemull requested a review from mauromsl June 3, 2024 15:57
@joemull joemull assigned mauromsl and unassigned joemull Jun 3, 2024
@mauromsl mauromsl merged commit 9013a4e into master Jun 4, 2024
1 check failed
@mauromsl mauromsl deleted the cb-63-supporter-management branch June 4, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajrbyers ajrbyers ajrbyers approved these changes

@mauromsl mauromsl mauromsl approved these changes

Assignees

@mauromsl mauromsl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joemull @mauromsl @ajrbyers