Merge pull request #1567 from shalott/issue_3815_assignments_hanging

Fixes for assignments crashing on invalid signups (issue 3815)
otwcode · Mar 15, 2014 · 7c5d430 · 7c5d430
2 parents cdfec9f + c6a73bf
commit 7c5d430
Show file tree

Hide file tree

Showing 8 changed files with 128 additions and 53 deletions.
diff --git a/app/controllers/challenge_signups_controller.rb b/app/controllers/challenge_signups_controller.rb
@@ -127,7 +127,10 @@ def summary
     end
   end
 
-  def show
+  def show    
+    unless @challenge_signup.valid?
+      flash[:error] = ts("This sign-up is invalid. Please check your sign-ups for a duplicate or edit to fix any other problems.")
+    end
   end
 
   protected

diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb
@@ -138,6 +138,15 @@ def collection_notification(collection_id, subject, message)
       :subject => "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] #{subject}"
     )
   end
+
+  def invalid_signup_notification(collection_id, invalid_signup_ids)
+    @collection = Collection.find(collection_id)
+    @invalid_signups = invalid_signup_ids
+    mail(
+      :to => @collection.get_maintainers_email,
+      :subject => "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Invalid Sign-ups Found"
+    )
+  end
 
   def potential_match_generation_notification(collection_id)
     @collection = Collection.find(collection_id)

diff --git a/app/models/potential_match.rb b/app/models/potential_match.rb
@@ -65,22 +65,31 @@ def self.regenerate_for_signup(signup)
   # The actual method that generates the potential matches for an entire collection
   def self.generate_in_background(collection_id)
     collection = Collection.find(collection_id)
-    PotentialMatch.clear!(collection)
-    settings = collection.challenge.potential_match_settings
+
+    # First, make sure there are no invalid signups
+    invalid_signup_ids = collection.signups.select {|s| !s.valid?}.collect(&:id)
+    unless invalid_signup_ids.empty?
+      UserMailer.invalid_signup_notification(collection.id, invalid_signup_ids).deliver
+      PotentialMatch.cancel_generation(collection)
+    else
+
+      PotentialMatch.clear!(collection)
+      settings = collection.challenge.potential_match_settings
 
-    # start by collecting the ids of all the tag sets of the offers/requests in this collection
-    collection_tag_sets = Prompt.where(:collection_id => collection.id).value_of(:tag_set_id, :optional_tag_set_id).flatten.compact
+      # start by collecting the ids of all the tag sets of the offers/requests in this collection
+      collection_tag_sets = Prompt.where(:collection_id => collection.id).value_of(:tag_set_id, :optional_tag_set_id).flatten.compact
 
-    # the topmost tags required for matching
-    required_types = settings.required_types.map {|t| t.classify}
+      # the topmost tags required for matching
+      required_types = settings.required_types.map {|t| t.classify}
+
+      # treat each signup as a request signup first
+      collection.signups.find_each do |signup|
+        break if PotentialMatch.canceled?(collection)
+        $redis.set progress_key(collection), signup.pseud.byline
+        PotentialMatch.generate_for_signup(collection, signup, settings, collection_tag_sets, required_types)
+      end
 
-    # treat each signup as a request signup first
-    collection.signups.find_each do |signup|
-      break if PotentialMatch.canceled?(collection)
-      $redis.set progress_key(collection), signup.pseud.byline
-      PotentialMatch.generate_for_signup(collection, signup, settings, collection_tag_sets, required_types)
     end
-
     # TODO: for any signups with no potential matches try regenerating?
 
     PotentialMatch.finish_generation(collection)

