AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines #4398

Cesium-Ice · 2022-11-25T22:40:36Z

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-5520

Purpose

What does this PR do?

Testing Instructions

How can the Archive's QA team verify that this is working as you intended?

If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.

References

Are there other relevant issues/pull requests/mailing list discussions?

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

Cesium-Ice, they/them
If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.

Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.

spec/controllers/wrangling_guidelines_controller_spec.rb

…dd-or-modify-wrangling-guidelines

sarken · 2023-06-10T05:39:26Z

app/views/admin/_header.html.erb

@@ -58,7 +60,9 @@
      </ul>
    </li>
  <% end %>
-  <li><%= link_to ts("Tag Wrangling", key: "header"), tag_wranglings_path %></li>
+  <% if policy(WranglingGuideline).new? %>


We're not actually restricting Tag Wrangling in this PR, so let's leave the Tag Wrangling link as-is.

sarken · 2023-06-10T05:48:45Z

app/controllers/wrangling_guidelines_controller.rb

@@ -14,21 +14,25 @@ def show
  # GET /wrangling_guidelines/new
  def new
    @wrangling_guideline = WranglingGuideline.new
+    authorize @wrangling_guideline


Anywhere we're doing @wrangling_guideline = WranglingGuideline.new or @wrangling_guideline = WranglingGuideline.find, we should be able to just stick authorize in front of WranglingGuideline to make things a bit neater.

sarken · 2023-06-10T05:51:59Z

app/views/wrangling_guidelines/show.html.erb

@@ -8,7 +8,7 @@

 <!--main content-->
 <div class="admin" role="article">
-  <% if logged_in_as_admin? %>
+  <% if policy(WranglingGuideline).new? %>


Since we're deciding whether to display the edit link, let's use policy(WranglingGuideline).edit? instead.

Cesium-Ice · 2024-05-21T09:30:05Z

Is there some way I can run the rubocop check locally?

Bilka2 · 2024-05-21T09:33:47Z

Is there some way I can run the rubocop check locally?

Yes, see https://github.com/otwcode/otwarchive/wiki/Coding-Standards#user-content-Previewing_Reviewdog_comments. If you're using Docker, I would recommend running the commands similarly to the tests, with docker compose run: docker compose run --rm test bundle exec rubocop app/controllers/wrangling_guidelines_controller.rb

Cesium-Ice · 2024-05-21T17:23:25Z

When I fix the error with rubocop by replacing instances of ts with t, the wrangling_guidelines_controller_spec.rb unit tests fails with errors like so:

8) WranglingGuidelinesController DELETE #destroy when logged in as an admin with superadmin role deletes and redirects to index
     Failure/Error: expect(flash[:notice]).to eq notice
     
       expected: "Wrangling Guideline was successfully deleted."
            got: "translation missing: Wrangling Guideline was successfully deleted."

Does this mean instead of having instances of t("insert string here") I need to move "insert string here" into en.yml?

Bilka2 · 2024-05-21T17:53:52Z

Yes, as you said you need to move the string into the yml file and use a key in the t() call. You can find more information on the i18n standards wiki page. You may find the i18n-tasks section useful for some commands to find if you missed any translations - it would also flag this issue.

Bilka2

I'm not sarken, but I had a look over the PR and it looks good. Thank you!

sarken · 2024-07-18T05:46:23Z

Sorry for the delay! We were discussing how we wanted to approach admin roles for wrangling areas, and we determined the best way to do it would be to put everything -- including Wrangling Guidelines -- under one WranglingPolicy as described in AO3-6758.

Obviously, that would require significant changes to this pull request. Is that something you'd be interested in working on? If not, that's fine! You've already spent a lot of time on this and I bet you're sick of it -- someone else would be happy to take over (and give you credit for all the work you did, of course). There's no rush to actually update this, but if you could let us know within the next 14 days if you'd like to keep working on this or would prefer someone else adopt it, that would be great.

Thank you!

Cesium-Ice · 2024-07-25T20:50:41Z

I’m good with working on it

brianjaustin · 2024-08-02T17:19:46Z

Hi @Cesium-Ice! I started working on another issue in this same epic, with the WranglingPolicy: #4885. Feel free to pull that in and modify as needed if helpful! (Otherwise I'm happy to resolve merge conflicts later if you already have something in progress.)

