From d51eda63fa66ad9bb380ddd14633fc4d12a59b41 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Wed, 24 Jan 2024 15:08:19 +0000 Subject: [PATCH 01/11] Minor view tidy up --- .../admin/{application => partners}/_address_fields.html.erb | 0 app/views/admin/partners/_form.html.erb | 5 +++-- 2 files changed, 3 insertions(+), 2 deletions(-) rename app/views/admin/{application => partners}/_address_fields.html.erb (100%) diff --git a/app/views/admin/application/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb similarity index 100% rename from app/views/admin/application/_address_fields.html.erb rename to app/views/admin/partners/_address_fields.html.erb diff --git a/app/views/admin/partners/_form.html.erb b/app/views/admin/partners/_form.html.erb index d736f9617..b106bf7bc 100644 --- a/app/views/admin/partners/_form.html.erb +++ b/app/views/admin/partners/_form.html.erb @@ -27,9 +27,10 @@
- <%= f.fields_for :address, @partner.address || Address.new do |a| %> - <%= render 'address_fields', f: a %> + <%= f.fields_for :address do |address_form| %> + <%= render 'address_fields', f: address_form %> <% end %> + <% if @partner&.address&.neighbourhood&.legacy_neighbourhood? %>

The address for this partner is assigned to an out of date neighbourhood. From 9c6e7191ceef4bbaaa252cc30583671f9bcc9d9e Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Wed, 24 Jan 2024 15:26:25 +0000 Subject: [PATCH 02/11] Action for admins to remove partner addresses --- app/controllers/admin/partners_controller.rb | 15 ++++++++++++++- app/models/partner.rb | 2 +- app/policies/partner_policy.rb | 4 ++++ config/routes.rb | 3 +++ .../controllers/admin/partners_controller_test.rb | 14 ++++++++++++++ 5 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/partners_controller.rb b/app/controllers/admin/partners_controller.rb index d4cbbdd4b..246f07fff 100644 --- a/app/controllers/admin/partners_controller.rb +++ b/app/controllers/admin/partners_controller.rb @@ -3,7 +3,7 @@ module Admin class PartnersController < Admin::ApplicationController include LoadUtilities - before_action :set_partner, only: %i[show edit update destroy] + before_action :set_partner, only: %i[show edit update destroy clear_address] before_action :set_tags, only: %i[new create edit] before_action :set_neighbourhoods, only: %i[new edit] before_action :set_partner_tags_controller, only: %i[new edit] @@ -108,6 +108,19 @@ def destroy end end + def clear_address + authorize @partner + + Partner.transaction do + address = @partner.address + @partner.update! address_id: nil + + address&.destroy + end + + render json: { message: 'Address cleared' } + end + def setup @partner = Partner.new authorize @partner diff --git a/app/models/partner.rb b/app/models/partner.rb index 4598eadbb..dae000e24 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -16,7 +16,7 @@ class Partner < ApplicationRecord has_and_belongs_to_many :users has_many :calendars, dependent: :destroy has_many :events - belongs_to :address, optional: true + belongs_to :address, optional: true, dependent: :destroy has_many :partner_tags, dependent: :destroy has_many :tags, through: :partner_tags diff --git a/app/policies/partner_policy.rb b/app/policies/partner_policy.rb index b58ae0804..a222493e0 100644 --- a/app/policies/partner_policy.rb +++ b/app/policies/partner_policy.rb @@ -37,6 +37,10 @@ def destroy? return true if user.only_neighbourhood_admin_for_partner?(record.id) end + def clear_address? + user.root? + end + def setup? create? end diff --git a/config/routes.rb b/config/routes.rb index 90c0e4083..3aef1141d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -37,6 +37,9 @@ collection do match :setup, via: %i[get post] end + member do + delete :clear_address + end end resources :tags resources :sites diff --git a/test/controllers/admin/partners_controller_test.rb b/test/controllers/admin/partners_controller_test.rb index 72208dcc5..e1eaf76ad 100644 --- a/test/controllers/admin/partners_controller_test.rb +++ b/test/controllers/admin/partners_controller_test.rb @@ -265,4 +265,18 @@ class Admin::PartnersControllerTest < ActionDispatch::IntegrationTest assert_response :redirect assert_equal 'Partners must have an address or a service area inside your neighbourhood', flash[:danger] end + + test 'root user clear address on partner clears address' do + sign_in @root + + delete clear_address_admin_partner_path(@partner) + assert_response :success + + @partner.reload + assert_nil @partner.address + + # will work even if the partner has no address + delete clear_address_admin_partner_path(@partner) + assert_response :success + end end From 52e46558719b28b0f38cd46df78a6d6b8388e917 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Thu, 25 Jan 2024 12:28:06 +0000 Subject: [PATCH 03/11] Admins clearing a partners' address --- app/javascript/controllers/index.js | 3 + .../controllers/partner_address_controller.js | 61 +++++++++++++++++++ .../admin/partners/_address_fields.html.erb | 42 +++++++++++-- app/views/admin/partners/_form.html.erb | 5 +- 4 files changed, 104 insertions(+), 7 deletions(-) create mode 100644 app/javascript/controllers/partner_address_controller.js diff --git a/app/javascript/controllers/index.js b/app/javascript/controllers/index.js index e5c0360d5..45e885b6b 100644 --- a/app/javascript/controllers/index.js +++ b/app/javascript/controllers/index.js @@ -21,3 +21,6 @@ application.register("user-partners", UserPartnersController); import PartnerTagsController from "./partner_tags_controller.js"; application.register("partner-tags", PartnerTagsController); + +import PartnerAddressController from "./partner_address_controller.js"; +application.register("partner-address", PartnerAddressController); diff --git a/app/javascript/controllers/partner_address_controller.js b/app/javascript/controllers/partner_address_controller.js new file mode 100644 index 000000000..3ad303ad8 --- /dev/null +++ b/app/javascript/controllers/partner_address_controller.js @@ -0,0 +1,61 @@ +import { Controller } from "@hotwired/stimulus"; + +export default class extends Controller { + static values = { + partnerId: String, + }; + + static targets = ["addressInfoArea"]; + + static addressFieldIds = [ + "partner_address_attributes_street_address", + "partner_address_attributes_street_address2", + "partner_address_attributes_street_address3", + "partner_address_attributes_city", + "partner_address_attributes_postcode", + ]; + + connect() {} + + disconnect() {} + + do_clear_address(event) { + console.log("do_clear_address"); + + event.preventDefault(); + console.log("do_clear_address"); + + if (!confirm("Please confirm you want to clear this partners address")) { + return; + } + + const csrfToken = document + .querySelector('meta[name="csrf-token"]') + .getAttribute("content"); + + const url = `/partners/${this.partnerIdValue}/clear_address`; + + const payload = { + method: "DELETE", + headers: { + Accept: "application/json", + "Content-Type": "application/json", + "X-CSRF-Token": csrfToken, + }, + body: "", + }; + + fetch(url, payload) + .then((response) => response.json()) + .then((data) => { + this.constructor.addressFieldIds.forEach((id) => { + let node = document.getElementById(id); + node.value = ""; + node.classList.remove("is-valid"); + }); + + this.addressInfoAreaTarget.innerHTML = + "

Address has been cleared

"; + }); + } +} diff --git a/app/views/admin/partners/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb index 8cee20785..db711c70b 100644 --- a/app/views/admin/partners/_address_fields.html.erb +++ b/app/views/admin/partners/_address_fields.html.erb @@ -1,7 +1,37 @@ -
- <%= f.input :street_address, class: "form-control address_1 address_field", label: 'Street address' %> - <%= f.input :street_address2, class: "form-control address_2 address_field", label: 'Street address 2' %> - <%= f.input :street_address3, class: "form-control address_3 address_field", label: 'Street address 3' %> - <%= f.input :city, class: "form-control city address_field" %> - <%= f.input :postcode, class: "form-control postcode address_field" %> +
+ + <%# this view is augmented by the /app/javascript/controllers/parter_address_controller.js %> + + <%= f.input :street_address, + class: "form-control address_1 address_field", + label: 'Street address' %> + + <%= f.input :street_address2, + class: "form-control address_2 address_field", + label: 'Street address 2' %> + + <%= f.input :street_address3, + class: "form-control address_3 address_field", + label: 'Street address 3' %> + + <%= f.input :city, + class: "form-control city address_field" %> + + <%= f.input :postcode, + class: "form-control postcode address_field" %> + + <% if address.present? %> +
+

Address in neighbourhood <%= link_to_neighbourhood(address.neighbourhood) %>.

+

+ <%= link_to 'Clear Address', + '#', + class: "btn btn-secondary btn-sm", + data: { action: "click->partner-address#do_clear_address" } %> + WARNING: clicking this button will permanently clear this partners address! +

+
+ <% end %>
diff --git a/app/views/admin/partners/_form.html.erb b/app/views/admin/partners/_form.html.erb index b106bf7bc..4afafea77 100644 --- a/app/views/admin/partners/_form.html.erb +++ b/app/views/admin/partners/_form.html.erb @@ -28,7 +28,10 @@
<%= f.fields_for :address do |address_form| %> - <%= render 'address_fields', f: address_form %> + <%= render 'address_fields', + f: address_form, + address: @partner.address, + partner: @partner %> <% end %> <% if @partner&.address&.neighbourhood&.legacy_neighbourhood? %> From f38d79034141e50200ae5003c4ec6294171779d5 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Mon, 29 Jan 2024 09:54:26 +0000 Subject: [PATCH 04/11] Checks on clearing partner addresses - must have address - must have a service area - must be root (for now) --- app/controllers/admin/partners_controller.rb | 12 +++++------ app/models/partner.rb | 20 +++++++++++++++++++ .../admin/partners/_address_fields.html.erb | 16 ++++++++------- app/views/admin/partners/_form.html.erb | 10 ++++------ .../admin/partners_controller_test.rb | 4 ++-- test/models/partner_test.rb | 14 +++++++++++++ 6 files changed, 55 insertions(+), 21 deletions(-) diff --git a/app/controllers/admin/partners_controller.rb b/app/controllers/admin/partners_controller.rb index 246f07fff..e9c27f925 100644 --- a/app/controllers/admin/partners_controller.rb +++ b/app/controllers/admin/partners_controller.rb @@ -111,14 +111,14 @@ def destroy def clear_address authorize @partner - Partner.transaction do - address = @partner.address - @partner.update! address_id: nil + if @partner.can_clear_address?(current_user) + @partner.clear_address! + render json: { message: 'Address cleared' } - address&.destroy + else + render json: { message: 'Could not clear address' }, + status: :unprocessable_entity end - - render json: { message: 'Address cleared' } end def setup diff --git a/app/models/partner.rb b/app/models/partner.rb index dae000e24..c1b0e9270 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -247,6 +247,26 @@ def has_service_areas? service_areas.any? end + def can_clear_address?(user = nil) + return false if address.blank? + return false if service_areas.empty? + return true if user&.root? + + # TODO: only allow address to be cleared if user has a neighbourhood + # that this partner has a service area falls within. + + false + end + + def clear_address! + Partner.transaction do + old_address = address + update! address_id: nil + + old_address&.destroy + end + end + def permalink "https://placecal.org/partners/#{id}" end diff --git a/app/views/admin/partners/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb index db711c70b..3d249488f 100644 --- a/app/views/admin/partners/_address_fields.html.erb +++ b/app/views/admin/partners/_address_fields.html.erb @@ -1,30 +1,31 @@ +<%= form.fields_for :address do |address_form| %>
<%# this view is augmented by the /app/javascript/controllers/parter_address_controller.js %> - <%= f.input :street_address, + <%= address_form.input :street_address, class: "form-control address_1 address_field", label: 'Street address' %> - <%= f.input :street_address2, + <%= address_form.input :street_address2, class: "form-control address_2 address_field", label: 'Street address 2' %> - <%= f.input :street_address3, + <%= address_form.input :street_address3, class: "form-control address_3 address_field", label: 'Street address 3' %> - <%= f.input :city, + <%= address_form.input :city, class: "form-control city address_field" %> - <%= f.input :postcode, + <%= address_form.input :postcode, class: "form-control postcode address_field" %> - <% if address.present? %> + <% if partner.can_clear_address? %>
-

