From 687a63ad1d9ffee138a892ed4d6b49f15772804f Mon Sep 17 00:00:00 2001 From: astolat Date: Sat, 8 Mar 2014 22:24:03 -0500 Subject: [PATCH 1/8] Fixes for assignments crashing on invalid signups https://code.google.com/p/otwarchive/issues/detail?id=3815 --- .../challenge_signups_controller.rb | 5 +- app/mailers/user_mailer.rb | 9 +++ app/models/potential_match.rb | 33 ++++++--- .../invalid_signup_notification.html.erb | 17 +++++ .../invalid_signup_notification.text.erb | 16 ++++ .../challenge_giftexchange.feature | 73 +++++++++---------- .../challege_gift_exchange_steps.rb | 18 +++++ public/help/challenge-matching.html | 11 +++ 8 files changed, 129 insertions(+), 53 deletions(-) create mode 100644 app/views/user_mailer/invalid_signup_notification.html.erb create mode 100644 app/views/user_mailer/invalid_signup_notification.text.erb diff --git a/app/controllers/challenge_signups_controller.rb b/app/controllers/challenge_signups_controller.rb index 235a11ea38e..02f332ff5f3 100644 --- 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 signup is invalid. Please check your signups 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 index 2a493633519..e4927bd4dac 100644 --- 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 Signups 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 index bf525c35f7c..5c91d36d6e6 100644 --- 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 new file mode 100644 index 00000000000..54425f07f4f --- /dev/null +++ b/app/views/user_mailer/invalid_signup_notification.html.erb @@ -0,0 +1,17 @@ +<% content_for :message do %> +

Dear Collection Maintainers!

+ +

We have found some invalid signups in your challenge <%= link_to @collection.title, collection_url(@collection) %>. Potential matches cannot be generated until these are cleaned up.

+ +

Invalid signups 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.

+ +

Here are the invalid signups: +

+

+ + <% content_for :sent_at do %><%= Time.now.to_s(:time_for_mailers) %><% end %> +<% end %> \ No newline at end of file diff --git a/app/views/user_mailer/invalid_signup_notification.text.erb b/app/views/user_mailer/invalid_signup_notification.text.erb new file mode 100644 index 00000000000..730d3b80fcf --- /dev/null +++ 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 signups in your challenge "<%= @collection.title %>" (<%= collection_url(@collection) %>). Potential matches cannot be generated until these are cleaned up. + +Invalid signups 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 signups: +<% @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 %> \ No newline at end of file diff --git a/features/gift_exchanges/challenge_giftexchange.feature b/features/gift_exchanges/challenge_giftexchange.feature index 226ef2ce454..93f257450ea 100644 --- a/features/gift_exchanges/challenge_giftexchange.feature +++ b/features/gift_exchanges/challenge_giftexchange.feature @@ -72,22 +72,21 @@ 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 "WARNING" + And I should see "signup 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 +98,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 +128,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 +137,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 signup" + 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 +197,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 index f1a92255660..2eb8124db03 100644 --- 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 index 694db930f38..b0be2cc62d1 100644 --- a/public/help/challenge-matching.html +++ b/public/help/challenge-matching.html @@ -13,6 +13,17 @@

Matching

Potential Match Issues

+

Invalid Signups

+ +

Sometimes a challenge can end up with one or more invalid signups -- duplicates, or signups 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 signup process -- read further for details about this.

+ +

If this happens, potential matching won't run, and you'll be quickly emailed a list of links to the invalid signups so you can examine them. You can often tell easily when two signups 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 signups or prompts they want to use.

+ +

If on the other hand you made the signup rules more strict mid-stream and you have a large number of invalid signups 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 signup, you may have a whole slew of signups 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 requiring 2/2, while allowing 3/3.

+ +

After you have dealt with all the invalid signups, you can then start matching again.

+

No Potential Recipients/No Potential Givers

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 From 68c5ae4b70f3fab5fc972000374c3e132fd85548 Mon Sep 17 00:00:00 2001 From: astolat Date: Sun, 9 Mar 2014 21:34:49 -0400 Subject: [PATCH 2/8] Update challenge_giftexchange.feature Argh, tweaked the warning text and forgot to edit the test. :P --- features/gift_exchanges/challenge_giftexchange.feature | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/features/gift_exchanges/challenge_giftexchange.feature b/features/gift_exchanges/challenge_giftexchange.feature index 93f257450ea..7080b9f154d 100644 --- a/features/gift_exchanges/challenge_giftexchange.feature +++ b/features/gift_exchanges/challenge_giftexchange.feature @@ -79,8 +79,7 @@ Feature: Gift Exchange Challenge # 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 "WARNING" - And I should see "signup is invalid" + Then I should see "signup is invalid" Scenario: Optional tags should be saved when editing a signup (gcode issue #2729) Given the gift exchange "Awesome Gift Exchange" is ready for signups From 84af841b5571235ab859f09af80112248d0cf2ff Mon Sep 17 00:00:00 2001 From: astolat Date: Sun, 9 Mar 2014 21:53:41 -0400 Subject: [PATCH 3/8] Fixing signup -> sign-up in user-facing text --- app/controllers/challenge_signups_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/challenge_signups_controller.rb b/app/controllers/challenge_signups_controller.rb index 02f332ff5f3..7882a71297e 100644 --- a/app/controllers/challenge_signups_controller.rb +++ b/app/controllers/challenge_signups_controller.rb @@ -129,7 +129,7 @@ def summary def show unless @challenge_signup.valid? - flash[:error] = ts("This signup is invalid. Please check your signups for a duplicate or edit to fix any other problems.") + 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 From 337c7833b9302c4c4195992d157c989355d05b98 Mon Sep 17 00:00:00 2001 From: astolat Date: Sun, 9 Mar 2014 21:54:35 -0400 Subject: [PATCH 4/8] Signups -> sign-ups in mailer subject line --- app/mailers/user_mailer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index e4927bd4dac..1086a051710 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -144,7 +144,7 @@ def invalid_signup_notification(collection_id, invalid_signup_ids) @invalid_signups = invalid_signup_ids mail( :to => @collection.get_maintainers_email, - :subject => "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Invalid Signups Found" + :subject => "[#{ArchiveConfig.APP_SHORT_NAME}][#{@collection.title}] Invalid Sign-ups Found" ) end From 15e87be88caecd4c5f5d38f8caf2256252362ea3 Mon Sep 17 00:00:00 2001 From: astolat Date: Sun, 9 Mar 2014 21:56:04 -0400 Subject: [PATCH 5/8] Signups -> sign-ups in message content --- .../user_mailer/invalid_signup_notification.html.erb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/user_mailer/invalid_signup_notification.html.erb b/app/views/user_mailer/invalid_signup_notification.html.erb index 54425f07f4f..607421e7e9b 100644 --- a/app/views/user_mailer/invalid_signup_notification.html.erb +++ b/app/views/user_mailer/invalid_signup_notification.html.erb @@ -1,11 +1,11 @@ <% content_for :message do %>

Dear Collection Maintainers!

-

We have found some invalid signups in your challenge <%= link_to @collection.title, collection_url(@collection) %>. Potential matches cannot be generated until these are cleaned up.

+

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.

-

Invalid signups 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.

+

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.

-

Here are the invalid signups: +

Here are the invalid sign-ups: