AO3-7143 Adding filter to user collection page by ReverM · Pull Request #5788 · otwcode/otwarchive

ReverM · 2026-05-05T20:57:28Z

Issue

https://otwarchive.atlassian.net/browse/AO3-7143

Purpose

This adds the ability to filter collections on user pages like on the generic collections page.

Credit

Danaël / Rever (they / he)

Include closed collections to open challenge check

This reverts commit b3246cd.

…AO3-7143

ReverM · 2026-05-05T20:58:31Z

#5487 original PR. Reopened the PR since merging upstream made it quite angry. Apologies for the added complexity.

ReverM · 2026-05-05T21:01:13Z

@sarken pinging you since you commented in the previous PR

ReverM · 2026-05-05T23:47:02Z

It seems that the spec controller failure is not expected behaviour after this PR?

sarken

I checked on the failing spec, and it looks like it's failing because we lost alphabetical sorting on user collection pages. That's something we definitely want to keep; users weren't happy when we accidentally switched to created_at sorting.

sarken · 2026-05-06T05:10:45Z

-    <% if @user && @user == current_user %>
-      <li><%= span_if_current ts("Collections"), user_collections_path(@user) %></li>
-      <li><%= link_to ts("Manage Collection Items"), user_collection_items_path(@user) %></li>
+<div class="navigation actions module">


We only put our subnavigation in div like this in rare instances where we have complex navigation with multiple separate sets of actions, like two lists, or a list and a form outside the list. The div on works/index.html.erb is there because we can have both a form and a list... but I think it's wrong, actually, because they shouldn't be there at the same time.

I'm pretty sure the reason you've added this is to make the navigation take up the full width of #main and push the filters into the right position on the page. Interestingly, the bookmarks index doesn't have this div, nor does it have that problem -- and after some investigating, I figured out why. Both bookmark and work index pages leave the empty ol.index on the page, while the collection index removes it with the unless @collections.blank? logic on line 52. Empty list elements aren't great for accessibility, so we don't want to replicate that here, either.

I think the solution we need here is to add a clear to form.filters. That should keep the filters in the right position without either the empty list or the div.

It seems that stylesheet 17 is taking priority over stylesheet 18. Regardless of if the index has filtered or not. The width: 100% seems to be the cause of the issue in that case? It is defined to be 75% in sheet 18 within line 9 but the one in style sheet 17 is applied first. Adding !important to the one in sheet 18 seems to fix it? But I wasn't sure if such a solution was desired.

Which page and device/browser, and which width: 100%? After actioning this comment, the only width: 100% I can find in 17 is .dashboard.works-index h4.landmark, .dashboard.works-index ol.pagination, and I'm not running into issues on either the user collection index or work index, but I've only checked in desktop Safari.

0c99f89 has the changes I'm testing with.

Oh I see, I got confused and reintroduced the class in page 17.

sarken · 2026-05-06T05:11:14Z

-      <li><%= link_to ts("Manage Collection Items"), user_collection_items_path(@user) %></li>
+<div class="navigation actions module">
+  <h3 class="landmark heading"><%= t(".navigation.landmark_heading") %></h3>
+  <ul class="navigation actions" role="navigation">


You can remove the role="navigation" while you're here -- it's not valid on list elements.

sarken · 2026-05-06T06:47:21Z

-  <h3 class="landmark heading"><%= ts("List of Collections") %></h3>
-  <ul class="collection picture index group">
+  <h3 class="landmark heading"><%= t(".main_content.landmark_heading") %></h3>
+  <ul class="collection picture index group <%= "no-filter" unless @sort_and_filter %>">


Instead of adding a class, let's take a look at what's causing the problem here. (But just a quick note for the future that we have some rules for adding classes.)

It looks like all collection index pages get the filtered class, regardless of whether they have filters, thanks to this:

otwarchive/app/helpers/application_helper.rb

Lines 60 to 62 in 7f6d5b1

def page_has_filters?

@facets.present? || (controller.action_name == 'index' && controller.controller_name == 'collections') || (controller.action_name == 'unassigned' && controller.controller_name == 'fandoms')

end

otwarchive/app/helpers/application_helper.rb

Lines 19 to 25 in 7f6d5b1

def classes_for_main

class_names = controller.controller_name + '-' + controller.action_name

show_sidebar = (!@hide_dashboard && (@user || @admin_posts || @collection || show_wrangling_dashboard))

