Move users index page to design system #2345
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've worked out why Capybara can't see the checkboxes. The The visual box is generated by a You can see the .govuk-checkboxes__input {
opacity: 1;
position: relative;
}
One solution for a JS-enabled integration test is to "click" the As an aside: I haven't investigated this completely, but I'm pretty sure if assets have previously been precompiled (e.g. by having just run a test), I don't think they are re-compiled when you re-run the test. So if you make a change to an asset, it's worth running |
I recently came across this gem which might make writing assertions against the table of users in an integration test much easier/readable. |
|
I just came across the ‘allow_label_click’ option for ‘Capybara::Node::Actions#check’ which might be a better option than finding and clicking the label… There’s also an ‘automatic_label_click’ configuration option (false by default) which might be worth setting to true… |
chrisroos
force-pushed
the
move-users-index-page-to-design-system
branch
2 times, most recently
from
6a503fc to
836b788
Compare
This method is nothing to do with filtering and it's only used in the users index page, so the UsersHelper seems like a better home.
This method is nothing to do with the filtering that the rest of this helper is concerned with. This method is only used in the app/views/users/_form_fields.html.erb partial template which allows setting or updating a user's role; whereas all the others are used in the filtering of users in the users index page.
This method is nothing to do with the filtering that the rest of this helper is concerned with. This method is only used in the app/views/users/_form_fields.html.erb partial template which allows setting or updating a user's role; whereas all the others are used in the filtering of users in the users index page.
Trello: https://trello.com/c/2gvEJKw9 This filtering functionality doesn't comform to the Design Sytem so we're going to have to re-implement it so that it does. As a first step I'm removing the old implementation which should hopefully make the addition of the new functionality easier to follow. I've inlined UserFilterHelper#user_role_text to a hard-coded string, because it no longer varies according to the filtered role.
Trello: https://trello.com/c/2gvEJKw9 The new design does not include this button and I've confirmed that its removal is intentional. It is still possible to unlock accounts from the user edit page.
Trello: https://trello.com/c/2gvEJKw9 The new design does not include this button and I've confirmed that its removal is intentional. It is still possible to resend a signup email from the user edit page.
Trello: https://trello.com/c/2gvEJKw9 The layout is a bit noddy, but my aim was to port as much of the content & functionality of the page as possible to use the GOV.UK Design System before then iterating towards the proposed design for the page.
This method is using the @users relation assigned in UsersController#index which already has includes(:organisation) on it.
Trello: https://trello.com/c/2gvEJKw9 This is a first step towards implementing the sidebar filtering in the new proposed design. It's loosely based on Whitehall's Admin::EditionsFilter [1]. I haven't actually implemented any filtering in this commit - I've just moved the ordering and pagination into the new UsersFilter in preparation for implementing filtering in the UsersFilter#users method based on the options set from the params. [1]: https://github.com/alphagov/whitehall/blob/8ddae783224de0c4889dbea0e3d049649287ee75/app/models/admin/edition_filter.rb
Previously the magic number, 3, in the assertion related to (a) the admin user created in the setup; (b) the user created in the test; and (c) the header row. Now we parse the CSV so we don't have to consider the header row and we use User.count to make it clear that we're including all users, i.e. in this case both the admin user and the normal user.
Trello: https://trello.com/c/2gvEJKw9 This adds a search input field to the filter sidebar. When the user enters a value in that input field and clicks either the integrated search button or the form "Update results" button, the list of users is restricted to only those which have a name or email partially matching the search input value. I've tried to give the new User scopes more intention-revealing names than the previous implementation. And I've tried to make the new implementation of the scopes more idiomatic Rails. Unlike the previous implementation of the scopes, I'm not stripping the supplied string, because I think this it's more appropriate for the caller to handle this, i.e. UsersFilter#users in this case. I considered wrapping the supplied string in a call to ActiveRecord::Sanitization::ClassMethods#sanitize_sql_like to escape underscores & percent signs. However, in the end I decided against doing this because it would be a change from the previous behaviour and in any case I can imagine it might be useful to use these wildcard characters in searches. I've used the slightly confusing "filter" param name, because that's what was used in the previous implementation and I didn't want to unnecessarily break any bookmarked URLs.
Trello: https://trello.com/c/2gvEJKw9 I've wrapped this in a button group with the "Update results" button so that the link appears alongside the button in wider screen widths, but below the button in narrower screen widths. I've added the govuk-link--no-visited-state CSS class to the link, because that's what we've done elsewhere - it's not obvious it's useful to highlight that the user has clicked on this link previously.
The copy was made from the finder-frontend repo at this commit [1]. We're planning to use this component on the users index page for the filter functionality in the proposed design. I haven't copied any of the specs, because I would've had to translate the RSpec spec into Minitest and I wasn't sure the Jasmine test would just work. Since the plan is to extract this component into the govuk_publishing_components gem, it didn't seem worth the effort. [1]: https://github.com/alphagov/finder-frontend/tree/38758e110bf8397c64b2143771794318af9d25b5
I preferred the constant lookup approach used in UserPolicy#can_manage? over that used in User#manageable_roles, so I've used that in the new method.
It's not obvious that the former was adding much value.
Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected roles. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single role. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `roles` as the parameter name to make it clear that more than one role can be selected and to distinguish it from the previous implementation which used `role`. We might want to consider redirecting URLs using `role` to use `roles` if we want bookmarked URLs to continue to work as expected. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4
This is similar to the User#manageable_roles method. I plan to use it in subsequent commits to make it clearer what a user with a given role is allowed to do.
Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which belong to one of the selected organisations. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single organisation. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `organisations` as the parameter name to make it clear that more than one organisation can be selected and to distinguish it from the previous implementation which used `organisation`. We might want to consider redirecting URLs using `organisation` to use `organisations` if we want bookmarked URLs to continue to work as expected. The option-select component is only displayed if the current user is allowed to manage more than one organisation. I've made this more explicit than the original implementation by using User#manageable_organisations in UsersFilter#organisation_option_select_options. If the current user is allowed to manage exactly one organisation (i.e. they are an organisation admin) then the filtered users heading is qualified with the relevant organisation, e.g. "399 users in Government Digital Service". This is because the organisation option select will not be displayed in this scenario so we need to make it clear which organisation the users in the table belong to. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4
Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected permissions. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single permission. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `permissions` as the parameter name to make it clear that more than one permission can be selected and to distinguish it from the previous implementation which used `permission`. We might want to consider redirecting URLs using `permission` to use `permissions` if we want bookmarked URLs to continue to work as expected. I've used arel_table to generate the column name in the existing User.with_partially_matching_name & User.with_partially_matching_email scopes so that they work in conjunction with the new User.with_permission scope, i.e. to avoid ambiguity in column names. I've tried to make the implementation of UsersHelper#permission_option_select_options a bit more idioomatic than the original version in UserFilterHelper#user_filter_list_items: * The new implementation doesn't specify an order, because we can rely on the default scope order of Doorkeeper::Application & SupportedPermission. * I didn't feel as if plucking values made the query significantly more efficient and it cluttered up the code. * Using a map loop within a flat_map loop allowed me to use instances of Doorkeeper::Application & SupportedPermission which made the code a bit simpler / more readable. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4
Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected statuses. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single status. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `statuses` as the parameter name to make it clear that more than one status can be selected and to distinguish it from the previous implementation which used `status`. We might want to consider redirecting URLs using `status` to use `statuses` if we want bookmarked URLs to continue to work as expected. Unlike for the previous implementation of User.with_status which used a case statement to return the relevant un-named scope, for User.with_statuses I've implemented a bunch of named scopes which make it easier to combine them programmatically using ActiveRecord::QueryMethods#or. This makes the code easier to test and I think it makes it easier to read. I'm hoping we might be able to re-use the named scopes elsewhere in the code. I've also introduced some new user factories for the various user "states". Note that the new User.locked scope perpetuates the issue where we don't consider Devise's time-based unlock strategy, i.e. where User#access_locked? may return false when the user is still included in the User.locked scope because User#locked_at is not nil. This problem is captured in this Trello card [2]. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4 [2]: https://trello.com/c/88jMyKuf
This is more consistent with how User#status works and it will allow me to reuse the new User::TWO_STEP_STATUS_*** constants in a subsequent commit to avoid duplication.
Trello: https://trello.com/c/2gvEJKw9 This adds an instance of the new option-select component to the filter sidebar. When the user selects one or more or the checkboxes and clicks the form "Update results" button, the list of users is restricted to only those which have one of the selected 2SV statuses. This represents a change from the orginal filtering implementation which only allowed the user to filter by a single 2SV status. I have confirmed [1] that this is an intentional change and not just an accidental implication of the proposed design. I've used `two_step_statuses` as the parameter name to make it clear that more than one 2SV status can be selected and to distinguish it from the previous implementation which used `two_step_status`. We might want to consider redirecting URLs using `two_step_status` to use `two_step_statuses` if we want bookmarked URLs to continue to work as expected. Unlike for the previous implementation of User.with_2sv_enabled which used a case statement to return the relevant un-named scope, for User.with_2sv_statuses I've implemented a bunch of named scopes which make it easier to combine them programmatically using ActiveRecord::QueryMethods#or. This makes the code easier to test and I think it makes it easier to read. I'm hoping we might be able to re-use the named scopes elsewhere in the code. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64e5f080394409b567e5bcb4
Trello: https://trello.com/c/2gvEJKw9 As per the propposed design.
I was seeing the following warning in the log when updating the filters
on the users page:
Unpermitted parameter: :option-select-filter
It seems as if the options-select component adds an input with this
name, but we don't need it in our implementation, so I've chosen to
accept it and then immediately discard it.
This makes the option-select components on the users page "open" if any of its options are selected. The option-select for statuses will also "open" if none of the option-select components have any options selected - this is to make it clearer to users what's hidden away in the "closed". I think this will also make things less confusing when we add JS to submit the form when any of the checkboxes changes state.
There are a lot of options in the option-select component for organisations & permissions on the users index page. While it's easy enough to find a specific option using the search box (displayed because show_filter is set to true), it's not so easy to find the options that have already been selected unless you can remember the option label. Putting the selected options above all the unselected options should make things much easier.
The filters on the users index page now allow multiple options to be selected and the param keys have changed from singular to plural. Also the values for the two-step status filter have changed to match the relevant User scopes. This change redirects requests to the users index page that use the legacy filter params. This is in case anyone has bookmarked some of these URLs. Note that the redirection won't handle all URLs containing a mixture of legacy and non-legacy params, but this seems like a real edge case that's not worth worrying about. It might be worth adding a flash message to instruct users to update their bookmark so that we can safely remove this functionality in the not too distant future, but I haven't done that here.
This introduces a new AutoSubmitForm JS module which is activated for the form on the users index page. This JS automatically submits the form whenever the form's change event fires, i.e. whenever one of the filter checkboxes is checked/unchecked. The "auto-submit-ignore" data attribute should be a comma-separated list of input names for which to ignore the form's change event. We use this for the "option-select-filter" text inputs which are dynamically added to the option-select component when `show_filter` is set to `true`. The values of those text inputs are not used on the server-side and so we don't want to auto-submit the form when they change.
This was added in the commit [1] where the layout was originally added. It sounds as if it was all just copied from Content Publisher, so I don't think there's a strong reason to add it until we actually need it. [1]: 5173710
As per the proposed design [1]. I'm not particularly happy with the changes to the admin_layout. I think the real problem is that we've got too much of the page in the layout and we can probably come up with a better way to avoid duplication. However, I haven't attempted to address that here, because it would be a significant chunk of work and this PR is already big enough! Since the container isn't using a flex layout as suggested by the prototype, I had to use `float: right` to position the button group to the right of the page on wider screens. I'm not 100% sure whether I've used the correct naming conventions in the naming of the "users-index-button-group" CSS class. [1]: https://trello.com/c/bXsVUK6c
As per the proposed design [1]. [1]: https://trello.com/c/bXsVUK6c
This means the table is now much more "responsive" i.e. it looks a lot better on mobile devices. I've fixed the UsersControllerTest assertions against table cells by using regular expressions c.f. the equivalent commit for the API users index page [1]. [1]: f0a8f53
As agreed in the collaboration session on Wed 06 Sep [1], we're removing these columns in order to reduce the chances of the table overflowing the right-hand side of the page in screens wider than the "tablet" breakpoint, i.e. when the table is still in "horizontal" mode. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64f87c053ccc070a1fae1821
As agreed in the collaboration session on Wed 06 Sep [1], we're increasing the value of `$govuk-page-width` from the default of 960px to 1140px which is the value used in Whitehall [2]. We're making this change in order to give more room to the table on the new version of the users index page which now has a filter sidebar occupying valuable screen wdith. This will reduce the chances of the table overflowing the right-hand side of the page in screens wider than the "tablet" breakpoint, i.e. when the table is still in "horizontal" mode. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64f87c053ccc070a1fae1821 [2]: https://github.com/alphagov/whitehall/blob/b355ef2caf4b414487d3687495d133b66f7f7846/app/assets/stylesheets/application.scss#L1
As agreed in the collaboration session on Wed 06 Sep [1], we're increasing the value of this breakpoint from the standard "tablet" breakpoint of 641px to a custom breakpoint of 940px. We chose this value so that the table changes into "vertical" mode before the table content overflows the right-hand side of the container and before a scrollbar appears at the bottom of the page. This change also affects the API users index page, but seems to work OK there too. [1]: https://trello.com/c/2gvEJKw9/207-migrate-users-page-to-use-design-system#comment-64f87c053ccc070a1fae1821
Ideally we'd re-write the git history, but I'm out of time!
chrisroos
force-pushed
the
move-users-index-page-to-design-system
branch
2 times, most recently
from
03dfb34 to
e54f70f
Compare
I've created a Google Doc of before/after screenshots and linked to them from the Trello card rather. |
I've added JavaScript enabled integration tests to test the JavaScript filter functionality. |
Thanks, @floehopper! I'm using this in the integration tests I've added. |
I've confirmed that the order of the Roles filter is the same before/after this PR for the different user roles. |
The equivalent non-JS functionality is already being tested in controller and unit tests. These additional happy path tests should give us confidence that the JavaScript functionality continues to work as expected.
chrisroos
force-pushed
the
move-users-index-page-to-design-system
branch
from
e54f70f to
201e2b7
Compare
chrisroos
approved these changes
Sep 12, 2023
There was a problem hiding this comment.
I've read through the changes in this PR a couple of times, added some JS-enabled tests and played around with the functionality in development. It all looks good to me! Good work, @floehopper!
|
I've merged this in @floehopper's absence. |
I notified 2ndline, gave them an overview of the changes and shared a link to the screenshots. |
I decided not to do this because it would've taken longer to extract them than it would've done to merge them all in together. |
I decided not to do this. If it comes up again then we can make the change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/2gvEJKw9 & https://trello.com/c/bXsVUK6c
Summary
The main motivation for this work is to convert the users index page to use the GOV.UK Design System instead of govuk_admin_template and improve the accessibility & responsiveness of the page.
Description
The new version of the page adds checkbox filters based on the option-select component from finder-frontend (e.g. Research and statistics) in a sidebar.
The new filters allow users to select multiple values per filter whereas previously it was only possible to select a single value per filter. In order to implement this new behaviour I've refactored/reimplemented a bunch of scopes & methods on the
Usermodel. In doing so I hope they're now more idiomatic and could possible already be used elsewhere to make the code more intention-revealing. URLs with query strings generated from the old filters will be redirected to query strings compatible with the new filters - this means that any bookmarked URLs should still work.A bit like the old version of the page, the "Organisations" filter is not displayed if the current user can only "manage" a single organisation (i.e. Organisation Admins); instead the table caption is amended to make it clear the users in the table are all associated with a given organisation.
We're using JS to auto-submit the filter form when checkboxes are checked/unchecked (triggering a full page reload) rather than implementing a fully-Ajaxified solution like that in the finder-frontend page(s). In order to reduce any potential confusion caused by the full page reload we're doing the following:
We chose to add the JS behaviour described above, because the "Update results" button can easily disappear off the bottom of the browser viewport and, in any case, it's very similar to how the old version of the page worked. The page should also work fine without JS enabled - it's just that the filters are all always "open" and the user will have to click the "Update results" button to apply the filters. This feels like a siginificant improvement on the old version of the page which effectively had two fairly distinct implementations (one when JS was enabled; and one when it was not).
The introduction of the filters sidebar has significantly reduced the page width available for the table which was already overflowing the main container on screens narrower than "desktop" size. In order to avoid this overflow problem, we've had to make some compromises on what data is displayed in the table: we've removed the "Actions" column which used to display "Unlock account" & "Resend signup email" buttons - these actions are still available from the user edit page; and we've removed the "Organisation" and "Last sign in" columns.
To further address this overflow problem and to improve the "responsiveness" of the page, we've also made the following changes:
max-widthofgovuk-width-containerfor all pages in the app following a precendent set in whitehallvertical_on_small_screenvariation of the table component to "pivot" the table on narrow screensThe "Export users as CSV" button has moved to the bottom of the filters sidebar and its now styled as a link. The "Upload a batch of users" & "Grant access to all users" buttons have become secondary buttons.
To do
filter: blur(4px)to blur the PII columns ("Name" & "Email") in screenshots that appear in public forums like PRs.This is a bit of a concern because it might be an indication that some screen-readers might also have problems.I'll add some notes about where I got to with this below. [Update: I got to the bottom of this - see this comment]Superadmin.manageable_rolesis implemented differently than in the other role classes and it returns roles in the opposite order. It would be nice to make all the implementations of.manageable_rolesreturn roles in the same order, perhaps making use of thelevelattribute...?Notes on the problem with JS-enabled integration tests
use_javascript_driverin the test. Note that calling this resets the session, so you may need to call it in the test setup if that's where the user signs in.save_screenshot(Rails.root.join("screenshot.png")). By defaultsave_screenshot()saves the screenshot in the projecttmpdirectory which is not accessible from outside thegovuk-dockercontainer by default. Saving the screenshot to the project root directory means that it is accessible.current_window.resize_to(1920, 1080).check "Active", visible: false.Selenium::WebDriver.logger.level = :debug; Selenium::WebDriver.logger.output = Rails.root.join("selenium.log")../descendant::input[(./@type = 'checkbox')][((((./@id = 'Active') or (./@name = 'Active')) or (./@placeholder = 'Active')) or (./@id = //label[contains(normalize-space(string(.)), 'Active')]/@for))] | .//label[contains(normalize-space(string(.)), 'Active')]//./descendant::input[(./@type = 'checkbox')]and this returned a value like this:[{"element-6066-11e4-a52e-4f735466cecf":"849902D65C2D92EADB74FC56B1B7247E_element_22"}].isDisplayedand it returned a value offalse. This seems to correspond to this documentation and possibly this function. The complex and minified/obfuscated nature of the function meant that I didn't take this any further, but it might be worth trying to invoke the function in a Chrome browser...?option-selectcomponent uses thecheckboxescomponent and thence thecheckboxcomponent to render the individual checkboxes in the filters. I tried changing theoption-selectcomponent to generate the checkboxes directly using HTML/ERB and the test successfully checked these. So I think the problem must lie somewhere in thecheckboxesand/orcheckboxcomponent. However, that's when I ran out of time - sorry!