-
Notifications
You must be signed in to change notification settings - Fork 0
ETT-1142 OTIS download report: filters with dropdown menus should inc… #277
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
Conversation
…lude all possible values for that field - Proof of concept which does not update values based on other popup menu selections (will test for viability) - Add `HTDownload` method `all_values` called by the presenter to populate the menus
aelkiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments about generating the filter list -- we don't need rights_date or pages. Performance seems OK for me in testing; if we run into issues in production with the joins to hathifiles we may consider alternate ways of getting the possible values.
Works fine for me in testing.
data_filter_data is a bit messy as you say, but we're likely to clean up at least some of that in the future, and I think the only remaining exception will be 'role'.
The tests all look fine.
- Set `pages` and `rights_date_used` to "input" rather than "select" - Remove support for these fields in `HTDownload.all_values`
|
I mentioned on the ticket, should build and deploy to staging and see what the performance looks like. |
|
@aelkiss Performance info noted on ticket. I think I'm good with what we have. |
…lude all possible values for that field
HTDownloadmethodall_valuescalled by the presenter to populate the menus.HTDownloadPresentercalls this method indata_filter_datanow for allselectcolumns, not justrole.data_filter_datahas key == value, butroleis still a special case, as it was before.full_downloadin subsequent work.This branch is currently on
otis-testing