class_names += " dashboard" if show_sidebar

class_names += " filtered" if page_has_filters?

Unfortunately, collections don't use @facets, so we can't just remove the collections logic from page_has_filters?. But we can tweak it to (controller.controller_name == "collections" && @sort_and_filter) or even just @sort_and_filter, since collections are the only place we use that variable, and I think it's reasonable to reserve it so it's only ever used to determine whether we display filters. #5721 also uses that variable for filtered challenge pages.

As a bonus, this approach should also fix the issue where /works/:id/collections pages

I was trying to find the root cause but couldn't quite find it. Thanks!

ReverM · 2026-05-12T00:02:39Z

I checked on the failing spec, and it looks like it's failing because we lost alphabetical sorting on user collection pages. That's something we definitely want to keep; users weren't happy when we accidentally switched to created_at sorting.

It seems that the default sort column is defined to be created_at in collection_search_form.rb. Should we switch to title.keyword?

sarken · 2026-05-12T01:27:08Z

We just want to change the sort column on the user collection pages, not everywhere, so I don't think that's the right place to do it. Does restoring sort_column: "title.keyword" in the controller not work here?

sarken · 2026-05-17T03:56:23Z

No rush, just noting that I put the label back to Reviewed: Action Needed since there were still test failures.

ReverM · 2026-05-18T17:39:38Z

I found the issue for the cucumber test, waiting for response for the rubocop failure.

redsummernight · 2026-05-20T05:58:28Z

    And the 2nd collection result should contain "Privates"
    And the 2nd collection result should contain "Bookmarked Items: 1"

+Scenario: Sort collections by Works and Bookmarks from user's collection


Since this issue adds the user collection page, we want to test that such a page only shows collections belonging to the user. You covered this in the other feature file, with "Filtering on a user collection page should only return collections by that user".

On the other hand, I think we can skip this scenario. It's the same as "Scenario: Sort collections by Works and Bookmarks". It also doesn't deal with multiple collection owners, just the same one that got renamed.

redsummernight · 2026-05-20T06:00:58Z

+    When I go to recengine's collections page
+      And I fill in "Filter by title" with "Duplicate Name"
+      And I press "Sort and Filter"
+    Then I should see "1 Collection"


You can add checks for the titles that should show up, "1nd collection result should contain" and so on.

redsummernight · 2026-05-20T06:02:38Z

+    Then I should see "2 Collections"
+    When I fill in "Filter by title" with "Not Mine!"
+      And I press "Sort and Filter"
+    Then I should not see "iminvisible"


This "should not see" step can be skipped since there's already a better "no collections found" check that covers the same thing.

redsummernight · 2026-05-20T06:08:40Z

+      @sort_and_filter = true
      @user = User.find_by!(login: params[:user_id])
-      @search = CollectionSearchForm.new({ maintainer_id: @user.id, sort_column: "title.keyword" }.merge(page: params[:page]))
+      @search = CollectionSearchForm.new(collection_filter_params.merge({ maintainer_id: @user.id, default_sort_column: "title.keyword" }.merge(page: params[:page])))


Can we use sort_column instead of a new parameter?

sort_column would override whatever sorting the user tries to make. It cannot be used

sarken · 2026-05-20T05:17:53Z

    And the 2nd collection result should contain "Privates"
    And the 2nd collection result should contain "Bookmarked Items: 1"

+Scenario: Sort collections by Works and Bookmarks from user's collection


I've left a couple of comments on lines in this scenario, but it looks like it's basically a duplicate of the scenario above it; the main difference appears to be that we are explicitly setting the sort direction to descending.

I'd suggest converting the previous test to a scenario outline, adding steps to specify the sort direction, and using the examples the collection page and moderator's collection page. It should cover the same things while adding fewer lines of code.

The sort direction is specified in this one because the collection page default to date_updated + descending, while the user collection is title.keyword + ascending

sarken · 2026-05-20T05:49:44Z

      And I press "Sort and Filter"
    Then I should see "Your search failed because of a syntax error"
+
+  @collections


We don't usually put tags before individual scenarios unless the tag is pulling double duty as a hook (e.g., @javascript and @load-default-skin).

sarken · 2026-05-20T05:50:18Z

    Then I should see "Your search failed because of a syntax error"
+
+  @collections
+  Scenario: Filtering on a user collection page should only return collections by that user.


