CLDC-730: Data Coordinators can edit org details (#150)

* Data coordinators can edit some organisation details

* Apply suggestions from code review

PR tweaks

Co-authored-by: Paul Robert Lloyd <paulrobertlloyd@users.noreply.github.com>

* PR comments

* Address line label

* Apply suggestions from code review

Co-authored-by: Paul Robert Lloyd <paulrobertlloyd@users.noreply.github.com>

Co-authored-by: Paul Robert Lloyd <paulrobertlloyd@users.noreply.github.com>

app/controllers/organisations_controller.rb

@@ -19,8 +19,31 @@ class OrganisationsController < ApplicationController
     render "show"
   end
 
+  def edit
+    if current_user.data_coordinator?
+      render "edit", layout: "application"
+    else
+      head :unauthorized
+    end
+  end
+
+  def update
+    if current_user.data_coordinator?
+      if @organisation.update(org_params)
+        flash[:notice] = I18n.t("organisation.updated")
+        redirect_to details_organisation_path(@organisation)
+      end
+    else
+      head :unauthorized
+    end
+  end
+
 private
 
+  def org_params
+    params.require(:organisation).permit(:name, :address_line1, :address_line2, :postcode, :phone)
+  end
+
   def authenticate_scope!
     head :not_found if current_user.organisation != @organisation
   end

app/models/organisation.rb

@@ -23,15 +23,15 @@ class Organisation < ApplicationRecord
   end
 
   def display_attributes
-    {
+    [
-      name: name,
+      { name: "name", value: name, editable: true },
-      address: address_string,
+      { name: "address", value: address_string, editable: true },
-      telephone_number: phone,
+      { name: "telephone_number", value: phone, editable: true },
-      type: "Org type",
+      { name: "type", value: "Org type", editable: false },
-      local_authorities_operated_in: local_authorities,
+      { name: "local_authorities_operated_in", value: local_authorities, editable: false },
-      holds_own_stock: holds_own_stock,
+      { name: "holds_own_stock", value: holds_own_stock, editable: false },
-      other_stock_owners: other_stock_owners,
+      { name: "other_stock_owners", value: other_stock_owners, editable: false },
-      managing_agents: managing_agents,
+      { name: "managing_agents", value: managing_agents, editable: false },
-    }
+    ]
   end
 end

app/views/organisations/edit.html.erb

@@ -0,0 +1,45 @@
+<% content_for :title, "Change #{@organisation.name}’s details" %>
+
+<% content_for :before_content do %>
+  <%= govuk_back_link(
+    text: 'Back',
+    href: :back,
+  ) %>
+<% end %>
+
+<%= form_for(@organisation, as: :organisation, html: { method: :patch }) do |f| %>
+  <div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds">
+      <h1 class="govuk-heading-l">
+        <%= content_for(:title) %>
+      </h1>
+
+      <%= f.govuk_text_field :name,
+        autocomplete: "name"
+      %>
+
+      <%= f.govuk_text_field :address_line1,
+        label: { text: "Address line 1" },
+        autocomplete: "address-line1"
+      %>
+
+      <%= f.govuk_text_field :address_line2,
+        label: { text: "Address line 2" },
+        autocomplete: "address-line2"
+      %>
+
+      <%= f.govuk_text_field :postcode,
+        autocomplete: "postal-code",
+        width: 10
+      %>
+
+      <%= f.govuk_phone_field :phone,
+        label: { text: "Telephone number" },
+        autocomplete: "tel",
+        width: 20
+      %>
+
+      <%= f.govuk_submit "Save changes" %>
+    </div>
+  </div>
+<% end %>

app/views/organisations/show.html.erb

@@ -7,11 +7,22 @@
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds-from-desktop">
     <%= govuk_summary_list do |summary_list| %>
-      <% @organisation.display_attributes.each do |attr, val| %>
+      <% @organisation.display_attributes.each do |attr| %>
-        <%= summary_list.row do |row|
+
-          row.key { attr.to_s.humanize }
+        <% if current_user.data_coordinator? && attr[:editable] %>
-          row.value { simple_format(val.to_s, {}, wrapper_tag: "div") }
+          <%= summary_list.row do |row|
-        end %>
+            row.key { attr[:name].to_s.humanize }
+            row.value { simple_format(attr[:value].to_s, {}, wrapper_tag: "div") }
+            row.action(visually_hidden_text: 'name', href: edit_organisation_path, html_attributes: { 'data-qa': "change-#{attr[:name]}" })
+          end %>
+        <% else %>
+          <%= summary_list.row do |row|
+            row.key { attr[:name].to_s.humanize }
+            row.value { simple_format(attr[:value].to_s, {}, wrapper_tag: "div") }
+            row.action()
+          end %>
+        <% end %>
+
       <% end %>
     <% end %>
   </div>

config/locales/en.yml

@@ -31,3 +31,5 @@
 
 en:
   service_name: "Log social housing lettings and sales (CORE)"
+  organisation:
+    updated: "Organisation details updated"

spec/requests/organisations_controller_spec.rb

@@ -6,6 +6,8 @@ RSpec.describe OrganisationsController, type: :request do
   let(:headers) { { "Accept" => "text/html" } }
   let(:page) { Capybara::Node::Simple.new(response.body) }
   let(:user) { FactoryBot.create(:user, :data_coordinator) }
+  let(:new_value) { "Test Name 35" }
+  let(:params) { { id: organisation.id, organisation: { name: new_value } } }
 
   context "a not signed in user" do
     describe "#show" do
@@ -74,6 +76,11 @@ RSpec.describe OrganisationsController, type: :request do
             expected_html = "<h2 class=\"govuk-visually-hidden\">  Details"
             expect(response.body).to include(expected_html)
           end
+
+          it "has a change details link" do
+            expected_html = "data-qa=\"change-name\" href=\"/organisations/#{organisation.id}/edit\""
+            expect(response.body).to include(expected_html)
+          end
         end
 
         context "organisation that are not in scope for the user, i.e. that they do not belong to" do
@@ -127,6 +134,66 @@ RSpec.describe OrganisationsController, type: :request do
           end
         end
       end
+
+      context "#edit" do
+        context "organisation that the user belongs to" do
+          before do
+            sign_in user
+            get "/organisations/#{organisation.id}/edit", headers: headers, params: {}
+          end
+
+          it "shows an edit form" do
+            expect(response.body).to include("Change #{organisation.name}’s details")
+            expect(page).to have_field("organisation-name-field")
+            expect(page).to have_field("organisation-phone-field")
+          end
+        end
+
+        context "organisation that the user does not belong to" do
+          before do
+            sign_in user
+            get "/organisations/#{unauthorised_organisation.id}/edit", headers: headers, params: {}
+          end
+
+          it "returns a 404 not found" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
+      end
+
+      context "#update" do
+        context "organisation that the user belongs to" do
+          before do
+            sign_in user
+            patch "/organisations/#{organisation.id}", headers: headers, params: params
+          end
+
+          it "updates the org" do
+            organisation.reload
+            expect(organisation.name).to eq(new_value)
+          end
+
+          it "redirects to the organisation details page" do
+            expect(response).to redirect_to("/organisations/#{organisation.id}/details")
+          end
+
+          it "shows a success banner" do
+            follow_redirect!
+            expect(page).to have_css(".govuk-notification-banner.govuk-notification-banner--success")
+          end
+        end
+
+        context "organisation that the user does not belong to" do
+          before do
+            sign_in user
+            patch "/organisations/#{unauthorised_organisation.id}", headers: headers, params: {}
+          end
+
+          it "returns a 404 not found" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
+      end
     end
 
     context "As a data provider user" do
@@ -154,6 +221,11 @@ RSpec.describe OrganisationsController, type: :request do
             expected_html = "<h2 class=\"govuk-visually-hidden\">  Details"
             expect(response.body).to include(expected_html)
           end
+
+          it "does not have a change details link" do
+            expected_html = "data-qa=\"change-name\" href=\"/organisations/#{organisation.id}/edit\""
+            expect(response.body).not_to include(expected_html)
+          end
         end
 
         context "organisation that are not in scope for the user, i.e. that they do not belong to" do
@@ -178,6 +250,28 @@ RSpec.describe OrganisationsController, type: :request do
           expect(response).to have_http_status(:unauthorized)
         end
       end
+
+      context "#edit" do
+        before do
+          sign_in user
+          get "/organisations/#{organisation.id}/edit", headers: headers, params: {}
+        end
+
+        it "redirects to home" do
+          expect(response).to have_http_status(:unauthorized)
+        end
+      end
+
+      context "#update" do
+        before do
+          sign_in user
+          patch "/organisations/#{organisation.id}", headers: headers, params: params
+        end
+
+        it "redirects to home" do
+          expect(response).to have_http_status(:unauthorized)
+        end
+      end
     end
   end
 end