RDCP: Sort by system modified date in browse by collection view #663
Conversation
To support time travel in tests where it's valuable
There was a problem hiding this comment.
@mcritchlow It looks good. Just curios regarding those two questions. Thanks.
|
|
||
| # RDCP prefers their collections sorted by most recently created first | ||
| # Therefore, we're sorting by modified date rather than title | ||
| params[:sort] = 'system_modified_dtsi desc, title_ssi asc' if params[:id].eql? 'rdcp' |
There was a problem hiding this comment.
Maybe this line can be in front of line#309 if the condition is correct? It may not be a big deal but just curios regarding the condition unless params[:sort] in line#309 works.
There was a problem hiding this comment.
My thinking was that this would be last in the chain of sorting checks. If I put it above, then it could potentially get overwritten later. But I may be misunderstanding you, can you help clarify with an example maybe?
There was a problem hiding this comment.
@mcritchlow I think it's just related to your last comment in #663 (comment). The default sort by title will be set with the unless params[:sort] in https://github.com/ucsdlib/damspas/pull/663/files#diff-956e46b33fb5307990c4e1a4b5fd86ccR309 only when there are no sort order rules are selected.
| old_collection_sort_order = page.body.index('A Old Collection') | ||
| new_collection_sort_order = page.body.index('The New Collection') | ||
| expect(old_collection_sort_order).to be > new_collection_sort_order | ||
| end |
There was a problem hiding this comment.
Could we add a test for user selected sorting rules like sort by collection title to see whether it works? It seems like there could be a gap but I may be wrong. See my other comment above.
There was a problem hiding this comment.
The default sort I think is tested in the DLDP browse test I wrote below the RDCP one, which basically demonstrates the normal alphabetical sort. Can you take a look at that again and let me know if you think there's still a gap that needs testing? It's certainly possible :)
There was a problem hiding this comment.
@lsitu - Oh, I apologize. You're saying if a user selects different sorting rules after the page has initially loaded. That's a really great point, and possibly a regression. The current code might always force this sorting for RDCP. I understand what you're saying now, thanks for pointing this out. I'll think about how to adjust for this.
- Only apply RDCP sorting if a user has not already selected a sort option in the UI - Adds a test for a user selected option when browsing RDCP collections
|
@lsitu - Thank you again for catching the user-selected sorting regression! I've changed the code in catalog controller to only apply those defaults if no sorting option has been selected by the user. And I added a test to confirm that. Let me know if you think this works. |
|
👍 |
Fixes #644
Local Checklist
masterbranch?What does this PR do?
system_modifieddate descending, withtitleascending as a secondary sort parametertimecopgem to easily support time-sensitive tests, which were relevant for the behavior in this PRWhy are we doing this? Any context of related work?
References #644 - for RDCP
@ucsdlib/developers - please review