dodona
dodona copied to clipboard
dodona-edu
Add dynamic counts to search filters
This pull request contains a refactor of search filters. It adds dynamic counts of the expected results to all search filters. When already partially filtered, it also limits the other filters to only show relevant options.
The initial goal was to get rid of some old code that used js functions as part of 'filterCollection' objects to extract the color and paramVal from a filter option. This made most related code quite ugly and there was no longer any need for it.
So with cc610d658950ff2679f3a362dca044e81194b20a and 810fa47aae22c7cc2bd3ece8d745be697e3497c4 I made filterCollection a pure map of data objects.
With these changes it became a lot simpler to dynamically update filters. I tried to write generic concerns that automatically define a dynamic option count when defining a scope. This way the end result should be less specific code.
In the most basic case writing a filter for a given field of a model is as simple as writing filterable_by :field in the model and writing has_filter :field in the controller. In the controller, the @filters variable can then be set by calling filters(elements) on the unfiltered list of elements. @filters is then used to update the shown filter options and counts.
Common cases, such as enums and foreign keys can be filtered using filterable_by :field, is_enum: true and filterable_by :field, model: Model respectively.
Naturally there are always some more complex cases, in which case a custom name_hash function can be written or even a full custom count function. See activity.rb for some examples of this.
The most complex case were course_labels as these required the course as extra argument. For now these are generalized in their own specific reusable function.
This PR seems to include a bunch of changes from other PRs as well (dependencies, fix for graph time, ...) Maybe double check why this is and if we won't merge things we don't want.
These were all changes already on main. A new merge with main has fixed the issue of them being listed here.
Can you explain the user of the Filters header?
This is only used in a single place. This usecase is saved annotations. The saved annotations list is the only page that doesn't fetch javascript upon search, but instead fetches the json data and feeds that to the web components. This request now also needs the filter information to update the filters. I treated the filter metadata the same way as the pagination metadata by including them in the headers of the request.
Did you test the performance of these changes? I don't know if adding the counts needs a lot of additional queries? e.g. for the submissions table this might be quite impactful.
I minimized the amount of queries. But testing it on naos, the queries are unusable slow. I'll have a look if they can be easily improved
We might have to reconsider this whole pr :( I underestimated how hard it would be to optimize 'COUNT' queries... There is no simple fix such as adding indexes as it seems innoDB does not use these indexes for counting.
The only suggested solution is keeping track of summary tables. But implementing this in a flexible way for all our search pages would add a whole new layer of complexity...
Maybe I overreacted.
The newly added count queries have a similar speed as the existing count query that already ran for pagination. For most pages it doesn't have a big impact. Most pages only have one or two filters and this results into at most 100ms extra response time on naos, but often a lot less.
I have already removed the counts for the submission pages, as it was unbearably slow there (but these pages where already way to slow on naos anyway). For reference the pagination count takes 15.8 seconds out of the total 16.7 seconds on the submissions page. Adding up to two extra count queries to this page tripled response time. Side note: removing the page count on this page would greatly speed up the page.
The activities page is an edge case. With 7 filters the increase in query time can be up to 300ms, which is about 20% of the total time required to render the page. This makes interactivity on naos feel slightly suboptimal. But our production server is always a bit faster, and the UX improvement is greatest for activity selection, so it might be worth a 20% increase in loading time. This page also has room for speed ups in other places, such as removing the need for n+1 queries for popularity and activity status.