Address in neighbourhood <%= link_to_neighbourhood(address.neighbourhood) %>.

+

Address in neighbourhood <%= link_to_neighbourhood(partner.address.neighbourhood) %>.

<%= link_to 'Clear Address', '#', @@ -35,3 +36,4 @@

<% end %>
+<% end %> diff --git a/app/views/admin/partners/_form.html.erb b/app/views/admin/partners/_form.html.erb index 4afafea77..16e518cf2 100644 --- a/app/views/admin/partners/_form.html.erb +++ b/app/views/admin/partners/_form.html.erb @@ -27,12 +27,10 @@
- <%= f.fields_for :address do |address_form| %> - <%= render 'address_fields', - f: address_form, - address: @partner.address, - partner: @partner %> - <% end %> + + <%= render 'address_fields', + form: f, + partner: @partner %> <% if @partner&.address&.neighbourhood&.legacy_neighbourhood? %>

diff --git a/test/controllers/admin/partners_controller_test.rb b/test/controllers/admin/partners_controller_test.rb index e1eaf76ad..75b57fcf6 100644 --- a/test/controllers/admin/partners_controller_test.rb +++ b/test/controllers/admin/partners_controller_test.rb @@ -275,8 +275,8 @@ class Admin::PartnersControllerTest < ActionDispatch::IntegrationTest @partner.reload assert_nil @partner.address - # will work even if the partner has no address + # will not work if no address is set delete clear_address_admin_partner_path(@partner) - assert_response :success + assert_response :unprocessable_entity end end diff --git a/test/models/partner_test.rb b/test/models/partner_test.rb index 0ebb2cf37..fead0d9ab 100644 --- a/test/models/partner_test.rb +++ b/test/models/partner_test.rb @@ -423,4 +423,18 @@ class PartnerTest < ActiveSupport::TestCase partner = create(:partner, :accessed_by_user => pa, :tags => [pa.tags.first]) assert_predicate partner, :valid? end + + test 'can_clear_address?' do + partner = Partner.new + assert_not partner.can_clear_address? + + partner.address = create(:address) + assert_not partner.can_clear_address? + + partner.service_areas.build(neighbourhood: create(:neighbourhood)) + assert_not partner.can_clear_address? + + root = create(:root) + assert partner.can_clear_address?(root) + end end From f30583063841363e9c29a04cf514c60a266452ba Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Mon, 29 Jan 2024 11:54:32 +0000 Subject: [PATCH 05/11] Allow partner admins to clear partner address --- app/models/partner.rb | 13 +++++++++---- test/models/partner_test.rb | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/app/models/partner.rb b/app/models/partner.rb index c1b0e9270..eca7d6a94 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -250,12 +250,17 @@ def has_service_areas? def can_clear_address?(user = nil) return false if address.blank? return false if service_areas.empty? - return true if user&.root? - # TODO: only allow address to be cleared if user has a neighbourhood - # that this partner has a service area falls within. + return false if user.blank? + return true if user.root? + return true if user.admin_for_partner?(id) - false + user_hood_ids = user.owned_neighbourhood_ids + return false if user_hood_ids.empty? + + sa_hood_ids = service_areas.pluck(:neighbourhood_id) + + Set.new(user_hood_ids).any?(Set.new(sa_hood_ids)) end def clear_address! diff --git a/test/models/partner_test.rb b/test/models/partner_test.rb index fead0d9ab..d67f42339 100644 --- a/test/models/partner_test.rb +++ b/test/models/partner_test.rb @@ -424,17 +424,45 @@ class PartnerTest < ActiveSupport::TestCase assert_predicate partner, :valid? end - test 'can_clear_address?' do - partner = Partner.new + test 'can_clear_address? for root' do + partner = build(:bare_partner) + # has no address assert_not partner.can_clear_address? partner.address = create(:address) + # missing service area assert_not partner.can_clear_address? partner.service_areas.build(neighbourhood: create(:neighbourhood)) + # is not root assert_not partner.can_clear_address? root = create(:root) assert partner.can_clear_address?(root) end + + test 'can_clear_address? for partner admin' do + partner = build(:bare_partner) + partner.address = create(:address) + partner.service_areas.build(neighbourhood: create(:neighbourhood)) + partner.save! + + citizen = create(:citizen) + citizen.partners << partner + + assert partner.can_clear_address?(citizen) + end + + test 'can_clear_address? for neighbourhood admin' do + neighbourhood = create(:neighbourhood) + citizen = create(:citizen) + citizen.neighbourhoods << neighbourhood + + partner = build(:bare_partner) + partner.address = create(:address) + partner.service_areas.build(neighbourhood: neighbourhood) + partner.save! + + assert partner.can_clear_address?(citizen) + end end From 541305c50db477227e3cd563706ab653c89053f1 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Mon, 29 Jan 2024 12:04:14 +0000 Subject: [PATCH 06/11] Partner address clear check with current user --- app/views/admin/partners/_address_fields.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/partners/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb index 3d249488f..1173dfbf4 100644 --- a/app/views/admin/partners/_address_fields.html.erb +++ b/app/views/admin/partners/_address_fields.html.erb @@ -23,7 +23,7 @@ <%= address_form.input :postcode, class: "form-control postcode address_field" %> - <% if partner.can_clear_address? %> + <% if partner.can_clear_address?(current_user) %>

Address in neighbourhood <%= link_to_neighbourhood(partner.address.neighbourhood) %>.

From 8780db6ddede9c2fc60d2cbd7e5cc8958db70048 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Mon, 29 Jan 2024 12:17:27 +0000 Subject: [PATCH 07/11] Lower authorisation levels on partner clear address action --- app/policies/partner_policy.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/policies/partner_policy.rb b/app/policies/partner_policy.rb index a222493e0..aeb9caec8 100644 --- a/app/policies/partner_policy.rb +++ b/app/policies/partner_policy.rb @@ -38,7 +38,7 @@ def destroy? end def clear_address? - user.root? + index? end def setup? From 03f561c7465c873b3711be160aea5f70c7245496 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Mon, 29 Jan 2024 14:43:41 +0000 Subject: [PATCH 08/11] Warning admins about clearing partner addresses If the admin is not root or does not own a partner then check to see if the admin can "see" the partner via service areas. If they cannot then warn the user that clearing the address will stop the admin from being able to access this partner at all. --- .rubocop.yml | 6 +++ .../controllers/partner_address_controller.js | 8 +++- app/models/partner.rb | 20 +++++++++- .../admin/partners/_address_fields.html.erb | 3 +- test/models/partner_test.rb | 38 +++++++++++++++++++ 5 files changed, 71 insertions(+), 4 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index da1449aeb..7cb3c5baf 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -55,6 +55,12 @@ Style/HashSyntax: Rails/ActionOrder: Enabled: false +Metrics/ClassLength: + Exclude: + - "test/**/*.rb" + + + Rails/RootPathnameMethods: Enabled: false GraphQL/MaxComplexitySchema: diff --git a/app/javascript/controllers/partner_address_controller.js b/app/javascript/controllers/partner_address_controller.js index 3ad303ad8..3624f8cba 100644 --- a/app/javascript/controllers/partner_address_controller.js +++ b/app/javascript/controllers/partner_address_controller.js @@ -3,6 +3,7 @@ import { Controller } from "@hotwired/stimulus"; export default class extends Controller { static values = { partnerId: String, + warnOfDelisting: String, // "true" or "false" }; static targets = ["addressInfoArea"]; @@ -25,7 +26,12 @@ export default class extends Controller { event.preventDefault(); console.log("do_clear_address"); - if (!confirm("Please confirm you want to clear this partners address")) { + let warning_text = "Please confirm you want to clear this partners address"; + if (this.warnOfDelistingValue === "true") { + warning_text = `This address links to you to this partner and by clearing this address you will no longer be able to access this partner,\n\n${warning_text}`; + } + + if (!confirm(warning_text)) { return; } diff --git a/app/models/partner.rb b/app/models/partner.rb index eca7d6a94..8c7e40b5f 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -255,12 +255,28 @@ def can_clear_address?(user = nil) return true if user.root? return true if user.admin_for_partner?(id) + return true if user.neighbourhood_admin? + return true if user.partnership_admin? + end + + def warn_user_clear_address?(user) + return false if user.root? + return false if user.admin_for_partner?(id) + user_hood_ids = user.owned_neighbourhood_ids - return false if user_hood_ids.empty? + # this shouldn't happen (?) - a user has no neighbourhoods + # but can still remove a partners address? if that is the case + # then they aren't seeing the partner by neighbourhood anyway + # so warning them that removing the address will remove the + # partner seems wrong + return true if user_hood_ids.empty? sa_hood_ids = service_areas.pluck(:neighbourhood_id) - Set.new(user_hood_ids).any?(Set.new(sa_hood_ids)) + any_service_areas = Set.new(user_hood_ids).any?(Set.new(sa_hood_ids)) + + # is the only way this user is tied to this partner through the address? + any_service_areas == false end def clear_address! diff --git a/app/views/admin/partners/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb index 1173dfbf4..804f058c5 100644 --- a/app/views/admin/partners/_address_fields.html.erb +++ b/app/views/admin/partners/_address_fields.html.erb @@ -1,7 +1,8 @@ <%= form.fields_for :address do |address_form| %>

+ data-partner-address-partner-id-value="<%= partner.id %>" + data-partner-address-warn-of-delisting-value="<%= partner.warn_user_clear_address?(current_user) ? 'true' : 'false' %>"> <%# this view is augmented by the /app/javascript/controllers/parter_address_controller.js %> diff --git a/test/models/partner_test.rb b/test/models/partner_test.rb index d67f42339..bda0b9890 100644 --- a/test/models/partner_test.rb +++ b/test/models/partner_test.rb @@ -465,4 +465,42 @@ class PartnerTest < ActiveSupport::TestCase assert partner.can_clear_address?(citizen) end + + test 'can_clear_address? for partnership admin' do + tag = create(:partnership) + citizen = create(:citizen) + citizen.tags << tag + + partner = build(:bare_partner) + partner.address = create(:address) + + neighbourhood = create(:neighbourhood) + partner.service_areas.build(neighbourhood: neighbourhood) + + partner.save! + + assert partner.can_clear_address?(citizen) + end + + test 'warn_user_clear_address?' do + partner = build(:bare_partner) + partner.address = create(:address) + partner.save! + + # am root + root = create(:root) + assert_not partner.warn_user_clear_address?(root) + + # am owner + citizen = create(:citizen) + citizen.partners << partner + assert_not partner.warn_user_clear_address?(citizen) + + # not root or owner, so warn + other_neighbourhood = create(:neighbourhood) + other_citizen = create(:citizen) + other_citizen.neighbourhoods << other_neighbourhood + + assert partner.warn_user_clear_address?(other_citizen) + end end From aa99d0d0622a2ff871056cff9aeea2832bdd9b53 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Tue, 30 Jan 2024 13:43:49 +0000 Subject: [PATCH 09/11] Minor fixes - admins can see address form even if partner has no address - update to policy governing who can trigger the clear address action - allow only neighbourhood admins to clear an address if that address is in their neighbourhood pool - removed redundant warning text - removed console logging from JS controller --- .../controllers/partner_address_controller.js | 3 --- app/models/address.rb | 8 ++++++++ app/models/partner.rb | 17 ++++++++++------- app/policies/partner_policy.rb | 2 +- .../admin/partners/_address_fields.html.erb | 3 +-- test/models/partner_test.rb | 10 +++++++--- 6 files changed, 27 insertions(+), 16 deletions(-) diff --git a/app/javascript/controllers/partner_address_controller.js b/app/javascript/controllers/partner_address_controller.js index 3624f8cba..a199da1ef 100644 --- a/app/javascript/controllers/partner_address_controller.js +++ b/app/javascript/controllers/partner_address_controller.js @@ -21,10 +21,7 @@ export default class extends Controller { disconnect() {} do_clear_address(event) { - console.log("do_clear_address"); - event.preventDefault(); - console.log("do_clear_address"); let warning_text = "Please confirm you want to clear this partners address"; if (this.warnOfDelistingValue === "true") { diff --git a/app/models/address.rb b/app/models/address.rb index 411bc2a0c..3d64882c1 100644 --- a/app/models/address.rb +++ b/app/models/address.rb @@ -31,6 +31,14 @@ def prepend_room_number(room_number_string) self end + def missing_values? + street_address.blank? && + street_address2.blank? && + street_address3.blank? && + city.blank? && + postcode.blank? + end + def first_address_line street_address end diff --git a/app/models/partner.rb b/app/models/partner.rb index 8c7e40b5f..22c2f5c8a 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -248,15 +248,23 @@ def has_service_areas? end def can_clear_address?(user = nil) - return false if address.blank? + return false if address.blank? || address.missing_values? return false if service_areas.empty? return false if user.blank? return true if user.root? return true if user.admin_for_partner?(id) - return true if user.neighbourhood_admin? + # must be at least one of these types of admin + return false unless user.neighbourhood_admin? || user.partnership_admin? + return true if user.partnership_admin? + + user_hood_ids = user.owned_neighbourhood_ids + return false if user_hood_ids.empty? # (not a neighbourhood admin) + + # must admin for this address specifically + user_hood_ids.include?(address.neighbourhood_id) end def warn_user_clear_address?(user) @@ -264,11 +272,6 @@ def warn_user_clear_address?(user) return false if user.admin_for_partner?(id) user_hood_ids = user.owned_neighbourhood_ids - # this shouldn't happen (?) - a user has no neighbourhoods - # but can still remove a partners address? if that is the case - # then they aren't seeing the partner by neighbourhood anyway - # so warning them that removing the address will remove the - # partner seems wrong return true if user_hood_ids.empty? sa_hood_ids = service_areas.pluck(:neighbourhood_id) diff --git a/app/policies/partner_policy.rb b/app/policies/partner_policy.rb index aeb9caec8..201ff4f97 100644 --- a/app/policies/partner_policy.rb +++ b/app/policies/partner_policy.rb @@ -38,7 +38,7 @@ def destroy? end def clear_address? - index? + edit? end def setup? diff --git a/app/views/admin/partners/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb index 804f058c5..e78bb3f76 100644 --- a/app/views/admin/partners/_address_fields.html.erb +++ b/app/views/admin/partners/_address_fields.html.erb @@ -1,4 +1,4 @@ -<%= form.fields_for :address do |address_form| %> +<%= form.fields_for :address, @partner.address || Address.new do |address_form| %>
partner-address#do_clear_address" } %> - WARNING: clicking this button will permanently clear this partners address!

<% end %> diff --git a/test/models/partner_test.rb b/test/models/partner_test.rb index bda0b9890..9fb2b1336 100644 --- a/test/models/partner_test.rb +++ b/test/models/partner_test.rb @@ -458,25 +458,29 @@ class PartnerTest < ActiveSupport::TestCase citizen = create(:citizen) citizen.neighbourhoods << neighbourhood + # cannot clear address if admin does not "own" address partner = build(:bare_partner) partner.address = create(:address) partner.service_areas.build(neighbourhood: neighbourhood) partner.save! + assert_not partner.can_clear_address?(citizen) + + # now partner is in admins neighbourhood pool, can clear + partner.address.neighbourhood = neighbourhood assert partner.can_clear_address?(citizen) end test 'can_clear_address? for partnership admin' do + neighbourhood = create(:neighbourhood) tag = create(:partnership) citizen = create(:citizen) citizen.tags << tag + # cannot clear address if admin does not "own" address partner = build(:bare_partner) partner.address = create(:address) - - neighbourhood = create(:neighbourhood) partner.service_areas.build(neighbourhood: neighbourhood) - partner.save! assert partner.can_clear_address?(citizen) From 3663759e8f8c249352a35341efc34db8de14fbd8 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Wed, 31 Jan 2024 09:17:39 +0000 Subject: [PATCH 10/11] Simplify clear address checks --- app/models/partner.rb | 9 +-------- test/models/partner_test.rb | 15 --------------- 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/app/models/partner.rb b/app/models/partner.rb index 22c2f5c8a..0e0885e83 100644 --- a/app/models/partner.rb +++ b/app/models/partner.rb @@ -255,15 +255,8 @@ def can_clear_address?(user = nil) return true if user.root? return true if user.admin_for_partner?(id) - # must be at least one of these types of admin - return false unless user.neighbourhood_admin? || user.partnership_admin? - - return true if user.partnership_admin? - - user_hood_ids = user.owned_neighbourhood_ids - return false if user_hood_ids.empty? # (not a neighbourhood admin) - # must admin for this address specifically + user_hood_ids = user.owned_neighbourhood_ids user_hood_ids.include?(address.neighbourhood_id) end diff --git a/test/models/partner_test.rb b/test/models/partner_test.rb index 9fb2b1336..53eca1eed 100644 --- a/test/models/partner_test.rb +++ b/test/models/partner_test.rb @@ -471,21 +471,6 @@ class PartnerTest < ActiveSupport::TestCase assert partner.can_clear_address?(citizen) end - test 'can_clear_address? for partnership admin' do - neighbourhood = create(:neighbourhood) - tag = create(:partnership) - citizen = create(:citizen) - citizen.tags << tag - - # cannot clear address if admin does not "own" address - partner = build(:bare_partner) - partner.address = create(:address) - partner.service_areas.build(neighbourhood: neighbourhood) - partner.save! - - assert partner.can_clear_address?(citizen) - end - test 'warn_user_clear_address?' do partner = build(:bare_partner) partner.address = create(:address) From f0bd66c64d700f8a9bdf27498f029579f3a14f82 Mon Sep 17 00:00:00 2001 From: Ivan Kocienski Date: Wed, 31 Jan 2024 09:52:23 +0000 Subject: [PATCH 11/11] Quick fix on admin partner address form Won't try to look up neighbourhood if address is not valid --- .../admin/partners/_address_fields.html.erb | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/app/views/admin/partners/_address_fields.html.erb b/app/views/admin/partners/_address_fields.html.erb index e78bb3f76..3bb6b1fc1 100644 --- a/app/views/admin/partners/_address_fields.html.erb +++ b/app/views/admin/partners/_address_fields.html.erb @@ -4,36 +4,38 @@ data-partner-address-partner-id-value="<%= partner.id %>" data-partner-address-warn-of-delisting-value="<%= partner.warn_user_clear_address?(current_user) ? 'true' : 'false' %>"> - <%# this view is augmented by the /app/javascript/controllers/parter_address_controller.js %> + <%# this view is augmented by the /app/javascript/controllers/parter_address_controller.js %> - <%= address_form.input :street_address, - class: "form-control address_1 address_field", - label: 'Street address' %> + <%= address_form.input :street_address, + class: "form-control address_1 address_field", + label: 'Street address' %> - <%= address_form.input :street_address2, - class: "form-control address_2 address_field", - label: 'Street address 2' %> + <%= address_form.input :street_address2, + class: "form-control address_2 address_field", + label: 'Street address 2' %> - <%= address_form.input :street_address3, - class: "form-control address_3 address_field", - label: 'Street address 3' %> + <%= address_form.input :street_address3, + class: "form-control address_3 address_field", + label: 'Street address 3' %> - <%= address_form.input :city, - class: "form-control city address_field" %> + <%= address_form.input :city, + class: "form-control city address_field" %> - <%= address_form.input :postcode, - class: "form-control postcode address_field" %> + <%= address_form.input :postcode, + class: "form-control postcode address_field" %> - <% if partner.can_clear_address?(current_user) %> -
-

Address in neighbourhood <%= link_to_neighbourhood(partner.address.neighbourhood) %>.

-

- <%= link_to 'Clear Address', - '#', - class: "btn btn-secondary btn-sm", - data: { action: "click->partner-address#do_clear_address" } %> -

-
+ <% if partner.can_clear_address?(current_user) %> +
+ <% if partner.address.neighbourhood.present? %> +

Address in neighbourhood <%= link_to_neighbourhood(partner.address.neighbourhood) %>.

<% end %> +

+ <%= link_to 'Clear Address', + '#', + class: "btn btn-secondary btn-sm", + data: { action: "click->partner-address#do_clear_address" } %> +

+
+ <% end %>
<% end %>