Repository: dallinger/dallinger
Issue: Over-recruited MTurk participants are not returned to MTurk to submit HIT

See #1299 

## Expected Behavior
Over-recruited MTurk workers should be prompted to return to MTurk to submit their HIT, just like workers who complete the experiment.

## Current Behavior
Over-recruited workers are shown the initial /ad page again, inviting them to accept the same HIT again. The participant is free to click on the experiment again, and then the experiment gets stuck at 0% in the waiting room.

## Notes
I think the /ad route needs to check for the `overrecruited` status in addition to the `working` status when deciding between the `ad.html` and `thanks.html` templates. Also need to check if overrecruits have a non-null `end_time`.

Reported by @mongates 


Changed files:
dallinger/experiment_server/experiment_server.py, tests/test_experiment_server.py, dallinger/experiment_server/utils.py

diff --git a/dallinger/experiment_server/experiment_server.py b/dallinger/experiment_server/experiment_server.py
index 0a5c0879..bd7005c2 100644
--- a/dallinger/experiment_server/experiment_server.py
+++ b/dallinger/experiment_server/experiment_server.py
@@ -9,7 +9,6 @@ from operator import attrgetter
 import os
 import re
 import sys
-import user_agents
 
 from flask import (
     abort,
@@ -42,6 +41,8 @@ from dallinger.heroku.messages import HITSummary
 from .replay import ReplayBackend
 from .worker_events import WorkerEvent
 from .utils import nocache
+from .utils import ValidatesBrowser
+
 
 # Initialize the Dallinger database.
 session = db.session
@@ -479,6 +480,20 @@ def launch():
     return success_response(recruitment_msg=message)
 
 
+def should_show_thanks_page_to(participant):
+    """In the context of the /ad route, should the participant be shown
+    the thanks.html page instead of ad.html?
+    """
+    if participant is None:
+        return False
+    status = participant.status
+    marked_done = participant.end_time is not None
+    ready_for_external_submission = status in ('overrecruited', 'working') and marked_done
+    assignment_complete = status in ('submitted', 'approved')
+
+    return assignment_complete or ready_for_external_submission
+
+
 @app.route('/ad', methods=['GET'])
 @nocache
 def advertisement():
@@ -496,24 +511,11 @@ def advertisement():
     if not ('hitId' in request.args and 'assignmentId' in request.args):
         raise ExperimentError('hit_assign_worker_id_not_set_in_mturk')
 
-    # Browser rule validation, if configured:
-    user_agent_string = request.user_agent.string
-    user_agent_obj = user_agents.parse(user_agent_string)
-    browser_ok = True
     config = _config()
-    for rule in config.get('browser_exclude_rule', '').split(','):
-        myrule = rule.strip()
-        if myrule in ["mobile", "tablet", "touchcapable", "pc", "bot"]:
-            if (myrule == "mobile" and user_agent_obj.is_mobile) or\\
-               (myrule == "tablet" and user_agent_obj.is_tablet) or\\
-               (myrule == "touchcapable" and user_agent_obj.is_touch_capable) or\\
-               (myrule == "pc" and user_agent_obj.is_pc) or\\
-               (myrule == "bot" and user_agent_obj.is_bot):
-                browser_ok = False
-        elif myrule and myrule in user_agent_string:
-            browser_ok = False
-
-    if not browser_ok:
+
+    # Browser rule validation, if configured:
+    browser = ValidatesBrowser(config)
+    if not browser.is_supported(request.user_agent.string):
         raise ExperimentError('browser_type_not_allowed')
 
     hit_id = request.args['hitId']
@@ -522,7 +524,7 @@ def advertisement():
     mode = config.get('mode')
     debug_mode = mode == 'debug'
     worker_id = request.args.get('workerId')
-    status = None
+    participant = None
 
     if worker_id is not None:
         # First check if this workerId has completed the task before
@@ -540,15 +542,13 @@ def advertisement():
         # Next, check for participants already associated with this very
         # assignment, and retain their status, if found:
         try:
-            part = models.Participant.query.\\
+            participant = models.Participant.query.\\
                 filter(models.Participant.hit_id == hit_id).\\
                 filter(models.Participant.assignment_id == assignment_id).\\
                 filter(models.Participant.worker_id == worker_id).\\
                 one()
         except exc.SQLAlchemyError:
             pass
-        else:
-            status = part.status
 
     recruiter_name = request.args.get('recruiter')
     if recruiter_name:
@@ -556,10 +556,8 @@ def advertisement():
     else:
         recruiter = recruiters.from_config(config)
         recruiter_name = recruiter.nickname
-    ready_for_external_submission = status == 'working' and part.end_time is not None
-    assignment_complete = status in ('submitted', 'approved')
 
-    if assignment_complete or ready_for_external_submission:
+    if should_show_thanks_page_to(participant):
         # They've either done, or they're from a recruiter that requires
         # submission of an external form to complete their participation.
         return render_template(
@@ -571,7 +569,7 @@ def advertisement():
             mode=config.get('mode'),
             app_id=app_id
         )
-    if status == 'working':
+    if participant and participant.status == 'working':
         # Once participants have finished the instructions, we do not allow
         # them to start the task again.
         raise ExperimentError('already_started_exp_mturk')
diff --git a/dallinger/experiment_server/utils.py b/dallinger/experiment_server/utils.py
index 34fddaf1..623850e9 100644
--- a/dallinger/experiment_server/utils.py
+++ b/dallinger/experiment_server/utils.py
@@ -1,3 +1,4 @@
+import user_agents
 from functools import update_wrapper
 from flask import make_response
 
@@ -10,3 +11,38 @@ def nocache(func):
         resp.cache_control.no_cache = True
         return resp
     return update_wrapper(new_func, func)
+
+
+class ValidatesBrowser(object):
+    """Checks if participant's browser has been excluded via the Configuration.
+    """
+    def __init__(self, config):
+        self.config = config
+
+    @property
+    def exclusions(self):
+        """Return list of browser exclusion rules defined in the Configuration.
+        """
+        exclusion_rules = [
+            r.strip() for r in self.config.get('browser_exclude_rule', '').split(',')
+            if r.strip()
+        ]
+        return exclusion_rules
+
+    def is_supported(self, user_agent_string):
+        """Check user agent against configured exclusions.
+        """
+        user_agent_obj = user_agents.parse(user_agent_string)
+        browser_ok = True
+        for rule in self.exclusions:
+            if rule in ["mobile", "tablet", "touchcapable", "pc", "bot"]:
+                if (rule == "mobile" and user_agent_obj.is_mobile) or\\
+                   (rule == "tablet" and user_agent_obj.is_tablet) or\\
+                   (rule == "touchcapable" and user_agent_obj.is_touch_capable) or\\
+                   (rule == "pc" and user_agent_obj.is_pc) or\\
+                   (rule == "bot" and user_agent_obj.is_bot):
+                    browser_ok = False
+            elif rule in user_agent_string:
+                browser_ok = False
+
+        return browser_ok
diff --git a/tests/test_experiment_server.py b/tests/test_experiment_server.py
index e5199cbb..041f1dfd 100644
--- a/tests/test_experiment_server.py
+++ b/tests/test_experiment_server.py
@@ -154,6 +154,30 @@ class TestAdvertisement(object):
         )
         assert b'action="https://www.mturk.com/mturk/externalSubmit"' in resp.data
 
+    def test_overrecruit_sees_thanks_page_in_sandbox(self, a, webapp, active_config):
+        active_config.extend({'mode': u'sandbox'})
+        p = a.participant()
+        p.end_time = datetime.now()
+        p.status = u'overrecruited'
+        resp = webapp.get(
+            '/ad?hitId={}&assignmentId={}&workerId={}'.format(
+                p.hit_id, p.assignment_id, p.worker_id
+            )
+        )
+        assert b'action="https://workersandbox.mturk.com/mturk/externalSubmit"' in resp.data
+
+    def test_overrecruit_sees_thanks_page_in_live_mode(self, a, webapp, active_config):
+        active_config.extend({'mode': u'live'})
+        p = a.participant()
+        p.end_time = datetime.now()
+        p.status = u'overrecruited'
+        resp = webapp.get(
+            '/ad?hitId={}&assignmentId={}&workerId={}'.format(
+                p.hit_id, p.assignment_id, p.worker_id
+            )
+        )
+        assert b'action="https://www.mturk.com/mturk/externalSubmit"' in resp.data
+
     @pytest.mark.skip(reason="fails pending support for different recruiters")
     def test_submitted_hit_shows_thanks_page_in_sandbox(self, a, webapp, active_config):
         active_config.extend({'mode': u'sandbox'})