Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

AO3-1870 Add mass unassignment of fandoms in user wrangling page #3168

Open
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

hatal175 commented Nov 18, 2017

Issue

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

Purpose

Adding a mass unassignment option to user tag wrangling page.

Testing

I've added a few lines to a feature test. Also I'd appreciate an opinion on how the interface change looks.

Credit

Tal Hayon

Please use he.

- assignment.destroy
- redirect_to tag_wranglers_path(media_id: params[:media_id], fandom_string: params[:fandom_string], wrangler_id: params[:wrangler_id])
+ assignments = WranglingAssignment.where(user_id: wrangler.id, fandom_id: params[:fandom_ids])
+ assignments.each { |f| f.destroy }
@houndci-bot

houndci-bot Nov 18, 2017

Pass &:destroy as an argument to each instead of a block.

Owner

sarken commented Nov 20, 2017

Just adding a screenshot for reference
unassign

Owner

sarken commented Nov 21, 2017

Thanks for looking at this! I've spoken to the Tag Wrangling chairs, and they have a couple of requests:

First, they'd like validation that makes it so only an admin or the wrangler who owns the page (i.e. testy if it's ao3.org/tag_wranglers/testy) can deassign fandoms.

Second, they'd like a confirmation step. For users with JavaScript enabled, that can be as simple as a confirmation dialogue asking, "Are you sure you want to deassign these fandoms?" For a non-JavaScript fallback, it should be something like the confirmation page for deleting multiple works:

<!--Descriptive page name, messages and instructions-->
<h2 class="heading"><%= ts("Unassign Fandoms") %></h2>

<p class="caution">
  You are about to unassign the following fandoms from [wrangler's name]:
</p>

[list of fandoms]

  <ul class="actions">
    <li><%= submit_tag ts("Yes, Unassign Fandoms") %></li>
    <li><%= link_to ts("Cancel"), tag_wrangler_path(wrangler) %></li>
  </ul>

<% end %>
<!--/content-->

Would you be able to make these changes?

Contributor

hatal175 commented Nov 21, 2017

Contributor

hatal175 commented Nov 24, 2017

Does this limitation : "validation that makes it so only an admin or the wrangler who owns the page can deassign fandoms." apply to the wranglers page as well? Or should wranglers still be able to unassign fandoms from other wranglers from there?

Owner

sarken commented Nov 25, 2017

@@ -1,6 +1,8 @@
class TagWranglersController < ApplicationController
before_action :check_user_status
- before_action :check_permission_to_wrangle
+ before_action :check_permission_to_wrangle, except: [:destroy, :confirm_delete]
@houndci-bot

houndci-bot Nov 25, 2017

Use %i or %I for an array of symbols.

- before_action :check_permission_to_wrangle
+ before_action :check_permission_to_wrangle, except: [:destroy, :confirm_delete]
+ before_action :load_wrangler, except: [:index]
+ before_action :load_assignments, only: [:destroy, :confirm_delete]
@houndci-bot

houndci-bot Nov 25, 2017

Use %i or %I for an array of symbols.

+ @fandoms = @assignments.map(&:fandom)
+ end
+
+ def confirm_delete
@houndci-bot

houndci-bot Nov 25, 2017

Put empty method definitions on a single line.

- assignment.destroy
- redirect_to tag_wranglers_path(media_id: params[:media_id], fandom_string: params[:fandom_string], wrangler_id: params[:wrangler_id])
+ if request.format == "html" && params[:commit] == 'Unassign'
+ render 'confirm_delete' and return
@houndci-bot

houndci-bot Nov 25, 2017

Use && instead of and.

before_action :check_user_status
- before_action :check_permission_to_wrangle
+ before_action :check_permission_to_wrangle, except: [:destroy, :confirm_delete]
@houndci-bot

houndci-bot Nov 25, 2017

Use %i or %I for an array of symbols.

hatal175 added some commits Dec 1, 2017

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