From 9978cc400c996abaaa860698b935ce188ef65259 Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 14:08:40 +0200 Subject: [PATCH 1/8] Change format of created_at column in training requests view --- workshops/templates/workshops/all_trainingrequests.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workshops/templates/workshops/all_trainingrequests.html b/workshops/templates/workshops/all_trainingrequests.html index 1aac695b1..c5b4fa5a4 100644 --- a/workshops/templates/workshops/all_trainingrequests.html +++ b/workshops/templates/workshops/all_trainingrequests.html @@ -70,7 +70,7 @@ {{ req.location }} {% endif %} - {{ req.created_at }} + {{ req.created_at|date:'Y-m-d H:i' }} {% if req.person %} From 141893d40f95c9988c03436ad8db4751cea296e9 Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 14:32:50 +0200 Subject: [PATCH 2/8] Display only pending and accepted training requests by default --- workshops/filters.py | 11 ++++++++++- workshops/models.py | 4 ++-- workshops/test/test_training_request.py | 4 +++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/workshops/filters.py b/workshops/filters.py index b752819e2..01d07bd75 100644 --- a/workshops/filters.py +++ b/workshops/filters.py @@ -375,6 +375,13 @@ 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', @@ -382,7 +389,9 @@ class TrainingRequestFilter(AMYFilterSet): ) 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( diff --git a/workshops/models.py b/workshops/models.py index 690c240e1..2721c3cf2 100644 --- a/workshops/models.py +++ b/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, diff --git a/workshops/test/test_training_request.py b/workshops/test/test_training_request.py index 5f216ec83..e5bd03fd9 100644 --- a/workshops/test/test_training_request.py +++ b/workshops/test/test_training_request.py @@ -139,8 +139,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 = { From e9e3395576f0588b758db546fff3482c964b849f Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 14:37:27 +0200 Subject: [PATCH 3/8] Display emails below full name in training requests view --- workshops/templates/workshops/all_trainingrequests.html | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/workshops/templates/workshops/all_trainingrequests.html b/workshops/templates/workshops/all_trainingrequests.html index c5b4fa5a4..8b4792bc6 100644 --- a/workshops/templates/workshops/all_trainingrequests.html +++ b/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 %} /> - {{ req.personal }} {{ req.family }} + + {{ req.personal }} {{ req.family }}
+ <{{ req.email|urlize }}> + {% if request.GET.affiliation or request.GET.location %} {{ req.affiliation }} @@ -77,7 +80,7 @@ {{ req.person.get_full_name }} {% if req.person.email %} - <{{ req.person.email|urlize }}> +
<{{ req.person.email|urlize }}> {% endif %} {% else %}—{% endif %} From ee8c3ac9c4ceedb6cce6b842cfa8d56d5804498a Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 14:40:40 +0200 Subject: [PATCH 4/8] Change title of training request edit view --- workshops/models.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/workshops/models.py b/workshops/models.py index 2721c3cf2..1d9ab8b50 100644 --- a/workshops/models.py +++ b/workshops/models.py @@ -2054,6 +2054,9 @@ def clean(self): 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): From 38bdc119569457028d21f9da60f9db0a5a07e1fd Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 16:50:06 +0200 Subject: [PATCH 5/8] Change validation rules of TrainingRequest.state Until now, pending requests can't be matched with a person and accepted requests must be matched with a person. Now, pending requests can't be matched with a *training* and accepted requests may or may not be matched with a training. It doesn't matter whether a request is matched with a person. --- workshops/forms.py | 7 +- workshops/models.py | 11 ++-- .../templates/workshops/trainingrequest.html | 8 +-- workshops/test/test_training_request.py | 65 +++++++++++++------ workshops/views.py | 14 ++-- 5 files changed, 62 insertions(+), 43 deletions(-) diff --git a/workshops/forms.py b/workshops/forms.py index 77867f147..5cc41b253 100644 --- a/workshops/forms.py +++ b/workshops/forms.py @@ -1428,7 +1428,8 @@ 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', @@ -1442,10 +1443,10 @@ class AcceptTrainingRequestForm(forms.Form): FormActions( Submit('match-selected-person', - 'Accept & match to selected trainee account'), + 'Match to selected trainee account'), HTML(' OR  '), Submit('create-new-person', - 'Accept & create new trainee account'), + 'Create new trainee account'), ) ) diff --git a/workshops/models.py b/workshops/models.py index 1d9ab8b50..cb4583887 100644 --- a/workshops/models.py +++ b/workshops/models.py @@ -2043,13 +2043,10 @@ 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]) diff --git a/workshops/templates/workshops/trainingrequest.html b/workshops/templates/workshops/trainingrequest.html index 395f1dd0c..390632b82 100644 --- a/workshops/templates/workshops/trainingrequest.html +++ b/workshops/templates/workshops/trainingrequest.html @@ -12,12 +12,12 @@ {% last_modified req %} -{% if req.state == 'p' %} -

Accept Request

+{% if not req.person %} +

Match Request to AMY account

{% if form.initial.person %} -

It looks like the submitter has already an account. Click "accept & match to selected trainee account" unless the following account doesn't match submitter.

+

It looks like the submitter has already an account. Click "match to selected trainee account" unless the following account doesn't match submitter.

{% else %} -

It looks like we don't have any account for the request's submitter. Click "accept & create new trainee account" unless you can find this submitter's account.

+

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.

{% endif %} {% crispy form %} {% endif %} diff --git a/workshops/test/test_training_request.py b/workshops/test/test_training_request.py index e5bd03fd9..cee4474b5 100644 --- a/workshops/test/test_training_request.py +++ b/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() @@ -203,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 = { @@ -223,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 @@ -249,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 = { @@ -288,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) @@ -333,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): @@ -343,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): diff --git a/workshops/views.py b/workshops/views.py index 53e7ee325..8551cf440 100644 --- a/workshops/views.py +++ b/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), From 23aa5fe733cc5bb6e59f586c67656f8d0b0953de Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 16:54:34 +0200 Subject: [PATCH 6/8] Remove cancel button from MatchTrainingRequestForm because it's unaligned with other buttons due to using custom layout. --- workshops/forms.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/workshops/forms.py b/workshops/forms.py index 5cc41b253..64ef6f2d4 100644 --- a/workshops/forms.py +++ b/workshops/forms.py @@ -1437,7 +1437,8 @@ class MatchTrainingRequestForm(forms.Form): widget=selectable.AutoComboboxSelectWidget, ) - helper = BootstrapHelper(add_submit_button=False) + helper = BootstrapHelper(add_submit_button=False, + add_cancel_button=False) helper.layout = Layout( 'person', From 3736524146ead339ce2f14cdec7bff5e0331e7e2 Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 16:56:06 +0200 Subject: [PATCH 7/8] Display emails below full name in trainees view --- workshops/templates/workshops/all_trainees.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workshops/templates/workshops/all_trainees.html b/workshops/templates/workshops/all_trainees.html index 12e4da06d..498e3b1ae 100644 --- a/workshops/templates/workshops/all_trainees.html +++ b/workshops/templates/workshops/all_trainees.html @@ -61,7 +61,7 @@ {{ trainee.get_full_name }} - {% if trainee.email %} <{{ trainee.email|urlize }}> {% endif %} + {% if trainee.email %}
<{{ trainee.email|urlize }}> {% endif %} {% for progress in trainee.trainingprogress_set.all %} From 7fcedd0dbea3eb2ff0a151f0bf548a9e65f4dacb Mon Sep 17 00:00:00 2001 From: Christopher Medrela Date: Wed, 28 Sep 2016 16:56:59 +0200 Subject: [PATCH 8/8] Change format of 'training requests' column in trainees view --- workshops/templates/workshops/all_trainees.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workshops/templates/workshops/all_trainees.html b/workshops/templates/workshops/all_trainees.html index 498e3b1ae..8821dec58 100644 --- a/workshops/templates/workshops/all_trainees.html +++ b/workshops/templates/workshops/all_trainees.html @@ -89,7 +89,7 @@ {% for req in trainee.trainingrequest_set.all %} - {{ req.created_at }}
+ {{ req.created_at|date:'Y-m-d H:i' }}
{% endfor %}