diff --git a/app/views/user_mailer/invalid_signup_notification.html.erb b/app/views/user_mailer/invalid_signup_notification.html.erb
@@ -0,0 +1,17 @@
+<% content_for :message do %>
+  <p>Dear Collection Maintainers!</p>
+
+  <p>We have found some invalid sign-ups in your challenge <%= link_to @collection.title, collection_url(@collection) %>. Potential matches cannot be generated until these are cleaned up. </p>
+
+  <p>Invalid sign-ups are often duplicates, or don't meet the requirements you've set for your challenge. Unfortunately, there is no automatic way to fix them, so you will have to manually edit or delete them. For more details, see <a href="<%= root_url %>help/challenge_matching.html#invalid_signups">the challenge matching help</a>. </p> 
+
+  <p>Here are the invalid sign-ups:
+    <ul>
+    <% @invalid_signups.each do |signup_id| %>
+      <li><%= collection_signup_url(@collection, signup_id) %></li>
+    <% end %>
+    </ul>
+  </p>
+
+  <% content_for :sent_at do %><%= Time.now.to_s(:time_for_mailers) %><% end %>
+<% end %>
diff --git a/app/views/user_mailer/invalid_signup_notification.text.erb b/app/views/user_mailer/invalid_signup_notification.text.erb
@@ -0,0 +1,16 @@
+<% content_for :message do %>
+Dear Collection Maintainers!
+
+We have found some invalid sign-ups in your challenge "<%= @collection.title %>" (<%= collection_url(@collection) %>). Potential matches cannot be generated until these are cleaned up. 
+
+Invalid sign-ups are often duplicates, or don't meet the requirements you've set for your challenge. Unfortunately, there is no automatic way to fix them, so you will have to manually edit or delete them. For more details, see the challenge-matching help file at <%= root_url %>help/challenge_matching.html#invalid_signups.
+
+Here are the invalid sign-ups:
+<% @invalid_signups.each do |signup_id| %>
+  <%= collection_signup_url(@collection, signup_id) %>
+<% end %>
+
+AO3 (<%= root_url %>)
+
+<% content_for :sent_at do %><%= Time.now.to_s(:time_for_mailers) %><% end %>
+<% end %>
diff --git a/features/gift_exchanges/challenge_giftexchange.feature b/features/gift_exchanges/challenge_giftexchange.feature
@@ -72,22 +72,20 @@ Feature: Gift Exchange Challenge
   Then I should see "New members invited: comod"
 
   Scenario: Sign up for a gift exchange
-
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
-  When I am logged in as "myname1"
+  Given the gift exchange "Awesome Gift Exchange" is ready for signups
+    And I am logged in as "myname1"
   When I sign up for "Awesome Gift Exchange" with combination A
   Then I should see "Sign-up was successfully created"
