From e32cb5df31932774fa980410cfc953f1b084d618 Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 6 Mar 2018 17:12:31 +0000 Subject: [PATCH] update organisation name --- app/main/forms.py | 8 + app/main/views/organisations.py | 101 +++++--- app/notify_client/organisations_api_client.py | 8 +- app/templates/org_nav.html | 1 + .../settings/edit-name/confirm.html | 28 +++ .../settings/edit-name/index.html | 21 ++ .../organisation/settings/index.html | 27 +++ .../organisation/update-organisation.html | 25 -- tests/app/main/views/test_organisations.py | 220 ++++++++++++++---- tests/conftest.py | 18 ++ 10 files changed, 354 insertions(+), 103 deletions(-) create mode 100644 app/templates/views/organisations/organisation/settings/edit-name/confirm.html create mode 100644 app/templates/views/organisations/organisation/settings/edit-name/index.html create mode 100644 app/templates/views/organisations/organisation/settings/index.html delete mode 100644 app/templates/views/organisations/organisation/update-organisation.html diff --git a/app/main/forms.py b/app/main/forms.py index 77b228fe7..94d28282f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -329,6 +329,14 @@ class RenameServiceForm(StripWhitespaceForm): ]) +class RenameOrganisationForm(StripWhitespaceForm): + name = StringField( + u'Organisation name', + validators=[ + DataRequired(message='Can’t be empty') + ]) + + class CreateServiceForm(StripWhitespaceForm): name = StringField( u'What’s your service called?', diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index f8b193689..570ab806b 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -1,13 +1,20 @@ -from flask import flash, redirect, render_template, request, url_for +from flask import flash, redirect, render_template, request, session, url_for from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError from werkzeug.exceptions import abort -from app import org_invite_api_client, organisations_client, user_api_client +from app import ( + current_organisation, + org_invite_api_client, + organisations_client, + user_api_client, +) from app.main import main from app.main.forms import ( + ConfirmPasswordForm, CreateOrUpdateOrganisation, InviteOrgUserForm, + RenameOrganisationForm, SearchUsersForm, ) from app.utils import user_is_platform_admin @@ -56,31 +63,6 @@ def organisation_dashboard(org_id): ) -@main.route("/organisations//edit", methods=['GET', 'POST']) -@login_required -@user_is_platform_admin -def update_organisation(org_id): - org = organisations_client.get_organisation(org_id) - - form = CreateOrUpdateOrganisation() - - if form.validate_on_submit(): - organisations_client.update_organisation( - org_id=org_id, - name=form.name.data - ) - - return redirect(url_for('.organisations')) - - form.name.data = org['name'] - - return render_template( - 'views/organisations/organisation/update-organisation.html', - form=form, - organisation=org - ) - - @main.route("/organisations//users", methods=['GET']) @login_required @user_is_platform_admin @@ -174,3 +156,68 @@ def cancel_invited_org_user(org_id, invited_user_id): org_invite_api_client.cancel_invited_user(org_id=org_id, invited_user_id=invited_user_id) return redirect(url_for('main.manage_org_users', org_id=org_id)) + + +@main.route("/organisations//settings/", methods=['GET']) +@login_required +@user_is_platform_admin +def organisation_settings(org_id): + return render_template( + 'views/organisations/organisation/settings/index.html', + ) + + +@main.route("/organisations//settings/edit-name", methods=['GET', 'POST']) +@login_required +@user_is_platform_admin +def edit_organisation_name(org_id): + form = RenameOrganisationForm() + + if request.method == 'GET': + form.name.data = current_organisation.get('name') + + if form.validate_on_submit(): + unique_name = organisations_client.is_organisation_name_unique(org_id, form.name.data) + if not unique_name: + form.name.errors.append("This organisation name is already in use") + return render_template('views/organisations/organisation/settings/edit-name/index.html', form=form) + session['organisation_name_change'] = form.name.data + return redirect(url_for('.confirm_edit_organisation_name', org_id=org_id)) + + return render_template( + 'views/organisations/organisation/settings/edit-name/index.html', + form=form, + ) + + +@main.route("/organisations//settings/edit-name/confirm", methods=['GET', 'POST']) +@login_required +@user_is_platform_admin +def confirm_edit_organisation_name(org_id): + # Validate password for form + def _check_password(pwd): + return user_api_client.verify_password(current_user.id, pwd) + + form = ConfirmPasswordForm(_check_password) + + if form.validate_on_submit(): + try: + organisations_client.update_organisation_name( + current_organisation['id'], + name=session['organisation_name_change'], + ) + except HTTPError as e: + error_msg = "Organisation name already exists" + if e.status_code == 400 and error_msg in e.message: + # Redirect the user back to the change service name screen + flash('This organisation name is already in use', 'error') + return redirect(url_for('main.edit_organisation_name', org_id=org_id)) + else: + raise e + else: + session.pop('organisation_name_change') + return redirect(url_for('.organisation_settings', org_id=org_id)) + return render_template( + 'views/organisations/organisation/settings/edit-name/confirm.html', + new_name=session['organisation_name_change'], + form=form) diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index 0c4b110fc..52b0a9f06 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -18,7 +18,7 @@ class OrganisationsClient(NotifyAdminAPIClient): } return self.post(url="/organisations", data=data) - def update_organisation(self, org_id, name): + def update_organisation_name(self, org_id, name): data = { "name": name } @@ -45,3 +45,9 @@ class OrganisationsClient(NotifyAdminAPIClient): user_id=user_id) data = _attach_current_user({}) return self.delete(endpoint, data) + + def is_organisation_name_unique(self, org_id, name): + return self.get( + url="/organisations/unique", + params={"org_id": org_id, "name": name} + )["result"] diff --git a/app/templates/org_nav.html b/app/templates/org_nav.html index 78805cb65..8a8be173d 100644 --- a/app/templates/org_nav.html +++ b/app/templates/org_nav.html @@ -2,5 +2,6 @@ diff --git a/app/templates/views/organisations/organisation/settings/edit-name/confirm.html b/app/templates/views/organisations/organisation/settings/edit-name/confirm.html new file mode 100644 index 000000000..6fa3d5c77 --- /dev/null +++ b/app/templates/views/organisations/organisation/settings/edit-name/confirm.html @@ -0,0 +1,28 @@ +{% extends "org_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block org_page_title %} + Confirm organisation name change +{% endblock %} + +{% block maincolumn_content %} + +

