New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bulk editing of mappings #118

Merged
merged 66 commits into from Dec 20, 2013

Conversation

Projects
None yet
4 participants
@jennyd
Contributor

jennyd commented Dec 18, 2013

Opening this to start more visible discussion, since it's a big one.

  • select multiple mappings from a site mappings index page
    • individually without javascript
    • using header checkbox for all or shift-click for ranges with javascript
  • choose to archive or redirect the selected mappings
  • see a pre-save confirmation page or modal, with a new URL input if redirecting
  • if all mappings are successfully updated, redirect to the last-seen site mappings index page
  • if the submitted new URL is invalid, (re)display the confirmation page with all mappings and the error message
  • if some mappings can't be updated for another reason (e.g. they are already invalid, probably because import bypasses validation), redisplay the confirmation page with only the ones which failed

The only acceptance criterion from the story which this doesn't address at all is client-side validation of the new URL input, but we think that is done adequately server-side for now.

Paul Hayes and others added some commits Dec 6, 2013

Create bulk editing table header styles
* Prepare table for form and checkboxes
* Switch add button from green to grey
* Add new table styles to style guide
Create selectable and selected row styles
* Style labels and checkboxes to make entire cell clickable
* Add examples to style guide
* Stub actual pages with the expected HTML
* Move padding helper from hits to base
Select table rows with JavaScript
* Toggle row class and checkbox state on selection
* Toggle all when using the header checkbox
* Use indeterminate state in header when only some of the rows are
selected
* Update style guide example
Select groups of rows using shift click
* Use the previously interacted with checkbox and the newly clicked
checkbox to determine the range
* Currently only toggles on
Don't select all mappings when submitting the form
Specify js interaction using class name so that only clicking the
header checkbox toggles table rows.
Paul Hayes
Handle shift click ranges correctly
* Shift click above and below previously selected rows
* Shift clicking now toggles the range based on the state of the
clicked row, eg. you can now unselect in groups
Paul Hayes
Mark rows as selected on load
* The browser maintains the state of checkboxes when moving
back/forward, our page needs to respond by applying the selected row
class
* The header state should also remain in sync
* Refactor so that shift clicking and normal clicking use the same code
paths
* Toggling a checkbox with shift click when no previous toggles have
been changed should work as normal
Choose archive or redirect for bulk editing
Display new url input on the edit multiple page if redirecting.
Include new_status hidden in the form to post to update.
Save updates to multiple mappings
Various issues remain unresolved, relating to error handling, data
consistency and post-save redirect preserving filter and pagination.
Paul Hayes
Fix radio button default and alignment
* The toggle on the checkbox in the header wasn’t specific enough and
was affecting the default state of the radio buttons
* Update module and tests to use js prefixed class names rather than
elements
*  Tweak alignment of radio buttons by removing padding and adding
margins
Paul Hayes
Explicity uncheck checkbox
* Adding the class attribute introduce a bug wherein the new parameter
evaluated to true and the input became checked.
Paul Hayes
Keep edit multiple table consistent with mappings
* Use same table header text
* Use table header class
Use '301'/'410' for form values
This is a bit simpler and more consistent than using
'redirect'/'archive' in forms.
Fix comment for options_for_supported_statuses
This mappings helper method was changed in 62a5440 and the comment now
matches its existing behaviour.
Paul Hayes
Refactor selectable table into its own file
* Separate table and mappings, avoid grouping behaviour based on where
it’s being used
* Rename spec to use _ to match file conventions
Paul Hayes
Split edit_multiple template into partials
* Break into parts for re-use in a modal
* Create an edit multiple modal
* Modify table slightly based on modal local variable passed in
Paul Hayes
Embolden 'edit selected' button
* Matches styles of other non-input type buttons
Paul Hayes
Return bulk edit table via ajax, display in modal
* Put back previously removed jquery_ujs so that ‘remote: true’ works
as expected
* Use javascript hooks to select an input type and submit the edit
multiple form, displaying the resulting HTML in a bootstrap modal window
* When the modal window is hidden remove it from the dom
* Extend selectable table module to include the wrapping form
* Known issues: Submitting the form without anything selected breaks
the modal. There’s no ajax loading indicator or error handling.
Actually fix comment for options_for_supported_statuses
This still had the official description for the codes, rather than
our more user-friendly ones.
Feature: Editing multiple mappings for a site
As a GDS User,
I want to update many existing mappings at once
so that I can efficiently improve the quailty of mappings

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor

Mmm, quailty 🥚

@jamiecobbett

This comment has been minimized.

Contributor

jamiecobbett commented Dec 18, 2013

As a rule, I'd much prefer to see big things like this broken down. In this case, I might have split it into a non-JS Pull Request and then a JS enhancement.

Paul Hayes
Quality not quailty
* Fowl begone
| 301 | /about/branding | http://gov.uk/branding |
| 410 | /about/corporate | |
And I visit the path /sites/directgov/mappings

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor

The next block's getting a bit too implementation-specific. This will make the tests resistant to change. Comments inline about how we might be less specific about implementation without altering the goal (while also acknowledging that I've been guilty of similar elsewhere and pledging to reform my character):

And I visit the path /sites/directgov/mappings
Scenario: Selecting multiple mappings to redirect without javascript
When I click on the checkboxes for the first and second mappings

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor
  When I select the first two mappings
Scenario: Selecting multiple mappings to redirect without javascript
When I click on the checkboxes for the first and second mappings
And I submit the form with the "Edit selected" button

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor
  And I go to edit my selection
Then the page title should be "Redirect mappings"
And I should see "/a"
And I should see "/about/branding"
And I should have 2 hidden inputs for mapping IDs

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor

This is pure implementation - it might be the only convenient place to test it, but I'm not sure it should be here.

And I should see "/about/branding"
And I should have 2 hidden inputs for mapping IDs
And I should see a "Redirect to" input
But I should not see "/about/corporate"

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor

I might replace all of 18-23 with

  Then I should see an editing form with title "Redirect to" containing only my selections

Things like the "I should see" parts, if they're important, can move into the step. Likewise the testing for hidden input parts can move into the step.

@@ -51,10 +51,17 @@ def mapping_edit_tabs(options = {})
##
# Return a FormBuilder-compatible list of HTTP Status codes with descriptions
# e.g. [['301 Moved Permanently', '301'], ['410 Gone', '410']]
# e.g. [['Archive', '301'], ['Redirect', '410']]

This comment has been minimized.

@jamiecobbett

jamiecobbett Dec 18, 2013

Contributor

This example is the wrong way around

This comment has been minimized.

@jennyd

jennyd Dec 18, 2013

Contributor

D'oh. Third time lucky...

</h3>
</header>
<div class="modal-body">
<%= render partial: 'edit_multiple_table', locals: { modal: true, mappings: @mappings, site: @site, new_url: @new_url, new_url_error: @new_url_error, http_status: @http_status } %>

This comment has been minimized.

@jamiecobbett

jamiecobbett Dec 18, 2013

Contributor

Would it make sense for this to be inside the footer? Then when you have a large list, you wouldn't need to scroll down past them to see the New URL box.

This comment has been minimized.

@fofr

fofr Dec 18, 2013

Contributor

@jennyd and I have discussed switching these two fields around, which I think makes sense

This comment has been minimized.

@jennyd

jennyd Dec 18, 2013

Contributor

Yes, I think that input should be more obvious in the modal - it's easy to miss at the moment.

This comment has been minimized.

@fofr

fofr Dec 18, 2013

Contributor

I've swapped these around, but only in the modal. See 1d78970

expect(mapping_c.new_url).to be_nil
end
it 'redirects to the last-visited site index page' do

This comment has been minimized.

@jamiecobbett

jamiecobbett Dec 18, 2013

Contributor

There are several places in this file where you've said "site index" but you mean "mappings index".

@update_data = { http_status: @http_status }
if @http_status == '301'
@update_data[:new_url] = @new_url = params[:new_url]

This comment has been minimized.

@rgarner

rgarner Dec 18, 2013

Contributor

@update_data isn't used in any view I can see. It should just be a variable, and in the one case it's required elsewhere (bulk_update_mappings) passed as a parameter. To save time, you could cherry-pick 6cd6a6d7b34b844ff0c093a7b605e6826b703e02

This comment has been minimized.

@jennyd

jennyd Dec 18, 2013

Contributor

Thanks 🍒

jennyd and others added some commits Dec 18, 2013

Paul Hayes
Put redirect above table of mappings in modal
* Break the redirect field and logic out of the edit_multiple_table
partial and into its own
* Change the order of the elements when being shown in a modal
Paul Hayes
Fix checkbox & radio non-javascript interactions
* Hide check-all checkbox when we aren’t using JavaScript, it won’t do
anything
* Update the label ‘for’ attribute so that labels are clickable again
Paul Hayes
Make edit features less implementation-specific
* Refer to selecting mappings, not clicking checkboxes
* Refer to editing a selection, not clicking a specific button
* Refer to a form containing a selection, not a list of URLs
* Refer to saving changes, rather than clicking a save button

(Mostly implementing @rgarner’s suggestions)
Get bulk update concerns out of MappingsController
* Helps edit/update_multiple read ok
* May violate SRP slightly, but probably less than we were doing
General tidyings:
* Make sure would_fail_on_url? works not just by coincidence
* Tidy test_mapping
* Whitespace in update_multiple
* Better naming for failed_updates (becomes failures)
* Rename would_fail_on_url? to would_fail_on_new_url?
@jamiecobbett

This comment has been minimized.

Contributor

jamiecobbett commented Dec 20, 2013

Awesome work, @fofr and @jennyd. Its great to have this feature in :)

jamiecobbett added a commit that referenced this pull request Dec 20, 2013

@jamiecobbett jamiecobbett merged commit e291050 into master Dec 20, 2013

@jamiecobbett jamiecobbett deleted the bulk-editing branch Dec 20, 2013

@jamiecobbett

This comment has been minimized.

Contributor

jamiecobbett commented Dec 20, 2013

1,340 lines MERGINATED

@rgarner

This comment has been minimized.

Contributor

rgarner commented Dec 20, 2013

🍖 🍈 📣 📝 🚹

Ok, who moved :merginator:?

@jamiecobbett

This comment has been minimized.

Contributor

jamiecobbett commented Dec 20, 2013

YIPPY-KI-MERGED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment