AO3-6925 Remove Open Challenges link from Subcollections page

[gelphcore]

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number
  as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait
  until they are reviewed and merged before creating new pull requests.

Issue

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

Purpose

• Remove the Open Challenges link from Subcollections page. (It still appears in the main collections index.)
• Have also removed the role="navigation" attribute from the ul

Before:

After:

Credit

gelphcore, they/them
Same on Jira

[sarken]

Hi, gelphcore!

Thank you so much for this pull request. I've left some comments on it, plus it looks like it could use some tests to make sure the correct links are present. The tests would probably be a good fit for features/collections/collections_browse.feature, which already has tests like that for the main collection index.

Since you've already created a Jira account, I've gone ahead and assigned the Jira issue to you and set its status to In Review. I've also given your account permission to comment on, assign, and transition issues in the future.

Thanks again for contributing! If you have any questions, you can contact us at [email protected].

[sarken]

This should say, "Collections link unless a logged in user viewing own collections page," without the "and." It also looks like the opening <% on all of the commented lines might have gotten eaten by a find + replace.

However, I just realized this comment is also indicative of a bug. This link points to ao3.org/collections and should never be used on a user's collections page. The link on a user's collections page should always use the code on line 37, regardless of whether the user is logged in.

So what we need here is:

<%# Collections link unless on a user's collections page %>
<% unless @user %>
  <li><%= span_if_current ts("Collections"), collections_path %></li>
<% end %>

And to replace lines 34-59:

<% if @user %>
  <%# User collections index gets link for user Collections %>
  <li><%= span_if_current ts("Collections"), user_collections_path(@user) %></li>
  <%# Logged in user on own collections index gets link for Manage Collections Item %>
  <% if logged_in? && @user && @user == current_user %>
    <li><%= link_to ts("Manage Collection Items"), user_collection_items_path(@user) %></li>
  <% end %>
<% end %>

However, that's a bit outside the issue you signed up for, so please let me know if you'd rather not work on that. I'm happy to create a new Jira issue for it instead.

[gelphcore]

Thanks for the review @sarken! Looks like the linter autocompleter was eating the <% so I added this to .erb-lint.yml:

Layout/CommentIndentation:
        Enabled: false

After implementing your code above, the Collections link on the Subcollections page still goes to the main collections index. Should this go to the user collections path instead? Or is this what you meant for it to do?

[sarken]

Thanks for catching that! We don't want to link to the main collections index, so that link should be under the same <% unless @user || @collection %> conditional as the Open Challenges link.

It wouldn't hurt to add a new "Subcollections" item:

<% if @collection %>
  <li><%= span_if_current ts("Subcollections"), collection_collections_path(@collection) %></li>
<% end %>

Much like "Collections" on the user's collections index, it will never actually appear as a link, but it will serve as a breadcrumb to remind us where we are.

The best place for it would probably be above the <% if @user %> conditional that's currently on line 34.

[Bilka2]

Could you please reenable Layout/CommentIndentation? I think it may have been misbehaving due to the extra space between <% and #, it should work fine generally.