From cce32a9b0900210d524b80160209c3e2b5dfb156 Mon Sep 17 00:00:00 2001 From: Jannis Leidel Date: Sun, 2 May 2010 23:02:16 +0000 Subject: [PATCH] Fixed #13166 - Added JavaScript warnings to admin changelist to help against ambiguity between action and list_editable form submission. Thanks to blinkylights and aaugustin for the report and initial patch. git-svn-id: http://code.djangoproject.com/svn/django/trunk@13072 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/media/js/actions.js | 24 ++++++ django/contrib/admin/media/js/actions.min.js | 8 +- django/contrib/admin/options.py | 55 +++++++++---- .../admin/templates/admin/change_list.html | 2 +- .../templates/admin/change_list_results.html | 2 +- tests/regressiontests/admin_views/tests.py | 77 ++++++++++++++++++- 6 files changed, 149 insertions(+), 19 deletions(-) diff --git a/django/contrib/admin/media/js/actions.js b/django/contrib/admin/media/js/actions.js index 1ac5814056..94aa6db934 100644 --- a/django/contrib/admin/media/js/actions.js +++ b/django/contrib/admin/media/js/actions.js @@ -2,6 +2,7 @@ $.fn.actions = function(opts) { var options = $.extend({}, $.fn.actions.defaults, opts); var actionCheckboxes = $(this); + var list_editable_changed = false; checker = function(checked) { if (checked) { showQuestion(); @@ -100,6 +101,29 @@ lastChecked = target; updateCounter(); }); + $('form#changelist-form table#result_list tr').find('td:gt(0) :input').change(function() { + list_editable_changed = true; + }); + $('form#changelist-form button[name="index"]').click(function(event) { + if (list_editable_changed) { + return confirm(gettext("You have unsaved changes on individual editable fields. If you run an action, your unsaved changes will be lost.")); + } + }); + $('form#changelist-form input[name="_save"]').click(function(event) { + var action_changed = false; + $('div.actions select option:selected').each(function() { + if ($(this).val()) { + action_changed = true; + } + }); + if (action_changed) { + if (list_editable_changed) { + return confirm(gettext("You have selected an action, but you haven't saved your changes to individual fields yet. Please click OK to save. You'll need to re-run the action.")); + } else { + return confirm(gettext("You have selected an action, and you haven't made any changes on individual fields. You're probably looking for the Go button rather than the Save button.")); + } + } + }); } /* Setup plugin defaults */ $.fn.actions.defaults = { diff --git a/django/contrib/admin/media/js/actions.min.js b/django/contrib/admin/media/js/actions.min.js index 3973b5352d..21f00cdab9 100644 --- a/django/contrib/admin/media/js/actions.min.js +++ b/django/contrib/admin/media/js/actions.min.js @@ -1,5 +1,7 @@ -(function(a){a.fn.actions=function(g){var b=a.extend({},a.fn.actions.defaults,g),e=a(this);checker=function(c){c?showQuestion():reset();a(e).attr("checked",c).parent().parent().toggleClass(b.selectedClass,c)};updateCounter=function(){var c=a(e).filter(":checked").length;a(b.counterContainer).html(interpolate(ngettext("%(sel)s of %(cnt)s selected","%(sel)s of %(cnt)s selected",c),{sel:c,cnt:_actions_icnt},true));a(b.allToggle).attr("checked",function(){if(c==e.length){value=true;showQuestion()}else{value= +(function(a){a.fn.actions=function(h){var b=a.extend({},a.fn.actions.defaults,h),e=a(this),f=false;checker=function(c){c?showQuestion():reset();a(e).attr("checked",c).parent().parent().toggleClass(b.selectedClass,c)};updateCounter=function(){var c=a(e).filter(":checked").length;a(b.counterContainer).html(interpolate(ngettext("%(sel)s of %(cnt)s selected","%(sel)s of %(cnt)s selected",c),{sel:c,cnt:_actions_icnt},true));a(b.allToggle).attr("checked",function(){if(c==e.length){value=true;showQuestion()}else{value= false;clearAcross()}return value})};showQuestion=function(){a(b.acrossClears).hide();a(b.acrossQuestions).show();a(b.allContainer).hide()};showClear=function(){a(b.acrossClears).show();a(b.acrossQuestions).hide();a(b.actionContainer).toggleClass(b.selectedClass);a(b.allContainer).show();a(b.counterContainer).hide()};reset=function(){a(b.acrossClears).hide();a(b.acrossQuestions).hide();a(b.allContainer).hide();a(b.counterContainer).show()};clearAcross=function(){reset();a(b.acrossInput).val(0);a(b.actionContainer).removeClass(b.selectedClass)}; a(b.counterContainer).show();a(this).filter(":checked").each(function(){a(this).parent().parent().toggleClass(b.selectedClass);updateCounter();a(b.acrossInput).val()==1&&showClear()});a(b.allToggle).show().click(function(){checker(a(this).attr("checked"));updateCounter()});a("div.actions span.question a").click(function(c){c.preventDefault();a(b.acrossInput).val(1);showClear()});a("div.actions span.clear a").click(function(c){c.preventDefault();a(b.allToggle).attr("checked",false);clearAcross();checker(0); -updateCounter()});lastChecked=null;a(e).click(function(c){if(!c)c=window.event;var d=c.target?c.target:c.srcElement;if(lastChecked&&a.data(lastChecked)!=a.data(d)&&c.shiftKey==true){var f=false;a(lastChecked).attr("checked",d.checked).parent().parent().toggleClass(b.selectedClass,d.checked);a(e).each(function(){if(a.data(this)==a.data(lastChecked)||a.data(this)==a.data(d))f=f?false:true;f&&a(this).attr("checked",d.checked).parent().parent().toggleClass(b.selectedClass,d.checked)})}a(d).parent().parent().toggleClass(b.selectedClass, -d.checked);lastChecked=d;updateCounter()})};a.fn.actions.defaults={actionContainer:"div.actions",counterContainer:"span.action-counter",allContainer:"div.actions span.all",acrossInput:"div.actions input.select-across",acrossQuestions:"div.actions span.question",acrossClears:"div.actions span.clear",allToggle:"#action-toggle",selectedClass:"selected"}})(django.jQuery); +updateCounter()});lastChecked=null;a(e).click(function(c){if(!c)c=window.event;var d=c.target?c.target:c.srcElement;if(lastChecked&&a.data(lastChecked)!=a.data(d)&&c.shiftKey==true){var g=false;a(lastChecked).attr("checked",d.checked).parent().parent().toggleClass(b.selectedClass,d.checked);a(e).each(function(){if(a.data(this)==a.data(lastChecked)||a.data(this)==a.data(d))g=g?false:true;g&&a(this).attr("checked",d.checked).parent().parent().toggleClass(b.selectedClass,d.checked)})}a(d).parent().parent().toggleClass(b.selectedClass, +d.checked);lastChecked=d;updateCounter()});a("form#changelist-form table#result_list tr").find("td:gt(0) :input").change(function(){f=true});a('form#changelist-form button[name="index"]').click(function(){if(f)return confirm(gettext("You have unsaved changes on individual editable fields. If you run an action, your unsaved changes will be lost."))});a('form#changelist-form input[name="_save"]').click(function(){var c=false;a("div.actions select option:selected").each(function(){if(a(this).val())c= +true});if(c)return f?confirm(gettext("You have selected an action, but you haven't saved your changes to individual fields yet. Please click OK to save. You'll need to re-run the action.")):confirm(gettext("You have selected an action, and you haven't made any changes on individual fields. You're probably looking for the Go button rather than the Save button."))})};a.fn.actions.defaults={actionContainer:"div.actions",counterContainer:"span.action-counter",allContainer:"div.actions span.all",acrossInput:"div.actions input.select-across", +acrossQuestions:"div.actions span.question",acrossClears:"div.actions span.clear",allToggle:"#action-toggle",selectedClass:"selected"}})(django.jQuery); diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 531ce4d9b7..6334a311da 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -737,11 +737,12 @@ class ModelAdmin(BaseModelAdmin): # Get the list of selected PKs. If nothing's selected, we can't # perform an action on it, so bail. Except we want to perform - # the action explicitely on all objects. + # the action explicitly on all objects. selected = request.POST.getlist(helpers.ACTION_CHECKBOX_NAME) if not selected and not select_across: # Reminder that something needs to be selected or nothing will happen - msg = _("Items must be selected in order to perform actions on them. No items have been changed.") + msg = _("Items must be selected in order to perform " + "actions on them. No items have been changed.") self.message_user(request, msg) return None @@ -761,6 +762,7 @@ class ModelAdmin(BaseModelAdmin): else: msg = _("No action selected.") self.message_user(request, msg) + return None @csrf_protect_m @transaction.commit_on_success @@ -971,20 +973,46 @@ class ModelAdmin(BaseModelAdmin): except IncorrectLookupParameters: # Wacky lookup parameters were given, so redirect to the main # changelist page, without parameters, and pass an 'invalid=1' - # parameter via the query string. If wacky parameters were given and - # the 'invalid=1' parameter was already in the query string, something - # is screwed up with the database, so display an error page. + # parameter via the query string. If wacky parameters were given + # and the 'invalid=1' parameter was already in the query string, + # something is screwed up with the database, so display an error + # page. if ERROR_FLAG in request.GET.keys(): return render_to_response('admin/invalid_setup.html', {'title': _('Database error')}) return HttpResponseRedirect(request.path + '?' + ERROR_FLAG + '=1') - # If the request was POSTed, this might be a bulk action or a bulk edit. - # Try to look up an action or confirmation first, but if this isn't an - # action the POST will fall through to the bulk edit check, below. - if actions and request.method == 'POST' and (helpers.ACTION_CHECKBOX_NAME in request.POST or 'index' in request.POST): - response = self.response_action(request, queryset=cl.get_query_set()) - if response: - return response + # If the request was POSTed, this might be a bulk action or a bulk + # edit. Try to look up an action or confirmation first, but if this + # isn't an action the POST will fall through to the bulk edit check, + # below. + action_failed = False + selected = request.POST.getlist(helpers.ACTION_CHECKBOX_NAME) + + # Actions with no confirmation + if (actions and request.method == 'POST' and + 'index' in request.POST and '_save' not in request.POST): + if selected: + response = self.response_action(request, queryset=cl.get_query_set()) + if response: + return response + else: + action_failed = True + else: + msg = _("Items must be selected in order to perform " + "actions on them. No items have been changed.") + self.message_user(request, msg) + action_failed = True + + # Actions with confirmation + if (actions and request.method == 'POST' and + helpers.ACTION_CHECKBOX_NAME in request.POST and + 'index' not in request.POST and '_save' not in request.POST): + if selected: + response = self.response_action(request, queryset=cl.get_query_set()) + if response: + return response + else: + action_failed = True # If we're allowing changelist editing, we need to construct a formset # for the changelist given all the fields to be edited. Then we'll @@ -992,7 +1020,8 @@ class ModelAdmin(BaseModelAdmin): formset = cl.formset = None # Handle POSTed bulk-edit data. - if request.method == "POST" and self.list_editable: + if (request.method == "POST" and self.list_editable and + '_save' in request.POST and not action_failed): FormSet = self.get_changelist_formset(request) formset = cl.formset = FormSet(request.POST, request.FILES, queryset=cl.result_list) if formset.is_valid(): diff --git a/django/contrib/admin/templates/admin/change_list.html b/django/contrib/admin/templates/admin/change_list.html index 002b887901..3b037e54f9 100644 --- a/django/contrib/admin/templates/admin/change_list.html +++ b/django/contrib/admin/templates/admin/change_list.html @@ -85,7 +85,7 @@ {% endif %} {% endblock %} -
{% csrf_token %} + {% csrf_token %} {% if cl.formset %} {{ cl.formset.management_form }} {% endif %} diff --git a/django/contrib/admin/templates/admin/change_list_results.html b/django/contrib/admin/templates/admin/change_list_results.html index 381dcb5d5d..0efcc9b24a 100644 --- a/django/contrib/admin/templates/admin/change_list_results.html +++ b/django/contrib/admin/templates/admin/change_list_results.html @@ -1,5 +1,5 @@ {% if results %} - +
{% for header in result_headers %} diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index c676555fa7..a516cf1b64 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -1120,6 +1120,8 @@ class AdminViewListEditable(TestCase): "form-2-alive": "checked", "form-2-gender": "1", "form-2-id": "3", + + "_save": "Save", } response = self.client.post('/test_admin/admin/admin_views/person/', data, follow=True) @@ -1140,6 +1142,8 @@ class AdminViewListEditable(TestCase): "form-2-alive": "checked", "form-2-gender": "1", "form-2-id": "3", + + "_save": "Save", } self.client.post('/test_admin/admin/admin_views/person/', data) @@ -1159,6 +1163,8 @@ class AdminViewListEditable(TestCase): "form-1-id": "3", "form-1-gender": "1", "form-1-alive": "checked", + + "_save": "Save", } self.client.post('/test_admin/admin/admin_views/person/?gender__exact=1', data) @@ -1171,7 +1177,9 @@ class AdminViewListEditable(TestCase): "form-MAX_NUM_FORMS": "0", "form-0-id": "1", - "form-0-gender": "1" + "form-0-gender": "1", + + "_save": "Save", } self.client.post('/test_admin/admin/admin_views/person/?q=mauchly', data) @@ -1187,6 +1195,10 @@ class AdminViewListEditable(TestCase): "form-0-id": "2", "form-0-alive": "1", "form-0-gender": "2", + + # Ensure that the form processing understands this as a list_editable "Save" + # and not an action "Go". + "_save": "Save", } response = self.client.post('/test_admin/admin/admin_views/person/', data) self.assertContains(response, "Grace is not a Zombie") @@ -1201,6 +1213,8 @@ class AdminViewListEditable(TestCase): "form-0-id": "2", "form-0-alive": "1", "form-0-gender": "2", + + "_save": "Save", } response = self.client.post('/test_admin/admin/admin_views/person/', data) non_form_errors = response.context['cl'].formset.non_form_errors() @@ -1236,6 +1250,10 @@ class AdminViewListEditable(TestCase): "form-3-order": "0", "form-3-id": "4", "form-3-collector": "1", + + # Ensure that the form processing understands this as a list_editable "Save" + # and not an action "Go". + "_save": "Save", } response = self.client.post('/test_admin/admin/admin_views/category/', data) # Successful post will redirect @@ -1247,6 +1265,63 @@ class AdminViewListEditable(TestCase): self.failUnlessEqual(Category.objects.get(id=3).order, 1) self.failUnlessEqual(Category.objects.get(id=4).order, 0) + def test_list_editable_action_submit(self): + # List editable changes should not be executed if the action "Go" button is + # used to submit the form. + data = { + "form-TOTAL_FORMS": "3", + "form-INITIAL_FORMS": "3", + "form-MAX_NUM_FORMS": "0", + + "form-0-gender": "1", + "form-0-id": "1", + + "form-1-gender": "2", + "form-1-id": "2", + + "form-2-alive": "checked", + "form-2-gender": "1", + "form-2-id": "3", + + "index": "0", + "_selected_action": [u'3'], + "action": [u'', u'delete_selected'], + } + self.client.post('/test_admin/admin/admin_views/person/', data) + + self.failUnlessEqual(Person.objects.get(name="John Mauchly").alive, True) + self.failUnlessEqual(Person.objects.get(name="Grace Hopper").gender, 1) + + def test_list_editable_action_choices(self): + # List editable changes should be executed if the "Save" button is + # used to submit the form - any action choices should be ignored. + data = { + "form-TOTAL_FORMS": "3", + "form-INITIAL_FORMS": "3", + "form-MAX_NUM_FORMS": "0", + + "form-0-gender": "1", + "form-0-id": "1", + + "form-1-gender": "2", + "form-1-id": "2", + + "form-2-alive": "checked", + "form-2-gender": "1", + "form-2-id": "3", + + "_save": "Save", + "_selected_action": [u'1'], + "action": [u'', u'delete_selected'], + } + self.client.post('/test_admin/admin/admin_views/person/', data) + + self.failUnlessEqual(Person.objects.get(name="John Mauchly").alive, False) + self.failUnlessEqual(Person.objects.get(name="Grace Hopper").gender, 2) + + + + class AdminSearchTest(TestCase): fixtures = ['admin-views-users','multiple-child-classes']