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.

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

New orderby filters #1021

Merged
merged 4 commits into from
Dec 18, 2023
Merged

New orderby filters #1021

merged 4 commits into from
Dec 18, 2023

Conversation

matti-lamppu
Copy link
Collaborator

@matti-lamppu matti-lamppu commented Dec 13, 2023

🛠️ Changelog

  • Add new order by filters to application, application events, and application event schedules endpoints

    • Applications
    • Events
    • Schedules
  • Add new field filters to event reservation units for preferred order

🧪 Test plan

  • Automated tests

🎫 Tickets

@matti-lamppu matti-lamppu changed the base branch from main to new-filters-for-application-events-and-scheudles December 13, 2023 10:22
Base automatically changed from new-filters-for-application-events-and-scheudles to main December 14, 2023 11:03
@matti-lamppu matti-lamppu marked this pull request as ready for review December 14, 2023 13:10
Copy link

Copy link

@vergama vergama left a comment

Choose a reason for hiding this comment

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

Looks good except that I started wondering why are the sorters for application_event.name name_fi/en/sv rather than just name? (Thus already before this PR, though.)
In the test env for now this GQL does work but orderBy: "name_en" or orderBy: "name_sv", on the other hand, don't affect the sort order (but are accepted, nevertheless):

{ applicationEvents(applicationRound: 92, orderBy: "-name_fi") {
  edges { node {
    pk, name
  } }
} }

@matti-lamppu
Copy link
Collaborator Author

Looks good except that I started wondering why are the sorters for application_event.name name_fi/en/sv rather than just name? (Thus already before this PR, though.) In the test env for now this GQL does work but orderBy: "name_en" or orderBy: "name_sv", on the other hand, don't affect the sort order (but are accepted, nevertheless):

{ applicationEvents(applicationRound: 92, orderBy: "-name_fi") {
  edges { node {
    pk, name
  } }
} }

@vergama For translated fields, name is an alias for name_fi, since Finnish is our default language (this is hot django-modeltranslation works). So, using name will filter with the Finnish names, and can work that way. Still, I don't know why name_en and name_sv are accepted through the filter. I'm pretty sure it should give an error about non-existing filter. This could be a bug between django-modeltranslation and django-filter.

@matti-lamppu matti-lamppu requested a review from vergama December 15, 2023 12:05
@matti-lamppu matti-lamppu merged commit e5ede5e into main Dec 18, 2023
5 checks passed
@matti-lamppu matti-lamppu deleted the new-orderby-filters branch December 18, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants