Merge pull request #465 from peterbe/bug-1199722-better-tag-merge-wor…

…kflow

fixes bug 1199722 - Better Tag Merge Workflow

airmozilla/manage/forms.py

@@ -768,16 +768,27 @@ class TagEditForm(BaseModelForm):
     class Meta:
         model = Tag
 
+    def clean_name(self):
+        value = self.cleaned_data['name']
+        other_tags = Tag.objects.filter(name__iexact=value)
+        if self.instance:
+            other_tags = other_tags.exclude(id=self.instance.id)
+        if other_tags.exists():
+            raise forms.ValidationError(
+                'Used by another tag. Consider merging.'
+            )
+        return value
 
-class TagMergeForm(BaseForm):
+
+class TagMergeRepeatedForm(BaseForm):
 
     keep = forms.ChoiceField(
         label='Name to keep',
         widget=forms.widgets.RadioSelect()
     )
 
     def __init__(self, this_tag, *args, **kwargs):
-        super(TagMergeForm, self).__init__(*args, **kwargs)
+        super(TagMergeRepeatedForm, self).__init__(*args, **kwargs)
 
         def describe_tag(tag):
             count = Event.objects.filter(tags=tag).count()
@@ -793,6 +804,26 @@ def describe_tag(tag):
         ]
 
 
+class TagMergeForm(BaseForm):
+
+    name = forms.CharField()
+
+    def __init__(self, this_tag, *args, **kwargs):
+        super(TagMergeForm, self).__init__(*args, **kwargs)
+        self.this_tag = this_tag
+
+    def clean_name(self):
+        value = self.cleaned_data['name']
+        other_tags = (
+            Tag.objects
+            .filter(name__iexact=value)
+            .exclude(id=self.this_tag.id)
+        )
+        if not other_tags.exists():
+            raise forms.ValidationError('Not found')
+        return value
+
+
 class VidlyResubmitForm(VidlyURLForm):
     id = forms.IntegerField(widget=forms.widgets.HiddenInput())
 

airmozilla/manage/templates/manage/tag_edit.html

@@ -10,11 +10,21 @@
   {% include "manage/_default_form.html" %}
 
   {% if is_repeated %}
-  <div id="repeated">
-    <p>This tag is repeated <b>{{ repeated }} times</b> in different case.</p>
+    <div id="repeated">
+      <p>This tag is repeated <b>{{ repeated }} times</b> in different case.</p>
+      <form action="{{ url('manage:tag_merge_repeated', tag.pk) }}" method="post" class="form-horizontal" role="form">
+          {{ csrf() }}
+          {{ bootstrapform_horizontal(repeated_form) }}
+          {% set submit_text='Merge' %}
+          {% include "manage/_form_buttons.html" %}
+      </form>
+    </div>
+  {% else %}
+  <div id="merge">
+    <p>To move all occurance of this tag to another, type the other's (the one you're keeping) name below.</p>
     <form action="{{ url('manage:tag_merge', tag.pk) }}" method="post" class="form-horizontal" role="form">
         {{ csrf() }}
-        {{ bootstrapform_horizontal(repeated_form) }}
+        {{ bootstrapform_horizontal(merge_form) }}
         {% set submit_text='Merge' %}
         {% include "manage/_form_buttons.html" %}
     </form>

airmozilla/manage/tests/views/test_tags.py

@@ -61,12 +61,10 @@ def test_tag_edit(self):
         response = self.client.post(url, {
             'name': 'ALREADYINUSE',
         })
-        eq_(response.status_code, 302)
-        # because this is causeing a duplicate it redirects back
-        self.assertRedirects(response, url)
-        eq_(Tag.objects.filter(name__iexact='Alreadyinuse').count(), 2)
+        eq_(response.status_code, 200)
+        ok_('Used by another tag' in response.content)
 
-    def test_tag_merge(self):
+    def test_tag_merge_repeated(self):
         t1 = Tag.objects.create(name='Tagg')
         t2 = Tag.objects.create(name='TaGG')
         t3 = Tag.objects.create(name='tAgg')
@@ -92,7 +90,7 @@ def test_tag_merge(self):
         response = self.client.get(edit_url)
         eq_(response.status_code, 200)
 
-        merge_url = reverse('manage:tag_merge', args=(t1.id,))
+        merge_url = reverse('manage:tag_merge_repeated', args=(t1.id,))
         ok_(merge_url in response.content)
         response = self.client.post(merge_url, {'keep': t2.id})
         eq_(response.status_code, 302)
@@ -109,3 +107,24 @@ def test_tag_merge(self):
         eq_(Event.objects.filter(tags__name='TaGG').count(), 3)
         eq_(Event.objects.filter(tags__name='Tagg').count(), 0)
         eq_(Event.objects.filter(tags__name='tAgg').count(), 0)
+
+    def test_tag_merge(self):
+        t1 = Tag.objects.create(name='Tagg')
+        event = Event.objects.get(title='Test event')
+        event.tags.add(t1)
+
+        t2 = Tag.objects.create(name='Other')
+        event.tags.add(t2)
+
+        # Now suppose you want to only use the 'Other' tag and
+        # move all tags called 'Tagg' to that.
+        url = reverse('manage:tag_merge', args=(t1.id,))
+        # But before we do that, let's make a typo!
+        response = self.client.post(url, {'name': 'UTHER'})
+        eq_(response.status_code, 400)
+        # Now let's spell it correctly
+        response = self.client.post(url, {'name': 'OTHER'})
+        eq_(response.status_code, 302)
+
+        ok_(not Tag.objects.filter(name__iexact='Tagg'))
+        eq_(list(event.tags.all()), list(Tag.objects.filter(name='Other')))

airmozilla/manage/urls.py

@@ -158,8 +158,10 @@
     url(r'^tags/$', tags.tags, name='tags'),
     url(r'^tags/data/$', tags.tags_data, name='tags_data'),
     url(r'^tags/(?P<id>\d+)/$', tags.tag_edit, name='tag_edit'),
-    url(r'^tags/remove/(?P<id>\d+)/$', tags.tag_remove, name='tag_remove'),
-    url(r'^tags/merge/(?P<id>\d+)/$', tags.tag_merge, name='tag_merge'),
+    url(r'^tags/(?P<id>\d+)/remove/$', tags.tag_remove, name='tag_remove'),
+    url(r'^tags/(?P<id>\d+)/merge/$', tags.tag_merge, name='tag_merge'),
+    url(r'^tags/(?P<id>\d+)/merge/repeated/$', tags.tag_merge_repeated,
+        name='tag_merge_repeated'),
     url(r'^locations/new/$', locations.location_new, name='location_new'),
     url(r'^locations/(?P<id>\d+)/$', locations.location_edit,
         name='location_edit'),

airmozilla/manage/views/tags.py

@@ -1,5 +1,6 @@
 import collections
 
+from django import http
 from django.contrib import messages
 from django.shortcuts import render, redirect, get_object_or_404
 from django.db import transaction
@@ -91,13 +92,16 @@ def tag_edit(request, id):
         'repeated': repeated,
         'is_repeated': repeated > 1
     }
-    if repeated:
-        context['repeated_form'] = forms.TagMergeForm(this_tag=tag)
+
+    if repeated > 1:
+        context['repeated_form'] = forms.TagMergeRepeatedForm(this_tag=tag)
+    else:
+        context['merge_form'] = forms.TagMergeForm(this_tag=tag)
     return render(request, 'manage/tag_edit.html', context)
 
 
 @staff_required
-@permission_required('main.change_event')
+@permission_required('main.remove_tag')
 @transaction.commit_on_success
 def tag_remove(request, id):
     if request.method == 'POST':
@@ -110,10 +114,42 @@ def tag_remove(request, id):
 
 
 @staff_required
-@permission_required('main.change_event')
+@permission_required('main.change_tag')
 @cancel_redirect('manage:tags')
 @transaction.commit_on_success
 def tag_merge(request, id):
+    tag = get_object_or_404(Tag, id=id)
+    form = forms.TagMergeForm(tag, request.POST)
+    if not form.is_valid():
+        return http.HttpResponseBadRequest(form.errors)
+    destination = get_object_or_404(
+        Tag,
+        name__iexact=form.cleaned_data['name']
+    )
+
+    count_events = 0
+    for event in Event.objects.filter(tags=tag):
+        event.tags.remove(tag)
+        event.tags.add(destination)
+        count_events += 1
+    tag.delete()
+
+    messages.info(
+        request,
+        '"%s" is the new cool tag (affected %s events)' % (
+            destination.name,
+            count_events,
+        )
+    )
+
+    return redirect('manage:tags')
+
+
+@staff_required
+@permission_required('main.change_tag')
+@cancel_redirect('manage:tags')
+@transaction.commit_on_success
+def tag_merge_repeated(request, id):
     tag = get_object_or_404(Tag, id=id)
     tag_to_keep = get_object_or_404(Tag, id=request.POST['keep'])