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

AO3-6761 Check admin roles in TagWranglingsController #4937

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

brianjaustin
Copy link
Member

@brianjaustin brianjaustin commented Oct 4, 2024

Pull Request Checklist

Issue

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

Purpose

Restrict access to PAC (tag bins only) and wrangling/super admins (wrangling tools, mass update)

References

This is probably going to cause conflicts with #4917; please merge that one first if this is in the same release, then I will fix the conflicts.

@brianjaustin
Copy link
Member Author

I noticed that "Wrangling Tools" in the left-hand navigation menu is selected, even when on mass wranglings page:
wrangling navigation menu with "Wrangling Tools" and "Fandoms by media (0)" selected

Do we want to hide the 'Wrangling Tools" item (which is not selectable) for any admin who does not have access to the general wrangling tools page?

@sarken
Copy link
Collaborator

sarken commented Oct 4, 2024

Huh, that's weird. Just to clarify because I can't pull the code right now: if you're on, e.g., https://test.archiveofourown.org/tag_wranglings?show=fandoms with this new code, "Wrangling Tools" is is a span instead of a link? Is this for all admins or just admins who can't access Wrangling Tools?

Regardless, I think the fix for that might be changing the link in the sidebar to explicitly check that the params are blank.

<li><%= span_if_current(ts('Wrangling Tools'), tag_wranglings_path) %></li>

 <li><%= span_if_current(ts('Wrangling Tools'), tag_wranglings_path, params[:show].blank?) %></li> 

(It's possible there's more than one version of this sidebar.)

@brianjaustin
Copy link
Member Author

@sarken that's actually the case now, not just with the new code... 😬 I don't object to fixing it here, but the main question I had was if we should hide that part of the menu for non-write admins (iirc the ticket doesn't say either way)

@sarken
Copy link
Collaborator

sarken commented Oct 8, 2024

Ah, okay! I thought it was a new bug, and you were asking if the way to deal with it was to hide that particular link while leaving the others visible.

I think we can leave the full sidebar visible for anyone with either type of access and just stick to this plan from AO3-6758 for everyone else:

The final pull request will need to hide the major navigational elements:

@sarken
Copy link
Collaborator

sarken commented Oct 8, 2024

(Feel free to fix or make a new issue for the span_if_current thing as desired!)

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Should there be a test for the navbar visibility?

I think it would be nice if you could i18n the wrangler_dashboard, none of the other pull requests touched it so hopefully it shouldn't lead to merge conflicts.

app/views/admin/_header.html.erb Outdated Show resolved Hide resolved
spec/controllers/tag_wranglings_controller_spec.rb Outdated Show resolved Hide resolved
spec/controllers/tag_wranglings_controller_spec.rb Outdated Show resolved Hide resolved
app/views/tag_wranglings/_wrangler_dashboard.html.erb Outdated Show resolved Hide resolved
app/controllers/tag_wranglings_controller.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Thank you!

<ul class="navigation actions">
<% if current_user.is_a?(User) %>
<li><%= span_if_current(ts('Wrangling Home'), tag_wrangler_path(current_user)) %></li>
<% if (logged_in_as_admin? && policy(:wrangling).full_access?) || current_user.try(:is_tag_wrangler?) || @counts %>
Copy link
Contributor

@Bilka2 Bilka2 Nov 10, 2024

Choose a reason for hiding this comment

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

🤔 I thought this needed to be read_access? as well but the tests pass without it? Maybe still good to change though so it's not confusing?

Copy link
Contributor

@Bilka2 Bilka2 Nov 10, 2024

Choose a reason for hiding this comment

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

I see, it doesn't show on the tags index page, because that one doesn't have @counts set. So this should be changed and the tests should all start out in the tags index, not the tag wrangling tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants