From 370f2527ff8e6e3c1260603c912f8b88e7ab397c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 28 Nov 2018 17:40:54 +0000 Subject: [PATCH 01/11] start js for template folder form have a bunch of separate elements within the sticky_template_forms div that we hide or show based on button presses and such. This commit just sets up the class - it doesn't actually deal with button presses or checkboxes etc yet. --- app/assets/javascripts/templateFolderForm.js | 58 ++++++++++++++++++++ app/templates/views/templates/_move_to.html | 2 + app/templates/views/templates/choose.html | 2 +- gulpfile.babel.js | 1 + 4 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 app/assets/javascripts/templateFolderForm.js diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js new file mode 100644 index 000000000..d9e686daa --- /dev/null +++ b/app/assets/javascripts/templateFolderForm.js @@ -0,0 +1,58 @@ +(function(Modules) { + "use strict"; + + Modules.TemplateFolderForm = function() { + + this.start = function(templateFolderForm) { + this.$form = $(templateFolderForm); + + this.$stickyBottom = this.$form.find('#sticky_template_forms'); + + this.$stickyBottom.append(this.nothingSelectedButtons); + this.$stickyBottom.append(this.itemsSelectedButtons); + + // all the diff states that we want to show or hide + this.states = { + nothingSelectedButtons: this.$form.find('#nothing_selected'), + itemsSelectedButtons: this.$form.find('#items_selected'), + moveToFolderRadios: this.$form.find('#move_to_folder_radios'), + addNewFolderName: this.$form.find('#add_new_folder_form'), + moveToNewFolderName: this.$form.find('#move_to_new_folder_form'), + }; + + this.render(); + }; + + this.countSelectedCheckboxes = function() { + return this.$form.find('input[type=checkbox]:checked').length; + }; + + this.render = function() { + let numSelected = this.countSelectedCheckboxes(); + + // hide everything + Object.values(this.states).forEach($el => $el.hide()); + + this.states.nothingSelectedButtons.show(); + }; + + this.nothingSelectedButtons = function() { + return ` +
+ + +
+ `; + }; + + this.itemsSelectedButtons = function() { + return ` +
+ + +
+ `; + }; + }; + +})(window.GOVUK.Modules); diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index b8e35407e..33793f118 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -1,6 +1,7 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} +
{% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %}
@@ -29,3 +30,4 @@ {{ page_footer('Continue', button_name='operation', button_value='add_template') }}
+
diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index 8cba7139c..adb6b19c2 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -78,7 +78,7 @@ {{ live_search(target_selector='#template-list .template-list-item', show=show_search_box, form=search_form) }} {% if can_manage_folders %} - {% call form_wrapper() %} + {% call form_wrapper(module='template-folder-form') %} {% include 'views/templates/_template_list.html' %} {% with templates=templates, template_folders=template_folders, templates_and_folders_form=templates_and_folders_form %} {% include 'views/templates/_move_to.html' %} diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 435b5dcf6..445bc988f 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -75,6 +75,7 @@ gulp.task('javascripts', () => gulp paths.src + 'javascripts/fullscreenTable.js', paths.src + 'javascripts/emailPreviewPane.js', paths.src + 'javascripts/colourPreview.js', + paths.src + 'javascripts/templateFolderForm.js', paths.src + 'javascripts/main.js' ]) .pipe(plugins.prettyerror()) From db9d4aa2cb0be181187b7e799905dd9df1c96cfe Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 29 Nov 2018 16:20:44 +0000 Subject: [PATCH 02/11] change actions based on which checkboxes are selected if action buttons are shown (either the nothing selected actions or the stuff selected actions), when a checkbox is selected or deselected, count how many checkboxes are selected. If it's zero, then show the new template/folder buttons, if it's non-zero, then show the move options. Under the hood, we set the `currentState` variable, then the render fn shows that element and hides all others. --- app/assets/javascripts/templateFolderForm.js | 22 ++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index d9e686daa..534fabb03 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -19,6 +19,22 @@ addNewFolderName: this.$form.find('#add_new_folder_form'), moveToNewFolderName: this.$form.find('#move_to_new_folder_form'), }; + // first off show the new template / new folder buttons + this.currentState = 'nothingSelectedButtons'; + + this.$form.on('change', 'input[type=checkbox]', () => this.templateFolderCheckboxChanged()); + + this.render(); + }; + + this.templateFolderCheckboxChanged = function(event) { + let numSelected = this.countSelectedCheckboxes(); + + if (this.currentState === 'nothingSelectedButtons' && numSelected !== 0) { + this.currentState = 'itemsSelectedButtons'; + } else if (this.currentState === 'itemsSelectedButtons' && numSelected === 0) { + this.currentState = 'nothingSelectedButtons'; + } this.render(); }; @@ -30,10 +46,8 @@ this.render = function() { let numSelected = this.countSelectedCheckboxes(); - // hide everything - Object.values(this.states).forEach($el => $el.hide()); - - this.states.nothingSelectedButtons.show(); + // hide everything, unless they are the currentState + Object.entries(this.states).forEach(([state, $el]) => (state === this.currentState ? $el.show() : $el.hide())); }; this.nothingSelectedButtons = function() { From 9942725d3512bcec04c4a28ab76171fe9dfc9de7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 29 Nov 2018 17:39:58 +0000 Subject: [PATCH 03/11] go to forms on click of action buttons, and detatch and reattach els MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the action buttons have a value that matches up with the key for the target form in the `this.states` object - we can just set the currentState to that and call re-render and it all Just Works™. detatch and reattach feel better than hide/unhide, mainly because it means when the form is posted, any data that might linger in them definitely won't be sent in the POST. --- app/assets/javascripts/templateFolderForm.js | 33 ++++++++++++++------ 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index 534fabb03..961fcdfcc 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -15,19 +15,32 @@ this.states = { nothingSelectedButtons: this.$form.find('#nothing_selected'), itemsSelectedButtons: this.$form.find('#items_selected'), + moveToFolderRadios: this.$form.find('#move_to_folder_radios'), - addNewFolderName: this.$form.find('#add_new_folder_form'), - moveToNewFolderName: this.$form.find('#move_to_new_folder_form'), + moveToNewFolderForm: this.$form.find('#move_to_new_folder_form'), + + addNewFolderForm: this.$form.find('#add_new_folder_form'), + addNewTemplateForm: this.$form.find('#add_new_template_form'), + }; + // first off show the new template / new folder buttons this.currentState = 'nothingSelectedButtons'; + this.$form.on('click', 'button.button-secondary', (event) => this.actionButtonClicked(event)); this.$form.on('change', 'input[type=checkbox]', () => this.templateFolderCheckboxChanged()); this.render(); }; - this.templateFolderCheckboxChanged = function(event) { + this.actionButtonClicked = function(event) { + event.preventDefault(); + this.currentState = $(event.currentTarget).val(); + + this.render(); + }; + + this.templateFolderCheckboxChanged = function() { let numSelected = this.countSelectedCheckboxes(); if (this.currentState === 'nothingSelectedButtons' && numSelected !== 0) { @@ -46,15 +59,17 @@ this.render = function() { let numSelected = this.countSelectedCheckboxes(); - // hide everything, unless they are the currentState - Object.entries(this.states).forEach(([state, $el]) => (state === this.currentState ? $el.show() : $el.hide())); + // detach everything, unless they are the currentState + Object.entries(this.states).forEach( + ([state, $el]) => (state === this.currentState ? this.$stickyBottom.append($el) : $el.detach()) + ); }; this.nothingSelectedButtons = function() { return `
- - + +
`; }; @@ -62,8 +77,8 @@ this.itemsSelectedButtons = function() { return `
- - + +
`; }; From af8de93c301eab51c939dd0be9881a3e5e467947 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 30 Nov 2018 12:02:51 +0000 Subject: [PATCH 04/11] add cancel buttons to template folder flow when cancelled, clears any data inputted into the sub-form action, and then uses an icky hack to go back to the buttons, by changing the state to "nothingSelectedButtons", and then pretending a checkbox was clicked so it works out which actions to show and re-renders. --- app/assets/javascripts/templateFolderForm.js | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index 961fcdfcc..8dc6a95eb 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -24,6 +24,12 @@ }; + // cancel buttons only relevant if JS enabled, so + this.addCancelButton(this.states.moveToFolderRadios); + this.addCancelButton(this.states.moveToNewFolderForm); + this.addCancelButton(this.states.addNewFolderForm); + this.addCancelButton(this.states.addNewTemplateForm); + // first off show the new template / new folder buttons this.currentState = 'nothingSelectedButtons'; @@ -33,6 +39,26 @@ this.render(); }; + this.addCancelButton = function($el) { + let $cancel = $('') + // .addClass('api-key') + // .css('min-height', $component.height()) + .html('Cancel') + .click((event) => { + event.preventDefault(); + // clear existing data + $el.find('input:radio').prop('checked', false); + $el.find('input:text').val(''); + + // gross hack - pretend we're in the choose actions state, then pretend a checkbox was clicked to work out + // whether to show zero or non-zero options. This calls a render at the end + this.currentState = 'nothingSelectedButtons'; + this.templateFolderCheckboxChanged(); + }); + + $el.append($cancel); + }; + this.actionButtonClicked = function(event) { event.preventDefault(); this.currentState = $(event.currentTarget).val(); From bb503268117942e5645c3f6c0b8e2ffac371c4a4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 30 Nov 2018 16:29:00 +0000 Subject: [PATCH 05/11] remove extra add template button and cleanup of html only remove the add template button if they have the folder service permission (thus can see the add button at the bottom). Also make some unnecessary functions into strings in the js, and remove some commented out code --- app/assets/javascripts/templateFolderForm.js | 36 ++++++++------------ app/templates/views/templates/choose.html | 7 ++-- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index 8dc6a95eb..fde801504 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -41,8 +41,6 @@ this.addCancelButton = function($el) { let $cancel = $('') - // .addClass('api-key') - // .css('min-height', $component.height()) .html('Cancel') .click((event) => { event.preventDefault(); @@ -70,8 +68,10 @@ let numSelected = this.countSelectedCheckboxes(); if (this.currentState === 'nothingSelectedButtons' && numSelected !== 0) { + // user has just selected first item this.currentState = 'itemsSelectedButtons'; } else if (this.currentState === 'itemsSelectedButtons' && numSelected === 0) { + // user has just deselected last item this.currentState = 'nothingSelectedButtons'; } @@ -79,35 +79,29 @@ }; this.countSelectedCheckboxes = function() { - return this.$form.find('input[type=checkbox]:checked').length; + return this.$form.find('input:checkbox:checked').length; }; this.render = function() { - let numSelected = this.countSelectedCheckboxes(); - // detach everything, unless they are the currentState Object.entries(this.states).forEach( ([state, $el]) => (state === this.currentState ? this.$stickyBottom.append($el) : $el.detach()) ); }; - this.nothingSelectedButtons = function() { - return ` -
- - -
- `; - }; + this.nothingSelectedButtons = ` +
+ + +
+ `; - this.itemsSelectedButtons = function() { - return ` -
- - -
- `; - }; + this.itemsSelectedButtons = ` +
+ + +
+ `; }; })(window.GOVUK.Modules); diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index adb6b19c2..8ff03b7b3 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -60,8 +60,9 @@ {% if current_user.has_permissions('manage_templates') %}
+ {% if not can_manage_folders %} Add new template - + {% endif %} {% if can_manage_folders and current_template_folder_id %} Manage {% endif %} @@ -80,9 +81,7 @@ {% if can_manage_folders %} {% call form_wrapper(module='template-folder-form') %} {% include 'views/templates/_template_list.html' %} - {% with templates=templates, template_folders=template_folders, templates_and_folders_form=templates_and_folders_form %} - {% include 'views/templates/_move_to.html' %} - {% endwith %} + {% include 'views/templates/_move_to.html' %} {% endcall %} {% else %} From 05de491c0cf066cb38a52ad66a5b9ca8d6d1738c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 30 Nov 2018 16:31:42 +0000 Subject: [PATCH 06/11] pre-check templates/folders that were checked previously (when rendering an error) --- app/main/forms.py | 3 +++ app/templates/views/templates/_template_list.html | 1 + 2 files changed, 4 insertions(+) diff --git a/app/main/forms.py b/app/main/forms.py index d46815e8d..a45644245 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1212,6 +1212,9 @@ class TemplateAndFoldersSelectionForm(Form): ('copy-existing', 'Copy of an existing template') if allow_adding_copy_of_template else None, ])) + def is_selected(self, template_folder_id): + return template_folder_id in (self.templates_and_folders.data or []) + def validate(self): self.op = request.form.get('operation') diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html index f26e32b0d..e4c030e76 100644 --- a/app/templates/views/templates/_template_list.html +++ b/app/templates/views/templates/_template_list.html @@ -17,6 +17,7 @@ {{ unlabelled_checkbox( id='templates-or-folder-{}'.format(item.id), name='templates_and_folders', + data=templates_and_folders_form.is_selected(item.id), value=item.id, ) }} {% endif %} From e27540243de1aee684786cd268b3fde6e8d92334 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 30 Nov 2018 16:32:24 +0000 Subject: [PATCH 07/11] sticky scroll the template form --- app/templates/views/templates/_move_to.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index 33793f118..fc0676219 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -1,7 +1,7 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} -
+
{% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %}
From 97f663f99e37fbb22800f1910cf95faa25ed776f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 4 Dec 2018 14:47:05 +0000 Subject: [PATCH 08/11] change operations to kebab-case so that they better align with the front-end, where they'll be used in data attributes. Also, making the kebab case is nice because it doesn't give favouritism to either JS or python naming conventions --- app/assets/javascripts/templateFolderForm.js | 18 +++++------ app/main/forms.py | 26 ++++++++-------- app/main/views/templates.py | 3 +- app/templates/views/templates/_move_to.html | 9 +++--- tests/app/main/views/test_template_folders.py | 30 +++++++++---------- tests/app/main/views/test_templates.py | 6 ++-- 6 files changed, 46 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index fde801504..722c2d368 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -50,7 +50,7 @@ // gross hack - pretend we're in the choose actions state, then pretend a checkbox was clicked to work out // whether to show zero or non-zero options. This calls a render at the end - this.currentState = 'nothingSelectedButtons'; + this.currentState = 'nothing-selected-buttons'; this.templateFolderCheckboxChanged(); }); @@ -67,12 +67,12 @@ this.templateFolderCheckboxChanged = function() { let numSelected = this.countSelectedCheckboxes(); - if (this.currentState === 'nothingSelectedButtons' && numSelected !== 0) { + if (this.currentState === 'nothing-selected-buttons' && numSelected !== 0) { // user has just selected first item - this.currentState = 'itemsSelectedButtons'; - } else if (this.currentState === 'itemsSelectedButtons' && numSelected === 0) { + this.currentState = 'items-selected-buttons'; + } else if (this.currentState === 'items-selected-buttons' && numSelected === 0) { // user has just deselected last item - this.currentState = 'nothingSelectedButtons'; + this.currentState = 'nothing-selected-buttons'; } this.render(); @@ -91,15 +91,15 @@ this.nothingSelectedButtons = `
- - + +
`; this.itemsSelectedButtons = `
- - + +
`; }; diff --git a/app/main/forms.py b/app/main/forms.py index a45644245..bc4a77b92 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1165,11 +1165,11 @@ class TemplateAndFoldersSelectionForm(Form): * unknown currently not implemented, but in the future will try and work out if there are any obvious commands that can be assumed based on which fields are empty vs populated. - * move_to_existing_folder + * move-to-existing-folder must have data for templates_and_folders checkboxes, and move_to radios - * move_to_new_folder + * move-to-new-folder must have data for move_to_new_folder_name, cannot have data for move_to_existing_folder_name - * add_new_folder + * add-new-folder must have data for move_to_existing_folder_name, cannot have data for move_to_new_folder_name """ @@ -1218,9 +1218,9 @@ class TemplateAndFoldersSelectionForm(Form): def validate(self): self.op = request.form.get('operation') - self.is_move_op = self.op in {'move_to_existing_folder', 'move_to_new_folder'} - self.is_add_folder_op = self.op in {'add_new_folder', 'move_to_new_folder'} - self.is_add_template_op = self.op in {'add_template'} + self.is_move_op = self.op in {'move-to-existing-folder', 'move-to-new-folder'} + self.is_add_folder_op = self.op in {'add-new-folder', 'move-to-new-folder'} + self.is_add_template_op = self.op in {'add-new-template'} if not (self.is_add_folder_op or self.is_move_op or self.is_add_template_op): return False @@ -1228,23 +1228,23 @@ class TemplateAndFoldersSelectionForm(Form): return super().validate() def get_folder_name(self): - if self.op == 'add_new_folder': + if self.op == 'add-new-folder': return self.add_new_folder_name.data - elif self.op == 'move_to_new_folder': + elif self.op == 'move-to-new-folder': return self.move_to_new_folder_name.data return None templates_and_folders = MultiCheckboxField('Choose templates or folders', validators=[ - required_for_ops('move_to_new_folder', 'move_to_existing_folder') + required_for_ops('move-to-new-folder', 'move-to-existing-folder') ]) move_to = RadioFieldWithNoneOption('Choose a folder', validators=[ Optional(), - required_for_ops('move_to_new_folder', 'move_to_existing_folder') + required_for_ops('move-to-new-folder', 'move-to-existing-folder') ]) - add_new_folder_name = StringField('Folder name', validators=[required_for_ops('add_new_folder')]) - move_to_new_folder_name = StringField('Folder name', validators=[required_for_ops('move_to_new_folder')]) + add_new_folder_name = StringField('Folder name', validators=[required_for_ops('add-new-folder')]) + move_to_new_folder_name = StringField('Folder name', validators=[required_for_ops('move-to-new-folder')]) add_template_by_template_type = RadioFieldWithNoneOption('Add new', validators=[ Optional(), - required_for_ops('add_template') + required_for_ops('add-new-template') ]) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 5e4617ba9..151f301de 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -122,7 +122,6 @@ def choose_template(service_id, template_type='all', template_folder_id=None): len(user_api_client.get_service_ids_for_user(current_user)) > 1 ), ) - if request.method == 'POST' and templates_and_folders_form.validate_on_submit(): if not can_manage_folders(): abort(403) @@ -142,7 +141,7 @@ def choose_template(service_id, template_type='all', template_folder_id=None): template_nav_items=get_template_nav_items(template_folder_id), template_type=template_type, search_form=SearchTemplatesForm(), - templates_and_folders_form=templates_and_folders_form + templates_and_folders_form=templates_and_folders_form, ) diff --git a/app/templates/views/templates/_move_to.html b/app/templates/views/templates/_move_to.html index fc0676219..27e8f1124 100644 --- a/app/templates/views/templates/_move_to.html +++ b/app/templates/views/templates/_move_to.html @@ -3,16 +3,17 @@
+ {% if templates_and_folders_form.move_to.choices and template_list.templates_to_show %}
{{ radios(templates_and_folders_form.move_to) }} - {{ page_footer('Move', button_name='operation', button_value='move_to_existing_folder') }} + {{ page_footer('Move', button_name='operation', button_value='move-to-existing-folder') }}
Move to a new folder {{ textbox(templates_and_folders_form.move_to_new_folder_name) }} - {{ page_footer('Move to a new folder', button_name='operation', button_value='move_to_new_folder') }} + {{ page_footer('Move to a new folder', button_name='operation', button_value='move-to-new-folder') }}
{% endif %} @@ -20,14 +21,14 @@
Add a new folder {{ textbox(templates_and_folders_form.add_new_folder_name) }} - {{ page_footer('New folder', button_name='operation', button_value='add_new_folder') }} + {{ page_footer('New folder', button_name='operation', button_value='add-new-folder') }}
Add a new template {{ radios(templates_and_folders_form.add_template_by_template_type) }} - {{ page_footer('Continue', button_name='operation', button_value='add_template') }} + {{ page_footer('Continue', button_name='operation', button_value='add-new-template') }}
diff --git a/tests/app/main/views/test_template_folders.py b/tests/app/main/views/test_template_folders.py index 6f0ff26d2..977d77994 100644 --- a/tests/app/main/views/test_template_folders.py +++ b/tests/app/main/views/test_template_folders.py @@ -797,10 +797,10 @@ def test_should_show_radios_and_buttons_for_move_destination_if_correct_permissi ] assert set(x['value'] for x in page.find_all('button', {'name': 'operation'})) == { 'unknown', - 'move_to_existing_folder', - 'move_to_new_folder', - 'add_new_folder', - 'add_template', + 'move-to-existing-folder', + 'move-to-new-folder', + 'add-new-folder', + 'add-new-template', } @@ -821,7 +821,7 @@ def test_should_be_able_to_move_to_existing_folder( 'main.choose_template', service_id=SERVICE_ONE_ID, _data={ - 'operation': 'move_to_existing_folder', + 'operation': 'move-to-existing-folder', 'move_to': PARENT_FOLDER_ID, 'templates_and_folders': [ FOLDER_TWO_ID, @@ -868,7 +868,7 @@ def test_should_not_be_able_to_move_to_existing_folder_if_dont_have_permission( 'main.choose_template', service_id=SERVICE_ONE_ID, _data={ - 'operation': 'move_to_existing_folder', + 'operation': 'move-to-existing-folder', 'move_to': PARENT_FOLDER_ID, 'templates_and_folders': [ FOLDER_TWO_ID, @@ -900,7 +900,7 @@ def test_should_be_able_to_move_a_sub_item( service_id=SERVICE_ONE_ID, template_folder_id=PARENT_FOLDER_ID, _data={ - 'operation': 'move_to_existing_folder', + 'operation': 'move-to-existing-folder', 'move_to': '__NONE__', 'templates_and_folders': [GRANDCHILD_FOLDER_ID], }, @@ -917,35 +917,35 @@ def test_should_be_able_to_move_a_sub_item( @pytest.mark.parametrize('data', [ # move to existing, but add new folder name given { - 'operation': 'move_to_existing_folder', + 'operation': 'move-to-existing-folder', 'templates_and_folders': [], 'add_new_folder_name': 'foo', 'move_to': PARENT_FOLDER_ID }, # move to existing, but move to new folder name given { - 'operation': 'move_to_existing_folder', + 'operation': 'move-to-existing-folder', 'templates_and_folders': [TEMPLATE_ONE_ID], 'move_to_new_folder_name': 'foo', 'move_to': PARENT_FOLDER_ID }, # move to existing, but no templates to move { - 'operation': 'move_to_existing_folder', + 'operation': 'move-to-existing-folder', 'templates_and_folders': [], 'move_to_new_folder_name': '', 'move_to': PARENT_FOLDER_ID }, # move to new, but nothing selected to move { - 'operation': 'move_to_new_folder', + 'operation': 'move-to-new-folder', 'templates_and_folders': [], 'move_to_new_folder_name': 'foo', 'move_to': None }, # add a new template, but also select move destination { - 'operation': 'add_template', + 'operation': 'add-new-template', 'templates_and_folders': [], 'move_to_new_folder_name': '', 'move_to': PARENT_FOLDER_ID, @@ -953,7 +953,7 @@ def test_should_be_able_to_move_a_sub_item( }, # add a new template, but also move to root folder { - 'operation': 'add_template', + 'operation': 'add-new-template', 'templates_and_folders': [], 'move_to_new_folder_name': '', 'move_to': '__NONE__', @@ -1019,7 +1019,7 @@ def test_new_folder_is_created_if_only_new_folder_is_filled_out( data = { 'move_to_new_folder_name': '', 'add_new_folder_name': 'new folder', - 'operation': 'add_new_folder' + 'operation': 'add-new-folder' } service_one['permissions'] += ['edit_folders'] @@ -1066,7 +1066,7 @@ def test_should_be_able_to_move_to_new_folder( service_id=SERVICE_ONE_ID, template_folder_id=None, _data={ - 'operation': 'move_to_new_folder', + 'operation': 'move-to-new-folder', 'move_to_new_folder_name': 'new folder', 'templates_and_folders': [ FOLDER_TWO_ID, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 6ade834fc..09dee0ec0 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -616,7 +616,7 @@ def test_dont_show_preview_letter_templates_for_bad_filetype( 'template_type': 'copy-existing' }), ('main.choose_template', { - 'operation': 'add_template', + 'operation': 'add-new-template', 'add_template_by_template_type': 'copy-existing' }), )) @@ -791,7 +791,7 @@ def test_cant_copy_template_from_non_member_service( ( 'main.choose_template', { - 'operation': 'add_template', + 'operation': 'add-new-template', 'add_template_by_template_type': 'email', }, "Sending emails has been disabled for your service." @@ -799,7 +799,7 @@ def test_cant_copy_template_from_non_member_service( ( 'main.choose_template', { - 'operation': 'add_template', + 'operation': 'add-new-template', 'add_template_by_template_type': 'sms', }, "Sending text messages has been disabled for your service." From 17cc262ea3a70b5605179ee324b30f9933574c30 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 4 Dec 2018 14:48:39 +0000 Subject: [PATCH 09/11] preserve previous state on form error the html now contains a `data-prev-state` attribute which contains the previous state, taken from the `operation` value in the form data (from the submit button). This is used to seed the `currentState` of the templateFolderForm. If not specified (or 'unknown', because the user hit enter last time round), then set it to nothingSelectedButtons. --- app/assets/javascripts/templateFolderForm.js | 39 +++++++++----------- app/templates/components/form.html | 8 +++- app/templates/views/templates/choose.html | 5 ++- 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index 722c2d368..c09a93599 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -12,26 +12,21 @@ this.$stickyBottom.append(this.itemsSelectedButtons); // all the diff states that we want to show or hide - this.states = { - nothingSelectedButtons: this.$form.find('#nothing_selected'), - itemsSelectedButtons: this.$form.find('#items_selected'), - - moveToFolderRadios: this.$form.find('#move_to_folder_radios'), - moveToNewFolderForm: this.$form.find('#move_to_new_folder_form'), - - addNewFolderForm: this.$form.find('#add_new_folder_form'), - addNewTemplateForm: this.$form.find('#add_new_template_form'), - - }; + this.states = [ + {key: 'nothing-selected-buttons', $el: this.$form.find('#nothing_selected'), cancellable: false}, + {key: 'items-selected-buttons', $el: this.$form.find('#items_selected'), cancellable: false}, + {key: 'move-to-existing-folder', $el: this.$form.find('#move_to_folder_radios'), cancellable: true}, + {key: 'move-to-new-folder', $el: this.$form.find('#move_to_new_folder_form'), cancellable: true}, + {key: 'add-new-folder', $el: this.$form.find('#add_new_folder_form'), cancellable: true}, + {key: 'add-new-template', $el: this.$form.find('#add_new_template_form'), cancellable: true} + ]; // cancel buttons only relevant if JS enabled, so - this.addCancelButton(this.states.moveToFolderRadios); - this.addCancelButton(this.states.moveToNewFolderForm); - this.addCancelButton(this.states.addNewFolderForm); - this.addCancelButton(this.states.addNewTemplateForm); + this.states.filter(state => state.cancellable).forEach((x) => this.addCancelButton(x)); // first off show the new template / new folder buttons - this.currentState = 'nothingSelectedButtons'; + let prevState = this.$form.data('prev-state') || 'unknown'; + this.currentState = (prevState === 'unknown') ? 'nothing-selected-buttons' : prevState; this.$form.on('click', 'button.button-secondary', (event) => this.actionButtonClicked(event)); this.$form.on('change', 'input[type=checkbox]', () => this.templateFolderCheckboxChanged()); @@ -39,14 +34,14 @@ this.render(); }; - this.addCancelButton = function($el) { + this.addCancelButton = function(state) { let $cancel = $('') .html('Cancel') .click((event) => { event.preventDefault(); // clear existing data - $el.find('input:radio').prop('checked', false); - $el.find('input:text').val(''); + state.$el.find('input:radio').prop('checked', false); + state.$el.find('input:text').val(''); // gross hack - pretend we're in the choose actions state, then pretend a checkbox was clicked to work out // whether to show zero or non-zero options. This calls a render at the end @@ -54,7 +49,7 @@ this.templateFolderCheckboxChanged(); }); - $el.append($cancel); + state.$el.append($cancel); }; this.actionButtonClicked = function(event) { @@ -84,8 +79,8 @@ this.render = function() { // detach everything, unless they are the currentState - Object.entries(this.states).forEach( - ([state, $el]) => (state === this.currentState ? this.$stickyBottom.append($el) : $el.detach()) + this.states.forEach( + state => (state.key === this.currentState ? this.$stickyBottom.append(state.$el) : state.$el.detach()) ); }; diff --git a/app/templates/components/form.html b/app/templates/components/form.html index f01ee87f4..17cabc7f0 100644 --- a/app/templates/components/form.html +++ b/app/templates/components/form.html @@ -4,7 +4,8 @@ autocomplete=False, class=None, id=None, - module=None + module=None, + data_kwargs={} ) %}
{{ caller() }} diff --git a/app/templates/views/templates/choose.html b/app/templates/views/templates/choose.html index 8ff03b7b3..29234898d 100644 --- a/app/templates/views/templates/choose.html +++ b/app/templates/views/templates/choose.html @@ -79,7 +79,10 @@ {{ live_search(target_selector='#template-list .template-list-item', show=show_search_box, form=search_form) }} {% if can_manage_folders %} - {% call form_wrapper(module='template-folder-form') %} + {% call form_wrapper( + module='template-folder-form', + data_kwargs={'prev-state': templates_and_folders_form.op or None} + ) %} {% include 'views/templates/_template_list.html' %} {% include 'views/templates/_move_to.html' %} {% endcall %} From f1dffd1cb83a62964ae81db51f307f4e58e7f465 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 5 Dec 2018 12:01:02 +0000 Subject: [PATCH 10/11] if state is unknown on load, then work out which action buttons to show previously, it'd always show nothing-selected-buttons - however, if some items were checked (due to being selected previously, and loading with a form error message), it would be showing the wrong buttons. Now, if the state is unknown, work out which state to show by counting checkboxes, the same as when someone presses the cancel button. --- app/assets/javascripts/templateFolderForm.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index c09a93599..3b223d424 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -25,8 +25,10 @@ this.states.filter(state => state.cancellable).forEach((x) => this.addCancelButton(x)); // first off show the new template / new folder buttons - let prevState = this.$form.data('prev-state') || 'unknown'; - this.currentState = (prevState === 'unknown') ? 'nothing-selected-buttons' : prevState; + this.currentState = this.$form.data('prev-state') || 'unknown'; + if (this.currentState === 'unknown') { + this.selectActionButtons(); + } this.$form.on('click', 'button.button-secondary', (event) => this.actionButtonClicked(event)); this.$form.on('change', 'input[type=checkbox]', () => this.templateFolderCheckboxChanged()); @@ -43,15 +45,21 @@ state.$el.find('input:radio').prop('checked', false); state.$el.find('input:text').val(''); - // gross hack - pretend we're in the choose actions state, then pretend a checkbox was clicked to work out - // whether to show zero or non-zero options. This calls a render at the end - this.currentState = 'nothing-selected-buttons'; - this.templateFolderCheckboxChanged(); + // go back to action buttons + this.selectActionButtons(); }); state.$el.append($cancel); }; + this.selectActionButtons = function () { + // If we want to show one of the grey choose actions state, we can pretend we're in the choose actions state, + // and then pretend a checkbox was clicked to work out whether to show zero or non-zero options. + // This calls a render at the end + this.currentState = 'nothing-selected-buttons'; + this.templateFolderCheckboxChanged(); + }; + this.actionButtonClicked = function(event) { event.preventDefault(); this.currentState = $(event.currentTarget).val(); From 827b58e7aca8c19e61c3628eab77fd30a5170027 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 5 Dec 2018 13:48:07 +0000 Subject: [PATCH 11/11] remove the unknown button in js When you hit enter while an input in a form is in focus, your browser finds the first button in the form, and carries out that action. So, for non-js users, we added a hidden submit button with a value of "unknown" to reflect that we don't know the intention of the user. However, with JS enabled, this ambiguity doesn't exist - there's only submit button and forms to fill in at a time, and non-visible fields aren't even submitted at all. We can remove the unknown button, supporting enter as submit properly. If the user is on one of the grey button states, with no submit, it'll press the first button, and go to the new template / move to existing folder dialog. That's fine enough. --- app/assets/javascripts/templateFolderForm.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index 3b223d424..335a66115 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -6,6 +6,10 @@ this.start = function(templateFolderForm) { this.$form = $(templateFolderForm); + // remove the hidden unknown button - if you've got JS enabled then the action you want to do is implied by + // which field is visible. + this.$form.find('button[value=unknown]').remove(); + this.$stickyBottom = this.$form.find('#sticky_template_forms'); this.$stickyBottom.append(this.nothingSelectedButtons);