Skip to content

Commit 7d5a457

Browse files
authored
Only show the impound update action for the org's own record (#4006)
A staff member at an `impound_bikes` org viewing an impounded bike got the **Update Impound Record** action no matter who impounded it — the form then pointed into *their* org's impound routes, which the record isn't in. An unorganized impound record has a nil `display_id` by design, so `organization_impound_record_path` couldn't build a URL at all and the registration page 500'd ([133103079](https://app.honeybadger.io/projects/35931/faults/133103079)). - Both the action button and its panel now require `current_impound_record.organization_id` to match the viewing org. Another org's record silently pointed at a display_id the org's `friendly_find!` would 404 on, so that case was broken too, just more quietly. - The wrapper preview's impounded bike carries an impound record now, so the preview renders the update panel it's meant to show.
1 parent b34854d commit 7d5a457

5 files changed

Lines changed: 38 additions & 4 deletions

File tree

app/components/registrations/show/org_top_actions/impound_update/component.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ def initialize(bike:, organization:)
1313
end
1414

1515
def render?
16-
@bike.status_impounded? && impound_record.present?
16+
@bike.status_impounded? && impound_record&.organization_id == @organization.id
1717
end
1818

1919
private

app/components/registrations/show/org_top_actions/wrapper/component.rb

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,13 @@ def show_create_impound?
9090
end
9191

9292
def show_update_impound?
93-
show_impound? && staff? && impounded?
93+
show_impound? && staff? && impounded_by_organization?
94+
end
95+
96+
# The update form posts to this org's impound routes, which another
97+
# org's record - or an unorganized one, which has no display_id - isn't in
98+
def impounded_by_organization?
99+
impounded? && @bike.current_impound_record&.organization_id == @organization.id
94100
end
95101

96102
def show_parking_notifications?

app/components/registrations/show/org_top_actions/wrapper/component_preview.rb

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ def scenarios
1919
{
2020
"With owner" => component(preview_bike(:status_with_owner)),
2121
"Stolen" => component(stolen_bike),
22-
"Impounded" => component(preview_bike(:status_impounded)),
22+
"Impounded" => component(impounded_bike),
2323
"Unregistered parking notification" => component(preview_bike(:unregistered_parking_notification)),
2424
"With parking notification" => component(bike_with_parking_notification),
2525
"Limited (non-staff) member" => component(preview_bike(:status_with_owner), org_role: :limited),
@@ -40,6 +40,13 @@ def stolen_bike
4040
preview_bike(:status_stolen).tap { |bike| bike.current_stolen_record = ::StolenRecord.new }
4141
end
4242

43+
# The update action only shows for the previewing org's own record
44+
def impounded_bike
45+
preview_bike(:status_impounded).tap do |bike|
46+
bike.current_impound_record = ::ImpoundRecord.new(organization_id: lookbook_organization.id, display_id: "0001")
47+
end
48+
end
49+
4350
# A live count and the notification panel are DB queries, so this variety
4451
# needs a real org bike that already carries a notification
4552
def bike_with_parking_notification

app/components/registrations/show/wrapper/component.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ module Wrapper
88
class Component < ApplicationComponent
99
# Digest of the markup inside the cache block — the cached_markup_digest spec
1010
# keeps it current, following what this tree renders out into UI:: and elsewhere
11-
MARKUP_DIGEST = "0b9c0ce42900"
11+
MARKUP_DIGEST = "298e764e84e8"
1212

1313
def initialize(bike:, current_user:, view:, available_views:, bike_sticker: nil)
1414
@bike = bike

spec/components/registrations/show/org_top_actions/wrapper/component_spec.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ def action_panels
4141

4242
context "when impounded" do
4343
let(:status) { :status_impounded }
44+
let(:impound_organization_id) { organization.id }
45+
let(:impound_record) { ImpoundRecord.new(organization_id: impound_organization_id, display_id: "0001") }
46+
let(:bike) { Bike.new(status:, cycle_type: "bike", current_impound_record: impound_record) }
4447

4548
# The owner isn't messageable, and there's no point filing a parking
4649
# notification against a bike that's already impounded
@@ -56,6 +59,24 @@ def action_panels
5659
expect(action_panels).to eq(%w[notifications_show])
5760
end
5861
end
62+
63+
context "by another organization" do
64+
let(:impound_organization_id) { FactoryBot.create(:organization).id }
65+
66+
it "renders no impound update" do
67+
expect(action_panels).to eq(%w[notifications_show])
68+
end
69+
end
70+
71+
# An unorganized impound record has a nil display_id, so rendering the form
72+
# raised UrlGenerationError rather than just linking to the wrong org
73+
context "by no organization" do
74+
let(:impound_record) { ImpoundRecord.new }
75+
76+
it "renders no impound update" do
77+
expect(action_panels).to eq(%w[notifications_show])
78+
end
79+
end
5980
end
6081

6182
context "without unstolen_notifications" do

0 commit comments

Comments
 (0)