I think this scenario would be better in collection_browse.feature since it doesn't need the Background steps at the top of this file.

However, like redsummernight said, I think we can also just live without this.

Should this scenario be removed then?

Switched to a scenario outline due to 2 tests being very similar, and moved a test from filters

sarken · 2026-05-21T01:05:17Z


  def default_sort_column
-    "created_at"
+    options[:default_sort_column] ||= "created_at"


To red's point about not adding a new parameter, would something like options[:maintainer_id] ? "title.keyword" : "created_at" work here instead of setting the parameter in the controller?

sarken · 2026-05-21T01:07:32Z


  def collection_filter_params
-    params.permit(:commit, collection_search: [
+    params.permit(:commit, :user_id, collection_search: [


Do you know when the user_id parameter is used? I didn't notice it in the URL and it seemed to work without this addition, but it was only a quick test while I was looking into red's suggestion.

ReverM and others added 28 commits November 18, 2025 11:08

AO3-4666

b3246cd

Include closed collections to open challenge check

Added filter to user collection pages

aae1900

Added test

c202951

Added missing definition for test

960ef97

Revert "AO3-4666"

f512be0

This reverts commit b3246cd.

Added missing s to test

466ffa6

Removed test and cleaned things up 1/2

cdad56d

Clean up 2/2

89eae21

Merge branch 'otwcode:master' into AO3-7143

23a987b

Added test and fixed style

a00bc74

Missed an s

322e4bc

Adressed rubocop for tests

9e610be

Adressed comments

c98d152

Removed newline so file is untouched by pr

2ad01c0

Added filter to user collection pages

6eb7ad8

Added test

30e34b7

Added missing definition for test

42163e5

Added missing s to test

182f9fa

Removed test and cleaned things up 1/2

84bfc54

Clean up 2/2

7b65fe4

Added test and fixed style

07776bb

Missed an s

d2a6a0d

Adressed rubocop for tests

ec61299

Adressed comments

1fef682

Removed newline so file is untouched by pr

d16b8d6

Adress comments

2da1469

Better tests

b180115

Merge branch 'AO3-7143' of https://github.com/ReverM/otwarchive into …

67f0718

…AO3-7143

github-actions Bot added the Awaiting Review label May 5, 2026

Styling

f9a5551

sarken requested changes May 8, 2026

View reviewed changes

sarken added Reviewed: Action Needed and removed Awaiting Review labels May 8, 2026

Adressed PR comments

7eaedbe

github-actions Bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels May 12, 2026

Making test not fail, hopefully

4a4036f

sarken added Reviewed: Action Needed and removed Coder Has Actioned Review labels May 15, 2026

Properly addressed failing tests

f2393fd

github-actions Bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels May 18, 2026

redsummernight reviewed May 20, 2026

View reviewed changes

redsummernight added Reviewed: Action Needed and removed Coder Has Actioned Review labels May 20, 2026

sarken requested changes May 20, 2026

View reviewed changes

Test update and reintroducing function

ba294ab

Switched to a scenario outline due to 2 tests being very similar, and moved a test from filters

github-actions Bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels May 20, 2026

ReverM added 2 commits May 20, 2026 16:42

Forgot I changed the description of file

4060147

Rubocop

311ffda

sarken reviewed May 21, 2026

View reviewed changes

	def page_has_filters?
	@facets.present? \|\| (controller.action_name == 'index' && controller.controller_name == 'collections') \|\| (controller.action_name == 'unassigned' && controller.controller_name == 'fandoms')
	end

	def classes_for_main
	class_names = controller.controller_name + '-' + controller.action_name

	show_sidebar = (!@hide_dashboard && (@user \|\| @admin_posts \|\| @collection \|\| show_wrangling_dashboard))
	class_names += " dashboard" if show_sidebar

	class_names += " filtered" if page_has_filters?

Conversation

ReverM commented May 5, 2026

Issue

Purpose

Credit

Uh oh!

ReverM commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ReverM commented May 5, 2026

Uh oh!

ReverM commented May 5, 2026

Uh oh!

sarken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReverM May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReverM commented May 12, 2026

Uh oh!

sarken commented May 12, 2026

Uh oh!

sarken commented May 17, 2026

Uh oh!

ReverM commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ReverM commented May 5, 2026 •

edited

Loading

ReverM May 11, 2026 •

edited

Loading