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

ANW-1101 use column prefs for top container listing #3010

Merged

Conversation

donaldjosephsmith
Copy link
Collaborator

These changes to the top containers controller and results view make the table listing configure columns based the standard browse/search column preferences and default sort, instead of being hard-coded.

This table view is different than other record types, and the columns and default sort were hard-coded previously. The sorting is performed in client-side javascript, and the user's chosen sort columns are saved in session storage, so the default sort will only take effect at the beginning of a new user session.

A few fields that were present in the hard-coded table had to be added to the search/browse column config in order to make them show up in the select boxes on the preferences page. Something to beware of: the Solr query to fetch the data uses the default sort field/direction from the preferences, so anything there (marked as sortable in search_browse_column_config.rb) must have a sort_by field specified that is in the index, otherwise the query will crash. It doesn't impact the results table directly since the data is sorted client-side, but it is an unfortunate complication that limits what can be chosen as a default sort.

Definitely not a perfect fit; it's a bit of a compromise for the sake of app consistency and implementation practicality.

@donaldjosephsmith donaldjosephsmith force-pushed the anw-1101-enable-top-container-listing-column-preferences branch from 4bd07f7 to e3d256c Compare May 30, 2023 20:42
@donaldjosephsmith donaldjosephsmith force-pushed the anw-1101-enable-top-container-listing-column-preferences branch 4 times, most recently from 9ce06cb to b14d475 Compare July 3, 2023 13:41
@donaldjosephsmith donaldjosephsmith force-pushed the anw-1101-enable-top-container-listing-column-preferences branch 5 times, most recently from 0e0cfdb to 92edd63 Compare July 7, 2023 13:53
@donaldjosephsmith donaldjosephsmith force-pushed the anw-1101-enable-top-container-listing-column-preferences branch from 92edd63 to 0a665c9 Compare July 11, 2023 13:40
@donaldjosephsmith
Copy link
Collaborator Author

Since these changes broke some of the tests in the selenium spec for containers/instances, I took the opportunity to rewrite them as a Capybara feature test. However, some tests required full indexing runs (run_all_indexers), and they were getting hung up at that point. After an inordinate amount of head-scratching, I discovered that the spec helper for the newer feature tests had forgotten to create a periodic indexer. Due to a peculiarity of Ruby and the way the indexers are used by the test helpers, it was just hanging during the attempt at calling the periodic indexer instead of throwing an exception. The periodic indexer initialization was added and cleared up these failures.

Due to issues that the team has been experiencing with some test specs (in many cases old selenium tests or even tests for other apps that have nothing to do with the changes) passing with no issue on our dev machines but failing during GitHub CI. In one case, the selenium resources_spec, I was able to add some 1-second sleeps to get it to go go through reliably. However, there were other tests failing intermittently where I didn't see any obvious workarounds, so I ended up re-running the CI several times until they all finally passed in the same run. (We are working on setting up our own cloud test runners to rectify these issues.)

@donaldjosephsmith donaldjosephsmith marked this pull request as ready for review July 11, 2023 20:21
quoideneuf
quoideneuf previously approved these changes Aug 18, 2023
Copy link
Collaborator

@quoideneuf quoideneuf left a comment

Choose a reason for hiding this comment

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

@donaldjosephsmith Looks good to me, but I did see one type in the view - see suggestion.

…l.erb

Co-authored-by: Brian Hoffman <brian.hoffman@lyrasis.org>
Copy link
Collaborator

@brianzelip brianzelip left a comment

Choose a reason for hiding this comment

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

👍🏼

@brianzelip brianzelip merged commit 3c6ee36 into master Aug 28, 2023
12 checks passed
@cdibella cdibella added this to the 3.5.0 milestone Aug 28, 2023
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.

None yet

4 participants