-
+  # Invalid signup should warn the user
+  When I create an invalid signup in the gift exchange "Awesome Gift Exchange"
+    And I reload the page
+  Then I should see "sign-up is invalid"  
 
   Scenario: Optional tags should be saved when editing a signup (gcode issue #2729)
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for signups
     And I edit settings for "Awesome Gift Exchange" challenge
     And I check "Optional Tags?"
     And I submit
-    And I open signups for "Awesome Gift Exchange"
   When I am logged in as "myname1"
     And I sign up for "Awesome Gift Exchange" with combination A
     And I follow "Edit Sign-up"
@@ -99,31 +97,24 @@ Feature: Gift Exchange Challenge
   Then I should see "Something else weird"
 
   Scenario: Sign-ups can be seen in the dashboard
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for signups
   When I am logged in as "myname1"
-  When I sign up for "Awesome Gift Exchange" with combination A
+    And I sign up for "Awesome Gift Exchange" with combination A
   When I am on my user page
   Then I should see "Sign-ups (1)"
   When I follow "Sign-ups (1)"
   Then I should see "Awesome Gift Exchange"
 
   Scenario: Ordinary users cannot see other signups
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
-  When I am logged in as "myname1"
+  Given the gift exchange "Awesome Gift Exchange" is ready for signups
+    And I am logged in as "myname1"
   When I sign up for "Awesome Gift Exchange" with combination A
-  When I go to the collections page
+    And I go to the collections page
     And I follow "Awesome Gift Exchange"
   Then I should not see "Sign-ups" within "#dashboard"
 
   Scenario: Mod can view signups
-
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for signups
     And everyone has signed up for the gift exchange "Awesome Gift Exchange"
   When I am logged in as "mod1"
     And I go to "Awesome Gift Exchange" collection's page
@@ -136,10 +127,7 @@ Feature: Gift Exchange Challenge
     And I should see "Alternate Universe - Historical"
 
   Scenario: Cannot generate matches while signup is open
-
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for signups
     And everyone has signed up for the gift exchange "Awesome Gift Exchange"
   When I am logged in as "mod1"
     And I go to "Awesome Gift Exchange" collection's page
@@ -148,24 +136,31 @@ Feature: Gift Exchange Challenge
     And I should not see "Generate Potential Matches"
 
   Scenario: Matches can be generated
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
-    And everyone has signed up for the gift exchange "Awesome Gift Exchange"
-  When I close signups for "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for matching
+    And I close signups for "Awesome Gift Exchange"
   When I follow "Matching"
-  When I follow "Generate Potential Matches"
+    And I follow "Generate Potential Matches"
   Then I should see "Beginning generation of potential matches. This may take some time, especially if your challenge is large."
   Given the system processes jobs
     And I wait 3 seconds
   When I reload the page
   Then I should see "Main Assignments"
 
+  Scenario: Invalid signups are caught before generation
+  Given the gift exchange "Awesome Gift Exchange" is ready for matching
+    And I create an invalid signup in the gift exchange "Awesome Gift Exchange"
+  When I close signups for "Awesome Gift Exchange"
+    And I follow "Matching"
+    And I follow "Generate Potential Matches"
+    And the system processes jobs
+    And I wait 3 seconds
+  Then 1 email should be delivered to "mod1"
+    And the email should contain "invalid sign-up"
+  When I reload the page
+    Then I should see "Generate Potential Matches"
+
   Scenario: Matches can be regenerated for a single signup
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
-    And everyone has signed up for the gift exchange "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for matching
     And I am logged in as "Mismatch"
     And I sign up for "Awesome Gift Exchange" with a mismatched combination
   When I am logged in as "mod1"
@@ -201,10 +196,7 @@ Feature: Gift Exchange Challenge
 
   Scenario: Assignments can be sent
 
-  Given I am logged in as "mod1"
-    And I have created the gift exchange "Awesome Gift Exchange"
-    And I open signups for "Awesome Gift Exchange"
-    And everyone has signed up for the gift exchange "Awesome Gift Exchange"
+  Given the gift exchange "Awesome Gift Exchange" is ready for matching
     And I have generated matches for "Awesome Gift Exchange"
   When I follow "Send Assignments"
   Then I should see "Assignments are now being sent out"

diff --git a/features/step_definitions/challege_gift_exchange_steps.rb b/features/step_definitions/challege_gift_exchange_steps.rb
@@ -62,6 +62,12 @@
   step %{I should see "(Open, Unmoderated, Gift Exchange Challenge)"}
 end
 
+Given /^the gift exchange "([^\"]*)" is ready for signups$/ do |title|
+  step %{I am logged in as "mod1"}
+  step %{I have created the gift exchange "Awesome Gift Exchange"}
+  step %{I open signups for "Awesome Gift Exchange"}
+end
+
 ## Signing up
 
 When /^I sign up for "([^\"]*)" with combination A$/ do |title|
@@ -137,6 +143,18 @@
 
 ## Matching
 
+Given /^the gift exchange "([^\"]*)" is ready for matching$/ do |title|
+  step %{the gift exchange "#{title}" is ready for signups}
+  step %{everyone has signed up for the gift exchange "#{title}"}
+end
+
+Given /^I create an invalid signup in the gift exchange "([^\"]*)"$/ do |challengename|
+  collection = Collection.find_by_title(challengename)
+  # create an invalid signup by deleting the first one's offers,
+  # bypassing the validation checks
+  collection.signups.first.offers.delete_all
+end  
+
 Given /^everyone has signed up for the gift exchange "([^\"]*)"$/ do |challengename|
   step %{I am logged in as "myname1"}
   step %{I sign up for "#{challengename}" with combination A}

diff --git a/public/help/challenge-matching.html b/public/help/challenge-matching.html
@@ -13,6 +13,17 @@ <h2>Matching</h2>
 
 <h3>Potential Match Issues</h3>
 
+<h4 id="invalid_signups">Invalid Sign-ups</h4>
+
+<p>Sometimes a challenge can end up with one or more invalid sign-ups -- duplicates, or sign-ups that have too many or too few offers/requests or ones that don't fit the rules of your challenge. Most commonly, this happens if someone clicks submit or update multiple times because their connection to the archive is slow. It can also happen if you have changed the rules of 
+your challenge partway through the sign-up process -- read further for details about this.</p>
+
+<p>If this happens, potential matching won't run, and you'll be quickly emailed a list of links to the invalid sign-ups so you can examine them. You can often tell easily when two sign-ups or offers/requests are duplicates, in which case you can just delete the duplicates. Sometimes you may have to contact a participant to figure out which of the sign-ups or prompts they want to use.</p>
+
+<p>If on the other hand you made the rules more strict mid-stream and you have a large number of invalid sign-ups as a result, you may only be able to fix this by easing your rules to match the lowest common denominator. For instance, if you started by requiring 2 offers and 2 requests, and then upped the requirement to 3 offers and 3 requests a day into sign-up, you may have a whole slew of sign-ups that only have 2/2 and are now therefore considered invalid. To fix this, you'll have to change your challenge settings back to only <em>requiring</em> 2/2, while <em>allowing</em> 3/3.</p>
+
+<p>After you have dealt with all the invalid sign-ups, you can then start matching again.</p>
+
 <h4>No Potential Recipients/No Potential Givers</h4>
 
 <p>If someone has no potential recipients, that means no one wants what they are offering. There is no way to fix this unless the person agrees to