Bilka2 · 2024-08-08T14:14:10Z

This is was missing from the Jira issue, but in app/views/admin/_admin_nav.html.erb there should be a check for the policy for showing the wrangling guidelines button, similar to the check in the admin header partial.

…dd-or-modify-wrangling-guidelines

…glingGuidelinePolicy

Bilka2

Thanks for updating this, there are just some minor issues remaining.

Bilka2 · 2024-09-12T19:25:01Z

app/views/admin/_admin_nav.html.erb

@@ -9,7 +9,7 @@
 </li>
  <% if params[:controller] == "admin_posts" && params[:action] == "edit" %>
    <li>
-      <%= link_to t("admin.admin_nav.delete", default: "Delete Post"),
+      <%= link_to t("admin.admin_nav.delete"),


Suggestion to match #4891:

Suggested change

<%= link_to t("admin.admin_nav.delete"),

<%= link_to t(".news.delete_post"),

Bilka2 · 2024-09-12T19:33:10Z

app/controllers/wrangling_guidelines_controller.rb

+  include WranglingHelper
+


Is anything from the helper used in this file?

Bilka2 · 2024-09-12T19:34:24Z

app/controllers/wrangling_guidelines_controller.rb

@@ -13,57 +15,64 @@ def show

  # GET /wrangling_guidelines/new
  def new
+    authorize :wrangling if logged_in_as_admin?


All of these actions are admin only (see the before_action), so the logged_in_as_admin? check is redundant. Please remove it here and from the other authorize calls in this controller.

Suggested change

authorize :wrangling if logged_in_as_admin?

authorize :wrangling

Bilka2 · 2024-09-12T19:37:54Z

app/views/admin/_admin_nav.html.erb

-  </li>
+  <% if policy(:wrangling).new? %>
+    <li>
+      <%= span_if_current t("admin.admin_nav.wrangling_guidelines"), wrangling_guidelines_path %>


This can be shortened by using a relative key:

Suggested change

<%= span_if_current t("admin.admin_nav.wrangling_guidelines"), wrangling_guidelines_path %>

<%= span_if_current t(".wrangling_guidelines"), wrangling_guidelines_path %>

Bilka2

Thank you very much!

Cesium-Ice added 3 commits November 25, 2022 13:46

AO3-5520 Admin roles restrict ability to change wrangling guidelines

82a248f

fix features

218ffe0

update specs

50685ce

github-actions bot added the Awaiting Review label Nov 25, 2022

Cesium-Ice commented Nov 25, 2022

View reviewed changes

spec/controllers/wrangling_guidelines_controller_spec.rb Outdated Show resolved Hide resolved

Cesium-Ice commented Nov 25, 2022

View reviewed changes

spec/controllers/wrangling_guidelines_controller_spec.rb Outdated Show resolved Hide resolved

Cesium-Ice added 3 commits November 25, 2022 14:49

please hound

e62258f

please hound 2

3977744

fix cucumber test

e43f067

Cesium-Ice marked this pull request as ready for review November 26, 2022 01:00

Cesium-Ice changed the title ~~Ao3 5520 admin roles restrict ability to add or modify wrangling guidelines~~ AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines Nov 26, 2022

please hound

678733b

Cesium-Ice requested a review from tickinginstant December 7, 2022 07:07

Merge branch 'master' into AO3-5520-admin-roles-restrict-ability-to-a…

ac13808

…dd-or-modify-wrangling-guidelines

sarken requested changes Jun 10, 2023

View reviewed changes

sarken added Reviewed: Action Needed and removed Awaiting Review labels Jun 10, 2023

Cesium-Ice added 2 commits May 20, 2024 16:19

merge from master

52016ec

address sarken comments

5d614f7

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels May 21, 2024

fix rubocop errors

504f4cc

fix rubocop by moving some stuff to localization file

c6da49b

Cesium-Ice requested a review from sarken May 22, 2024 07:50

Bilka2 approved these changes Jul 17, 2024

View reviewed changes

Bilka2 removed the Coder Has Actioned Review label Jul 17, 2024

Bilka2 added the Reviewed: Ready to Merge label Jul 17, 2024