Confirm organisation name change

+ +
+
+ +
+ {{ textbox(form.password) }} +

Your organisation name will be changed from {{ current_org.name }} to {{ new_name }}

+ {{ page_footer( + 'Confirm', + destructive=destructive, + back_link=url_for('.organisation_settings', org_id=current_org.id) + ) }} +
+
+
+ +{% endblock %} diff --git a/app/templates/views/organisations/organisation/settings/edit-name/index.html b/app/templates/views/organisations/organisation/settings/edit-name/index.html new file mode 100644 index 000000000..2c3df4084 --- /dev/null +++ b/app/templates/views/organisations/organisation/settings/edit-name/index.html @@ -0,0 +1,21 @@ +{% extends "org_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/textbox.html" import textbox, colour_textbox %} + +{% block org_page_title %} + Change organisation name +{% endblock %} + +{% block maincolumn_content %} + +

Change organisation name

+
+ {{textbox(form.name)}} + {{ page_footer( + 'Save', + back_link=url_for('.organisation_settings', org_id=current_org.id), + back_link_text='Back to settings', + ) }} +
+ +{% endblock %} diff --git a/app/templates/views/organisations/organisation/settings/index.html b/app/templates/views/organisations/organisation/settings/index.html new file mode 100644 index 000000000..d8c08593a --- /dev/null +++ b/app/templates/views/organisations/organisation/settings/index.html @@ -0,0 +1,27 @@ +{% extends "org_template.html" %} +{% from "components/table.html" import mapping_table, row, text_field, edit_field with context %} + +{% block org_page_title %} + Organisation settings +{% endblock %} + +{% block maincolumn_content %} +

Organisation settings

+
+ {% call mapping_table( + caption='General', + field_headings=['Label', 'Value', 'Action'], + field_headings_visible=False, + caption_visible=False + ) %} + {% call row() %} + {{ text_field('Organisation name') }} + {{ text_field(current_org.name) }} + {{ edit_field( + 'Change', + url_for('.edit_organisation_name', org_id=current_org.id) + ) + }} + {% endcall %} + {% endcall %} +{% endblock %} diff --git a/app/templates/views/organisations/organisation/update-organisation.html b/app/templates/views/organisations/organisation/update-organisation.html deleted file mode 100644 index 47eff28f4..000000000 --- a/app/templates/views/organisations/organisation/update-organisation.html +++ /dev/null @@ -1,25 +0,0 @@ -{% extends "views/platform-admin/_base_template.html" %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/textbox.html" import textbox, colour_textbox %} - -{% block per_page_title %} - Update an organisation -{% endblock %} - -{% block org_page_title %} - Update an organisation -{% endblock %} - -{% block platform_admin_content %} - -

Update an organisation

-
- {{textbox(form.name)}} - {{ page_footer( - 'Save', - back_link=url_for('.organisations'), - back_link_text='Back to organisations', - ) }} -
- -{% endblock %} diff --git a/tests/app/main/views/test_organisations.py b/tests/app/main/views/test_organisations.py index 2be6c9f86..dc025a4c0 100644 --- a/tests/app/main/views/test_organisations.py +++ b/tests/app/main/views/test_organisations.py @@ -1,9 +1,10 @@ from datetime import datetime, timedelta -from unittest.mock import ANY +from unittest.mock import ANY, Mock import pytest from bs4 import BeautifulSoup from flask import url_for +from notifications_python_client.errors import HTTPError from tests.conftest import normalize_spaces from app.notify_client.models import InvitedOrgUser @@ -63,26 +64,6 @@ def test_view_organisation_shows_the_correct_organisation( assert normalize_spaces(page.select_one('.heading-large').text) == org['name'] -def test_edit_organisation_shows_the_correct_organisation( - logged_in_platform_admin_client, - fake_uuid, - mocker -): - org = {'id': fake_uuid, 'name': 'Test 1', 'active': True} - mocker.patch( - 'app.organisations_client.get_organisation', return_value=org - ) - - response = logged_in_platform_admin_client.get( - url_for('.update_organisation', org_id=fake_uuid) - ) - - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert page.select_one('#name').attrs.get('value') == org['name'] - - def test_create_new_organisation( logged_in_platform_admin_client, mocker, @@ -103,33 +84,6 @@ def test_create_new_organisation( mock_create_organisation.assert_called_once_with(name=org['name']) -def test_update_organisation( - logged_in_platform_admin_client, - mocker, - fake_uuid, -): - org = {'name': 'new name'} - - mocker.patch( - 'app.organisations_client.get_organisation', return_value=org - ) - mock_update_organisation = mocker.patch( - 'app.organisations_client.update_organisation' - ) - - logged_in_platform_admin_client.post( - url_for('.update_organisation', org_id=fake_uuid), - content_type='multipart/form-data', - data=org - ) - - assert mock_update_organisation.called - mock_update_organisation.assert_called_once_with( - org_id=fake_uuid, - name=org['name'] - ) - - def test_organisation_services_show( logged_in_platform_admin_client, mock_get_organisation, @@ -233,8 +187,6 @@ def test_invite_org_user_errors_when_same_email_as_inviter( assert mock_invite_org_user.called is False assert normalize_spaces(page.select_one('.error-message').text) == 'You can’t send an invitation to yourself' -# Broken - def test_accepted_invite_when_user_already_logged_in( logged_in_client, @@ -515,3 +467,171 @@ def test_verified_org_user_redirects_to_dashboard( org_id=invited_org_user['organisation'], _external=True ) + + +def test_organisation_settings( + logged_in_platform_admin_client, + mock_get_organisation, + organisation_one +): + expected_rows = [ + 'Label Value Action', + 'Organisation name Org 1 Change', + ] + + response = logged_in_platform_admin_client.get(url_for('.organisation_settings', org_id=organisation_one['id'])) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert page.find('h1').text == 'Organisation 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']) + + +def test_update_organisation_name( + logged_in_platform_admin_client, + organisation_one, + mock_get_organisation, + mock_organisation_name_is_unique +): + response = logged_in_platform_admin_client.post( + url_for('.edit_organisation_name', org_id=organisation_one['id']), + data={'name': 'TestNewOrgName'} + ) + + assert response.status_code == 302 + assert response.location == url_for( + '.confirm_edit_organisation_name', + org_id=organisation_one['id'], + _external=True + ) + assert mock_organisation_name_is_unique.called + + +def test_update_organisation_with_incorrect_input( + logged_in_platform_admin_client, + organisation_one, + mock_get_organisation, +): + response = logged_in_platform_admin_client.post( + url_for('.edit_organisation_name', org_id=organisation_one['id']), + data={'name': ''} + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert normalize_spaces( + page.select_one('.error-message').text + ) == "Can’t be empty" + + +def test_update_organisation_with_non_unique_name( + logged_in_platform_admin_client, + organisation_one, + mock_get_organisation, + mock_organisation_name_is_not_unique +): + response = logged_in_platform_admin_client.post( + url_for('.edit_organisation_name', org_id=organisation_one['id']), + data={'name': 'TestNewOrgName'} + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert normalize_spaces( + page.select_one('.error-message').text + ) == 'This organisation name is already in use' + + assert mock_organisation_name_is_not_unique.called + + +def test_confirm_update_organisation( + logged_in_platform_admin_client, + organisation_one, + mock_get_organisation, + mock_verify_password, + mock_update_organisation_name, + mocker +): + with logged_in_platform_admin_client.session_transaction() as session: + session['organisation_name_change'] = 'newName' + + response = logged_in_platform_admin_client.post( + url_for( + '.confirm_edit_organisation_name', + org_id=organisation_one['id'], + data={'password', 'validPassword'} + ) + ) + + assert response.status_code == 302 + assert response.location == url_for('.organisation_settings', org_id=organisation_one['id'], _external=True) + + mock_update_organisation_name.assert_called_with( + organisation_one['id'], + name=session['organisation_name_change'] + ) + + +def test_confirm_update_organisation_with_incorrect_password( + logged_in_platform_admin_client, + organisation_one, + mock_get_organisation, + mocker +): + with logged_in_platform_admin_client.session_transaction() as session: + session['organisation_name_change'] = 'newName' + + mocker.patch('app.user_api_client.verify_password', return_value=False) + + response = logged_in_platform_admin_client.post( + url_for( + '.confirm_edit_organisation_name', + org_id=organisation_one['id'] + ) + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert normalize_spaces( + page.select_one('.error-message').text + ) == 'Invalid password' + + +def test_confirm_update_organisation_with_name_already_in_use( + logged_in_platform_admin_client, + organisation_one, + mock_get_organisation, + mock_verify_password, + mocker +): + with logged_in_platform_admin_client.session_transaction() as session: + session['organisation_name_change'] = 'newName' + + mocker.patch( + 'app.organisations_client.update_organisation_name', + side_effect=HTTPError( + response=Mock( + status_code=400, + json={'result': 'error', 'message': 'Organisation name already exists'} + ), + message="Organisation name already exists" + ) + ) + + response = logged_in_platform_admin_client.post( + url_for( + '.confirm_edit_organisation_name', + org_id=organisation_one['id'] + ) + ) + + assert response.status_code == 302 + assert response.location == url_for('main.edit_organisation_name', org_id=organisation_one['id'], _external=True) diff --git a/tests/conftest.py b/tests/conftest.py index 7635a709b..82952fab4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2842,3 +2842,21 @@ def mock_add_user_to_organisation(mocker, organisation_one, api_user_active): return api_user_active return mocker.patch('app.user_api_client.add_user_to_organisation', side_effect=_add_user) + + +@pytest.fixture(scope='function') +def mock_organisation_name_is_not_unique(mocker): + return mocker.patch('app.organisations_client.is_organisation_name_unique', return_value=False) + + +@pytest.fixture(scope='function') +def mock_organisation_name_is_unique(mocker): + return mocker.patch('app.organisations_client.is_organisation_name_unique', return_value=True) + + +@pytest.fixture(scope='function') +def mock_update_organisation_name(mocker): + def _update_org_name(organisation_id, name): + return + + return mocker.patch('app.organisations_client.update_organisation_name', side_effect=_update_org_name)