From 647daa6ca40c07773bf07412c90c6b8b3cbb7eb3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Jun 2019 13:29:28 +0100 Subject: [PATCH 1/5] Make organisation settings platform admin only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment the only setting that a normal organisation team member can change is the name of the organisation is its name. And we don’t even want them to be able to change this. So this commit hides the settings page entirely for non-platform-admin users. --- app/main/views/organisations.py | 4 ++-- .../views/organisations/test_organisation.py | 20 ++++++------------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 58c8e3b48..cefbcdd60 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -180,7 +180,7 @@ def cancel_invited_org_user(org_id, invited_user_id): @main.route("/organisations//settings/", methods=['GET']) @login_required -@user_has_permissions() +@user_is_platform_admin def organisation_settings(org_id): email_branding = 'GOV.UK' @@ -206,7 +206,7 @@ def organisation_settings(org_id): @main.route("/organisations//settings/edit-name", methods=['GET', 'POST']) @login_required -@user_has_permissions() +@user_is_platform_admin def edit_organisation_name(org_id): form = RenameOrganisationForm() diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index e3c2964c6..416ad52e2 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -161,24 +161,16 @@ def test_organisation_trial_mode_services_doesnt_work_if_not_platform_admin( ) -def test_organisation_settings( +def test_organisation_settings_platform_admin_only( client_request, mock_get_organisation, organisation_one ): - expected_rows = [ - 'Label Value Action', - 'Organisation name Org 1 Change', - ] - - page = client_request.get('.organisation_settings', org_id=organisation_one['id']) - - assert page.find('h1').text == 'Settings' - rows = page.select('tr') - assert len(rows) == len(expected_rows) - for index, row in enumerate(expected_rows): - assert row == " ".join(rows[index].text.split()) - mock_get_organisation.assert_called_with(organisation_one['id']) + client_request.get( + '.organisation_settings', + org_id=organisation_one['id'], + _expected_status=403, + ) def test_organisation_settings_for_platform_admin( From 3b22216a681ee8f1d0364d8f57e3593bc7a46f00 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Jun 2019 13:29:37 +0100 Subject: [PATCH 2/5] Remove redundant decorators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@user_has_permissions` is overridden by @user_is_platform_admin – it doesn’t make sense to have them used together. --- app/main/views/organisations.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index cefbcdd60..3a825974d 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -229,7 +229,6 @@ def edit_organisation_name(org_id): @main.route("/organisations//settings/edit-type", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def edit_organisation_type(org_id): @@ -252,7 +251,6 @@ def edit_organisation_type(org_id): @main.route("/organisations//settings/edit-crown-status", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def edit_organisation_crown_status(org_id): @@ -283,7 +281,6 @@ def edit_organisation_crown_status(org_id): @main.route("/organisations//settings/edit-agreement", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def edit_organisation_agreement(org_id): @@ -314,7 +311,6 @@ def edit_organisation_agreement(org_id): @main.route("/organisations//settings/set-email-branding", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def edit_organisation_email_branding(org_id): @@ -341,7 +337,6 @@ def edit_organisation_email_branding(org_id): @main.route("/organisations//settings/preview-email-branding", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def organisation_preview_email_branding(org_id): @@ -412,7 +407,6 @@ def organisation_preview_letter_branding(org_id): @main.route("/organisations//settings/edit-organisation-domains", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def edit_organisation_domains(org_id): @@ -471,7 +465,6 @@ def confirm_edit_organisation_name(org_id): @main.route("/organisations//settings/edit-go-live-notes", methods=['GET', 'POST']) @login_required -@user_has_permissions() @user_is_platform_admin def edit_organisation_go_live_notes(org_id): From 80e6c1654b1f82857f12e8967246d3fd7d6d97c5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Jun 2019 13:31:45 +0100 Subject: [PATCH 3/5] Make organisation settings one table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s no longer a difference between regular settings and platform admin settings – it’s all platform admin. --- .../organisation/settings/index.html | 178 ++++++++---------- .../views/organisations/test_organisation.py | 2 - 2 files changed, 82 insertions(+), 98 deletions(-) diff --git a/app/templates/views/organisations/organisation/settings/index.html b/app/templates/views/organisations/organisation/settings/index.html index aa440f700..6f2ae3d75 100644 --- a/app/templates/views/organisations/organisation/settings/index.html +++ b/app/templates/views/organisations/organisation/settings/index.html @@ -23,102 +23,88 @@ ) }} {% endcall %} + {% call row() %} + {{ text_field('Organisation type') }} + {{ optional_text_field({ + 'central': 'Central government', + 'local': 'Local government', + 'nhs': 'NHS', + }.get(current_org.organisation_type)) }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_type', org_id=current_org.id) + ) + }} + {% endcall %} + {% call row() %} + {{ text_field('Crown organisation') }} + {{ optional_text_field( + { + True: 'Yes', + False: 'No', + }.get(current_org.crown), + default='Not sure' + ) }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_crown_status', org_id=current_org.id) + ) + }} + {% endcall %} + {% call row() %} + {{ text_field('Data sharing and financial agreement') }} + {{ text_field( + { + True: 'Signed', + False: 'Not signed', + None: 'Not signed (but we have some service-specific agreements in place)' + }.get(current_org.agreement_signed) + ) }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_agreement', org_id=current_org.id) + ) + }} + {% endcall %} + {% call row() %} + {{ text_field('Request to go live notes') }} + {{ optional_text_field(current_org.request_to_go_live_notes, default='None') }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_go_live_notes', org_id=current_org.id) + ) + }} + {% endcall %} + {% call row() %} + {{ text_field('Default email branding') }} + {{ text_field(email_branding) }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_email_branding', org_id=current_org.id) + ) + }} + {% endcall %} + {% call row() %} + {{ text_field('Default letter branding') }} + {{ optional_text_field( + letter_branding, + default='No branding' + ) }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_letter_branding', org_id=current_org.id) + ) + }} + {% endcall %} + {% call row() %} + {{ text_field('Known email domains') }} + {{ optional_text_field(current_org.domains or None, default='None') }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_domains', org_id=current_org.id) + ) + }} + {% endcall %} {% endcall %} - - - {% if current_user.platform_admin %} -

Platform admin settings

-
- {% call mapping_table( - caption='Platform admin settings', - field_headings=['Label', 'Value', 'Action'], - field_headings_visible=False, - caption_visible=False - ) %} - {% call row() %} - {{ text_field('Organisation type') }} - {{ optional_text_field({ - 'central': 'Central government', - 'local': 'Local government', - 'nhs': 'NHS', - }.get(current_org.organisation_type)) }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_type', org_id=current_org.id) - ) - }} - {% endcall %} - {% call row() %} - {{ text_field('Crown organisation') }} - {{ optional_text_field( - { - True: 'Yes', - False: 'No', - }.get(current_org.crown), - default='Not sure' - ) }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_crown_status', org_id=current_org.id) - ) - }} - {% endcall %} - {% call row() %} - {{ text_field('Data sharing and financial agreement') }} - {{ text_field( - { - True: 'Signed', - False: 'Not signed', - None: 'Not signed (but we have some service-specific agreements in place)' - }.get(current_org.agreement_signed) - ) }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_agreement', org_id=current_org.id) - ) - }} - {% endcall %} - {% call row() %} - {{ text_field('Request to go live notes') }} - {{ optional_text_field(current_org.request_to_go_live_notes, default='None') }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_go_live_notes', org_id=current_org.id) - ) - }} - {% endcall %} - {% call row() %} - {{ text_field('Default email branding') }} - {{ text_field(email_branding) }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_email_branding', org_id=current_org.id) - ) - }} - {% endcall %} - {% call row() %} - {{ text_field('Default letter branding') }} - {{ optional_text_field( - letter_branding, - default='No branding' - ) }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_letter_branding', org_id=current_org.id) - ) - }} - {% endcall %} - {% call row() %} - {{ text_field('Known email domains') }} - {{ optional_text_field(current_org.domains or None, default='None') }} - {{ edit_field( - 'Change', - url_for('.edit_organisation_domains', org_id=current_org.id) - ) - }} - {% endcall %} - {% endcall %} -
- {% endif %} {% endblock %} diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index 416ad52e2..eb8a7093f 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -182,8 +182,6 @@ def test_organisation_settings_for_platform_admin( expected_rows = [ 'Label Value Action', 'Organisation name Org 1 Change', - - 'Label Value Action', 'Organisation type Not set Change', 'Crown organisation Yes Change', 'Data sharing and financial agreement Not signed Change', From 6fc163526652124b5cdfb7c19877eba240efc935 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Jun 2019 13:34:50 +0100 Subject: [PATCH 4/5] Rename some of the settings This reflects the language we use for these things when talking about them amongst ourselves. --- .../organisation/settings/edit-type.html | 17 +++++++---------- .../organisation/settings/index.html | 4 ++-- .../views/organisations/test_organisation.py | 4 ++-- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/app/templates/views/organisations/organisation/settings/edit-type.html b/app/templates/views/organisations/organisation/settings/edit-type.html index f362e8565..46d34cf6f 100644 --- a/app/templates/views/organisations/organisation/settings/edit-type.html +++ b/app/templates/views/organisations/organisation/settings/edit-type.html @@ -1,21 +1,18 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/form.html" import form_wrapper %} {% extends "org_template.html" %} {% block org_page_title %} - Organisation type + Organisation sector {% endblock %} {% block maincolumn_content %} -

Organisation type

-
-
- {% call form_wrapper() %} - {{ radios(form.organisation_type) }} - {{ page_footer('Save') }} - {% endcall %} -
-
+ {{ page_header("Organisation sector") }} + {% call form_wrapper() %} + {{ radios(form.organisation_type) }} + {{ page_footer('Save') }} + {% endcall %} {% endblock %} diff --git a/app/templates/views/organisations/organisation/settings/index.html b/app/templates/views/organisations/organisation/settings/index.html index 6f2ae3d75..5708deeed 100644 --- a/app/templates/views/organisations/organisation/settings/index.html +++ b/app/templates/views/organisations/organisation/settings/index.html @@ -15,7 +15,7 @@ caption_visible=False ) %} {% call row() %} - {{ text_field('Organisation name') }} + {{ text_field('Name') }} {{ text_field(current_org.name) }} {{ edit_field( 'Change', @@ -24,7 +24,7 @@ }} {% endcall %} {% call row() %} - {{ text_field('Organisation type') }} + {{ text_field('Sector') }} {{ optional_text_field({ 'central': 'Central government', 'local': 'Local government', diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index eb8a7093f..afd7f3dc3 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -181,8 +181,8 @@ def test_organisation_settings_for_platform_admin( ): expected_rows = [ 'Label Value Action', - 'Organisation name Org 1 Change', - 'Organisation type Not set Change', + 'Name Org 1 Change', + 'Sector Not set Change', 'Crown organisation Yes Change', 'Data sharing and financial agreement Not signed Change', 'Request to go live notes None Change', From ee5212941cd35ee0d3e4aade63058344bd404850 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Jun 2019 13:39:25 +0100 Subject: [PATCH 5/5] Add back links to organisation settings pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They’re nice. --- .../organisation/settings/edit-agreement.html | 6 +++++- .../organisation/settings/edit-crown-status.html | 6 +++++- .../organisations/organisation/settings/edit-domains.html | 6 +++++- .../organisation/settings/edit-go-live-notes.html | 6 +++++- .../organisations/organisation/settings/edit-type.html | 5 ++++- .../organisation/settings/set-email-branding.html | 6 +++++- .../organisation/settings/set-letter-branding.html | 6 +++++- .../organisations/organisation/users/invite-org-user.html | 8 +++++--- 8 files changed, 39 insertions(+), 10 deletions(-) diff --git a/app/templates/views/organisations/organisation/settings/edit-agreement.html b/app/templates/views/organisations/organisation/settings/edit-agreement.html index 76f587cb9..05fb63c30 100644 --- a/app/templates/views/organisations/organisation/settings/edit-agreement.html +++ b/app/templates/views/organisations/organisation/settings/edit-agreement.html @@ -1,5 +1,6 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/form.html" import form_wrapper %} {% extends "org_template.html" %} @@ -9,7 +10,10 @@ {% endblock %} {% block maincolumn_content %} -

Data sharing and financial agreement

+ {{ page_header( + "Data sharing and financial agreement", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }}
{% call form_wrapper() %} diff --git a/app/templates/views/organisations/organisation/settings/edit-crown-status.html b/app/templates/views/organisations/organisation/settings/edit-crown-status.html index 795cf12c3..b53bb1252 100644 --- a/app/templates/views/organisations/organisation/settings/edit-crown-status.html +++ b/app/templates/views/organisations/organisation/settings/edit-crown-status.html @@ -1,5 +1,6 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/form.html" import form_wrapper %} {% extends "org_template.html" %} @@ -9,7 +10,10 @@ {% endblock %} {% block maincolumn_content %} -

Crown organisation

+ {{ page_header( + "Crown organisation", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }}
{% call form_wrapper() %} diff --git a/app/templates/views/organisations/organisation/settings/edit-domains.html b/app/templates/views/organisations/organisation/settings/edit-domains.html index 295cd417d..7407be3ae 100644 --- a/app/templates/views/organisations/organisation/settings/edit-domains.html +++ b/app/templates/views/organisations/organisation/settings/edit-domains.html @@ -1,5 +1,6 @@ {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/list-entry.html" import list_entry %} {% from "components/form.html" import form_wrapper %} @@ -10,7 +11,10 @@ {% endblock %} {% block maincolumn_content %} -

Known email domains

+ {{ page_header( + "Known email domains", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }}

diff --git a/app/templates/views/organisations/organisation/settings/edit-go-live-notes.html b/app/templates/views/organisations/organisation/settings/edit-go-live-notes.html index 67f4143f8..e53e6db0e 100644 --- a/app/templates/views/organisations/organisation/settings/edit-go-live-notes.html +++ b/app/templates/views/organisations/organisation/settings/edit-go-live-notes.html @@ -1,5 +1,6 @@ {% from "components/form.html" import form_wrapper %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/textbox.html" import textbox %} {% extends "org_template.html" %} @@ -9,7 +10,10 @@ {% endblock %} {% block maincolumn_content %} -

Edit request to go live notes

+ {{ page_header( + "Edit request to go live notes", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }}

diff --git a/app/templates/views/organisations/organisation/settings/edit-type.html b/app/templates/views/organisations/organisation/settings/edit-type.html index 46d34cf6f..4ce32d672 100644 --- a/app/templates/views/organisations/organisation/settings/edit-type.html +++ b/app/templates/views/organisations/organisation/settings/edit-type.html @@ -10,7 +10,10 @@ {% endblock %} {% block maincolumn_content %} - {{ page_header("Organisation sector") }} + {{ page_header( + "Organisation sector", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }} {% call form_wrapper() %} {{ radios(form.organisation_type) }} {{ page_footer('Save') }} diff --git a/app/templates/views/organisations/organisation/settings/set-email-branding.html b/app/templates/views/organisations/organisation/settings/set-email-branding.html index 71034e51d..e283cf224 100644 --- a/app/templates/views/organisations/organisation/settings/set-email-branding.html +++ b/app/templates/views/organisations/organisation/settings/set-email-branding.html @@ -1,6 +1,7 @@ {% extends "org_template.html" %} {% from "components/radios.html" import radios %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/live-search.html" import live_search %} {% from "components/form.html" import form_wrapper %} @@ -10,7 +11,10 @@ {% block maincolumn_content %} -

Default email branding

+ {{ page_header( + "Default email branding", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }} {% call form_wrapper(data_kwargs={'preview-type': 'email'}) %}
diff --git a/app/templates/views/organisations/organisation/settings/set-letter-branding.html b/app/templates/views/organisations/organisation/settings/set-letter-branding.html index d8c157798..5fd8d20fe 100644 --- a/app/templates/views/organisations/organisation/settings/set-letter-branding.html +++ b/app/templates/views/organisations/organisation/settings/set-letter-branding.html @@ -2,6 +2,7 @@ {% from "components/radios.html" import radios %} {% from "components/live-search.html" import live_search %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/form.html" import form_wrapper %} {% block org_page_title %} @@ -10,7 +11,10 @@ {% block maincolumn_content %} -

Default letter branding

+ {{ page_header( + "Default letter branding", + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }} {% call form_wrapper(data_kwargs={'preview-type': 'letter'}) %}
diff --git a/app/templates/views/organisations/organisation/users/invite-org-user.html b/app/templates/views/organisations/organisation/users/invite-org-user.html index ac54d27d4..14fb4dbe3 100644 --- a/app/templates/views/organisations/organisation/users/invite-org-user.html +++ b/app/templates/views/organisations/organisation/users/invite-org-user.html @@ -2,6 +2,7 @@ {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} {% from "components/form.html" import form_wrapper %} {% block service_page_title %} @@ -10,9 +11,10 @@ {% block maincolumn_content %} -

- Invite a team member -

+ {{ page_header( + "Invite a team member", + back_link=url_for('.manage_org_users', org_id=current_org.id) + ) }}
{% call form_wrapper(class="column-three-quarters") %} {{ textbox(form.email_address, width='1-1', safe_error_message=True) }}