sarken added Reviewed: Action Needed and removed Reviewed: Ready to Merge labels Jul 18, 2024

brianjaustin added the Priority: High label Aug 2, 2024

Merge branch 'master' into AO3-5520-admin-roles-restrict-ability-to-a…

92d3f42

…dd-or-modify-wrangling-guidelines

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Sep 10, 2024

Cesium-Ice added 3 commits September 9, 2024 23:47

put wrangling guideline changes under WranglingPolicy instead of Wran…

a48fef7

…glingGuidelinePolicy

fix wrangling policy variable in erb files

1683ac4

address linter

67faec0

Cesium-Ice requested a review from Bilka2 September 10, 2024 08:27

Bilka2 suggested changes Sep 12, 2024

View reviewed changes

Bilka2 added Reviewed: Action Needed and removed Coder Has Actioned Review labels Sep 12, 2024

address bilka2 comments

0858f9a

github-actions bot added Coder Has Actioned Review and removed Reviewed: Action Needed labels Sep 13, 2024

Bilka2 approved these changes Sep 13, 2024

View reviewed changes

Bilka2 added Reviewed: Ready to Merge and removed Coder Has Actioned Review labels Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines #4398

AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines #4398

Cesium-Ice commented Nov 25, 2022 •

edited

Loading

sarken Jun 10, 2023

sarken Jun 10, 2023

sarken Jun 10, 2023

Cesium-Ice commented May 21, 2024

Bilka2 commented May 21, 2024 •

edited

Loading

Cesium-Ice commented May 21, 2024 •

edited

Loading

Bilka2 commented May 21, 2024

Bilka2 left a comment

sarken commented Jul 18, 2024 •

edited by jira bot

Loading

Cesium-Ice commented Jul 25, 2024

brianjaustin commented Aug 2, 2024

Bilka2 commented Aug 8, 2024 •

edited

Loading

Bilka2 left a comment

Bilka2 Sep 12, 2024

Bilka2 Sep 12, 2024

Bilka2 Sep 12, 2024

Bilka2 Sep 12, 2024

Bilka2 left a comment

	<%= link_to t("admin.admin_nav.delete"),
	<%= link_to t(".news.delete_post"),

	authorize :wrangling if logged_in_as_admin?
	authorize :wrangling

	<%= span_if_current t("admin.admin_nav.wrangling_guidelines"), wrangling_guidelines_path %>
	<%= span_if_current t(".wrangling_guidelines"), wrangling_guidelines_path %>

AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines #4398

Are you sure you want to change the base?

AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines #4398

Conversation

Cesium-Ice commented Nov 25, 2022 • edited Loading

Pull Request Checklist

Issue

Purpose

Testing Instructions

References

Credit

sarken Jun 10, 2023

Choose a reason for hiding this comment

sarken Jun 10, 2023

Choose a reason for hiding this comment

sarken Jun 10, 2023

Choose a reason for hiding this comment

Cesium-Ice commented May 21, 2024

Bilka2 commented May 21, 2024 • edited Loading

Cesium-Ice commented May 21, 2024 • edited Loading

Bilka2 commented May 21, 2024

Bilka2 left a comment

Choose a reason for hiding this comment

sarken commented Jul 18, 2024 • edited by jira bot Loading

Cesium-Ice commented Jul 25, 2024

brianjaustin commented Aug 2, 2024

Bilka2 commented Aug 8, 2024 • edited Loading

Bilka2 left a comment

Choose a reason for hiding this comment

Bilka2 Sep 12, 2024

Choose a reason for hiding this comment

Bilka2 Sep 12, 2024

Choose a reason for hiding this comment

Bilka2 Sep 12, 2024

Choose a reason for hiding this comment

Bilka2 Sep 12, 2024

Choose a reason for hiding this comment

Bilka2 left a comment

Choose a reason for hiding this comment

Cesium-Ice commented Nov 25, 2022 •

edited

Loading

Bilka2 commented May 21, 2024 •

edited

Loading

Cesium-Ice commented May 21, 2024 •

edited

Loading

sarken commented Jul 18, 2024 •

edited by jira bot

Loading

Bilka2 commented Aug 8, 2024 •

edited

Loading