Fix #1007 -- small improvements to training request workflow

workshops/filters.py

@@ -375,14 +375,23 @@ def filter_affiliation(queryset, affiliation):
                        .distinct()
 
 
+def filter_training_requests_by_state(queryset, choice):
+    if choice == '':
+        return queryset.exclude(state='d')
+    else:
+        return queryset.filter(state=choice)
+
+
 class TrainingRequestFilter(AMYFilterSet):
     search = django_filters.CharFilter(
         label='Name or Email',
         action=filter_by_person,
     )
 
     state = django_filters.ChoiceFilter(
-        choices=(('', 'All'),) + TrainingRequest.STATES,
+        label='State',
+        choices=[('', 'Pending or accepted')] + TrainingRequest.STATES,
+        action=filter_training_requests_by_state,
     )
 
     matched = django_filters.ChoiceFilter(

workshops/forms.py

@@ -1428,24 +1428,26 @@ def clean(self):
                                   'and match with a trainee.')
 
 
-class AcceptTrainingRequestForm(forms.Form):
+class MatchTrainingRequestForm(forms.Form):
+    """Form used to match a training request to a Person."""
     person = selectable.AutoCompleteSelectField(
         lookup_class=lookups.PersonLookup,
         label='Trainee Account',
         required=False,
         widget=selectable.AutoComboboxSelectWidget,
     )
 
-    helper = BootstrapHelper(add_submit_button=False)
+    helper = BootstrapHelper(add_submit_button=False,
+                             add_cancel_button=False)
     helper.layout = Layout(
         'person',
 
         FormActions(
             Submit('match-selected-person',
-                   'Accept & match to selected trainee account'),
+                   'Match to selected trainee account'),
             HTML('&nbsp;<strong>OR</strong>&nbsp;&nbsp;'),
             Submit('create-new-person',
-                   'Accept & create new trainee account'),
+                   'Create new trainee account'),
         )
     )
 

workshops/models.py

@@ -1850,11 +1850,11 @@ def build_choice_field_with_other_option(choices, default, verbose_name=None):
 
 @reversion.register
 class TrainingRequest(ActiveMixin, CreatedUpdatedMixin, models.Model):
-    STATES = (
+    STATES = [
         ('p', 'Pending'),  # initial state
         ('a', 'Accepted'),  # state after matching a Person record
         ('d', 'Discarded'),
-    )
+    ]
     state = models.CharField(choices=STATES, default='p', max_length=1)
 
     person = models.ForeignKey(Person, null=True, blank=True,
@@ -2043,17 +2043,17 @@ class TrainingRequest(ActiveMixin, CreatedUpdatedMixin, models.Model):
     def clean(self):
         super().clean()
 
-        if self.state == 'a' and self.person is None:
-            raise ValidationError({'person': 'Accepted training request must '
-                                             'be matched to a person.'})
-
-        if self.state == 'p' and self.person is not None:
-            raise ValidationError({'person': 'Pending training requests cannot '
-                                             'be matched to a person.'})
+        if self.state == 'p' and self.person is not None \
+                and self.person.get_training_tasks().exists():
+            raise ValidationError({'state': 'Pending training request cannot '
+                                            'be matched with a training.'})
 
     def get_absolute_url(self):
         return reverse('trainingrequest_details', args=[self.pk])
 
+    def __str__(self):
+        return "Training request #{}".format(self.pk)
+
 
 @reversion.register
 class TrainingRequirement(models.Model):

workshops/templates/workshops/all_trainees.html

@@ -61,7 +61,7 @@
           <a href="{% url 'person_details' trainee.id %}">
             {{ trainee.get_full_name }}
           </a>
-          {% if trainee.email %} &lt;{{ trainee.email|urlize }}&gt; {% endif %}
+          {% if trainee.email %}<br /> &lt;{{ trainee.email|urlize }}&gt; {% endif %}
         </td>
         <td>
           {% for progress in trainee.trainingprogress_set.all %}
@@ -89,7 +89,7 @@
         </td>
         <td>
           {% for req in trainee.trainingrequest_set.all %}
-            <a href="{% url 'trainingrequest_details' req.pk %}">{{ req.created_at }}</a><br />
+            <a href="{% url 'trainingrequest_details' req.pk %}">{{ req.created_at|date:'Y-m-d H:i' }}</a><br />
           {% endfor %}
         </td>
         <td>

workshops/templates/workshops/all_trainingrequests.html

@@ -59,7 +59,10 @@
                    {% if req in form.cleaned_data.requests or req in match_form.cleaned_data.requests %}checked {% endif %}
             />
           </td>
-          <td>{{ req.personal }} {{ req.family }}</td>
+          <td>
+            {{ req.personal }} {{ req.family }}<br />
+            &lt;{{ req.email|urlize }}&gt;
+          </td>
           {% if request.GET.affiliation or request.GET.location %}
             <td>
               {{ req.affiliation }}
@@ -70,14 +73,14 @@
             </td>
             <td>{{ req.location }}</td>
           {% endif %}
-          <td>{{ req.created_at }}</td>
+          <td>{{ req.created_at|date:'Y-m-d H:i' }}</td>
           <td>
             {% if req.person %}
               <a href="{% url 'person_details' req.person.pk %}">
                 {{ req.person.get_full_name }}
               </a>
               {% if req.person.email %}
-                &lt;{{ req.person.email|urlize }}&gt;
+                <br />&lt;{{ req.person.email|urlize }}&gt;
               {% endif %}
             {% else %}—{% endif %}
           </td>

workshops/templates/workshops/trainingrequest.html

@@ -12,12 +12,12 @@
 
 {% last_modified req %}
 
-{% if req.state == 'p' %}
-  <h2>Accept Request</h2>
+{% if not req.person %}
+  <h2>Match Request to AMY account</h2>
   {% if form.initial.person %}
-    <p>It looks like the submitter has already an account. Click "accept &amp; match to selected trainee account" unless the following account doesn't match submitter.</p>
+    <p>It looks like the submitter has already an account. Click "match to selected trainee account" unless the following account doesn't match submitter.</p>
   {% else %}
-    <p>It looks like we don't have any account for the request's submitter. Click "accept &amp; create new trainee account" unless you can find this submitter's account.</p>
+    <p>It looks like we don't have any account for the request's submitter. Click "create new trainee account" unless you can find this submitter's account.</p>
   {% endif %}
   {% crispy form %}
 {% endif %}

workshops/test/test_training_request.py

@@ -89,23 +89,44 @@ def create_training_request(state, person):
 
 class TestTrainingRequestModel(TestBase):
     def setUp(self):
+        # create admin account
         self._setUpUsersAndLogin()
 
-    def test_valid_pending_request(self):
-        req = create_training_request(state='p', person=None)
+        # create trainee account
+        self._setUpRoles()
+        self._setUpTags()
+
+        self.trainee = Person.objects.create_user(
+            username='trainee', personal='Bob',
+            family='Smith', email='bob@smith.com')
+        org = Organization.objects.create(domain='example.com',
+                                          fullname='Test Organization')
+        training = Event.objects.create(slug='training', host=org)
+        training.tags.add(Tag.objects.get(name='TTT'))
+        learner = Role.objects.get(name='learner')
+        Task.objects.create(person=self.trainee, event=training, role=learner)
+
+    def test_accepted_request_are_always_valid(self):
+        """Accepted training requests are valid regardless of whether they
+        are matched to a training."""
+        req = create_training_request(state='a', person=None)
         req.full_clean()
 
-    def test_valid_accepted_request(self):
         req = create_training_request(state='a', person=self.admin)
         req.full_clean()
 
-    def test_pending_request_must_not_be_matched(self):
+        req = create_training_request(state='a', person=self.trainee)
+        req.full_clean()
+
+    def test_valid_pending_request(self):
+        req = create_training_request(state='p', person=None)
+        req.full_clean()
+
         req = create_training_request(state='p', person=self.admin)
-        with self.assertRaises(ValidationError):
-            req.full_clean()
+        req.full_clean()
 
-    def test_accepted_request_must_be_matched_to_a_trainee(self):
-        req = create_training_request(state='a', person=None)
+    def test_pending_request_must_not_be_matched(self):
+        req = create_training_request(state='p', person=self.trainee)
         with self.assertRaises(ValidationError):
             req.full_clean()
 
@@ -139,8 +160,10 @@ def setUp(self):
     def test_view_loads(self):
         rv = self.client.get(reverse('all_trainingrequests'))
         self.assertEqual(rv.status_code, 200)
+        # By default, only pending and accepted requests are displayed,
+        # therefore, self.first_req is missing.
         self.assertEqual(set(rv.context['requests']),
-                         {self.first_req, self.second_req, self.third_req})
+                         {self.second_req, self.third_req})
 
     def test_successful_bulk_discard(self):
         data = {
@@ -201,8 +224,8 @@ def test_successful_matching_twice_to_the_same_training(self):
                                                   task__role__name='learner')),
                          {self.first_training})
 
-    def test_matching_to_training_fails_in_the_case_of_pending_requests(self):
-        """Pending requests that are not matched with any trainee cannot be
+    def test_matching_to_training_fails_in_the_case_of_unmatched_persons(self):
+        """Requests that are not matched with any trainee account cannot be
         matched with a training. """
 
         data = {
@@ -221,7 +244,7 @@ def test_matching_to_training_fails_in_the_case_of_pending_requests(self):
         msg = 'Fix errors below and try again.'
         self.assertContains(rv, msg)
         msg = ('Some of the requests are not matched to a trainee yet. Before '
-              'matching them to a training, you need to accept them '
+               'matching them to a training, you need to accept them '
                'and match with a trainee.')
         self.assertContains(rv, msg)
         # Check that Spiderman is not matched to second_training even though
@@ -247,8 +270,8 @@ def test_successful_unmatching(self):
                                                   task__role__name='learner')),
                          set())
 
-    def test_unmatching_fails_in_the_case_of_pending_requests(self):
-        """Pending requests that are not matched with any trainee cannot be
+    def test_unmatching_fails_when_no_matched_trainee(self):
+        """Requests that are not matched with any trainee cannot be
         unmatched from a training."""
 
         data = {
@@ -286,24 +309,26 @@ def test_basic(self):
         self.assertEqual(got, expected)
 
 
-class TestAcceptingTrainingRequestAndDetailedView(TestBase):
+class TestMatchingTrainingRequestAndDetailedView(TestBase):
     def setUp(self):
         self._setUpUsersAndLogin()
         self._setUpRoles()
 
     def test_detailed_view_of_pending_request(self):
-        """Accept Request form should be displayed only for pending requests."""
+        """Match Request form should be displayed only when no account is
+        matched. """
         req = create_training_request(state='p', person=None)
         rv = self.client.get(reverse('trainingrequest_details', args=[req.pk]))
         self.assertEqual(rv.status_code, 200)
-        self.assertContains(rv, 'Accept Request')
+        self.assertContains(rv, 'Match Request to AMY account')
 
     def test_detailed_view_of_accepted_request(self):
-        """Accept Request form should be displayed only for pending requests."""
-        req = create_training_request(state='a', person=self.admin)
+        """Match Request form should be displayed only when no account is
+        matched. """
+        req = create_training_request(state='p', person=self.admin)
         rv = self.client.get(reverse('trainingrequest_details', args=[req.pk]))
         self.assertEqual(rv.status_code, 200)
-        self.assertNotContains(rv, 'Accept Request')
+        self.assertNotContains(rv, 'Match Request to AMY account')
 
     def test_person_is_suggested(self):
         req = create_training_request(state='p', person=None)
@@ -331,7 +356,7 @@ def test_matching_with_existing_account_works(self):
                               follow=True)
         self.assertEqual(rv.status_code, 200)
         req.refresh_from_db()
-        self.assertEqual(req.state, 'a')
+        self.assertEqual(req.state, 'p')
         self.assertEqual(req.person, self.admin)
 
     def test_matching_with_new_account_works(self):
@@ -341,7 +366,7 @@ def test_matching_with_new_account_works(self):
                               follow=True)
         self.assertEqual(rv.status_code, 200)
         req.refresh_from_db()
-        self.assertEqual(req.state, 'a')
+        self.assertEqual(req.state, 'p')
 
 
 class TestTrainingRequestTemplateTags(TestBase):

workshops/views.py

@@ -85,7 +85,7 @@
     BulkAddTrainingProgressForm,
     BulkChangeTrainingRequestForm,
     BulkMatchTrainingRequestForm,
-    AcceptTrainingRequestForm,
+    MatchTrainingRequestForm,
     TrainingRequestUpdateForm,
     SendHomeworkForm,
     BulkDiscardProgressesForm,
@@ -3291,7 +3291,7 @@ def all_trainingrequests(request):
     return render(request, 'workshops/all_trainingrequests.html', context)
 
 
-def _accept_training_request(form, training_request, request):
+def _match_training_request_to_person(form, training_request, request):
     assert form.action in ('match', 'create')
     try:
         if form.action == 'create':
@@ -3328,11 +3328,9 @@ def _accept_training_request(form, training_request, request):
         training_request.person.is_active = True
         training_request.person.save()
         training_request.person.synchronize_usersocialauth()
-
-        training_request.state = 'a'  # accepted
         training_request.save()
 
-        messages.success(request, 'Request accepted.')
+        messages.success(request, 'Request matched with the person.')
 
         return True
 
@@ -3342,10 +3340,10 @@ def trainingrequest_details(request, pk):
     req = get_object_or_404(TrainingRequest, pk=int(pk))
 
     if request.method == 'POST':
-        form = AcceptTrainingRequestForm(request.POST)
+        form = MatchTrainingRequestForm(request.POST)
 
         if form.is_valid():
-            ok = _accept_training_request(form, req, request)
+            ok = _match_training_request_to_person(form, req, request)
             if ok:
                 next_url = request.GET.get('next', None)
                 if next_url is not None and is_safe_url(next_url):
@@ -3364,7 +3362,7 @@ def trainingrequest_details(request, pk):
                                            Q(personal__iexact=req.personal,
                                              family__iexact=req.family)) \
                                    .first()  # may return None
-        form = AcceptTrainingRequestForm(initial={'person': person})
+        form = MatchTrainingRequestForm(initial={'person': person})
 
     context = {
         'title': 'Training request #{}'.format(req.pk),