Deprecate and remove remaining bucket key-based GraphQL/Liquid surface

app/graphql/mutations/update_event.rb

@@ -39,16 +39,16 @@ def apply_registration_policy(event, registration_policy_attributes, bucket_key_
   end
 
   # EventChangeRegistrationPolicyService still works entirely in terms of from_key/to_key (see the
-  # comment on that service). This resolves incoming from_bucket_id/to_bucket_id args (only usable
-  # when the destination bucket already exists, since a bucket being newly created in this same
-  # edit has no id yet) down to that shape.
+  # comment on that service). This resolves incoming from_bucket_id/to_bucket_id args down to that
+  # shape -- to_bucket_id is only usable when the destination bucket already exists, since a
+  # bucket being newly created in this same edit has no id yet, so to_key is still accepted too.
   def resolve_bucket_key_mappings(event, bucket_key_mappings)
     (bucket_key_mappings || []).map { |mapping| resolve_bucket_key_mapping(event, mapping.to_h) }
   end
 
   def resolve_bucket_key_mapping(event, mapping)
     {
-      from_key: mapping[:from_key] || bucket_key_for_id(event, mapping[:from_bucket_id]),
+      from_key: bucket_key_for_id(event, mapping[:from_bucket_id]),
       to_key: mapping[:to_key] || bucket_key_for_id(event, mapping[:to_bucket_id])
     }
   end

app/graphql/types/bucket_key_mapping_input_type.rb

@@ -7,12 +7,6 @@ class Types::BucketKeyMappingInputType < Types::BaseInputObject
            required: false,
            camelize: true,
            description: "The id of the old bucket being removed or changed"
-  argument :from_key,
-           String,
-           required: false,
-           camelize: false,
-           deprecation_reason: "Use from_bucket_id instead",
-           description: "The old bucket key being removed or changed"
   argument :to_bucket_id, ID, required: false, camelize: true, description: <<~MARKDOWN
              The id of the new bucket to map to (nil means no preference). Only usable when mapping
              to a bucket that already exists -- mapping to a bucket being newly created in the same

app/graphql/types/registration_policy_bucket_type.rb

@@ -4,7 +4,11 @@ class Types::RegistrationPolicyBucketType < Types::BaseObject
 
   field :description, String, null: true, description: "A long-form description for the bucket"
   field :id, ID, null: false, description: "The ID of this bucket"
-  field :key, String, null: false, description: "The unique string identifier for this bucket"
+  field :key,
+        String,
+        null: false,
+        deprecation_reason: "Use id instead",
+        description: "The unique string identifier for this bucket"
   field :minimum_slots, Integer, null: true, description: "The minimum number of attendees needed for this bucket"
   field :name, String, null: false, description: "The name of this bucket"
   field :preferred_slots, Integer, null: true, description: "The preferred number of attendees for this bucket"

app/javascript/EventAdmin/BucketKeyRemappingModal.tsx

@@ -10,9 +10,13 @@ type BucketOption = {
   name?: string | null;
 };
 
+// A removed bucket is always a persisted row, so it always has a real id -- unlike a bucket in
+// newPolicyBuckets, which might have been added in the current, not-yet-saved edit.
+type RemovedBucketOption = BucketOption & { id: string };
+
 export type BucketKeyRemappingModalProps = {
   visible: boolean;
-  removedBuckets: BucketOption[];
+  removedBuckets: RemovedBucketOption[];
   newPolicyBuckets: BucketOption[];
   preventNoPreferenceSignups: boolean;
   onConfirm: (mappings: BucketKeyMappingInput[]) => Promise<void>;
@@ -42,22 +46,22 @@ function BucketKeyRemappingModal({
   const [prevRemovedBuckets, setPrevRemovedBuckets] = useState(removedBuckets);
   if (prevRemovedBuckets !== removedBuckets) {
     setPrevRemovedBuckets(removedBuckets);
-    setMappings(Object.fromEntries(removedBuckets.map((bucket) => [bucket.key, null])));
+    setMappings(Object.fromEntries(removedBuckets.map((bucket) => [bucket.id, null])));
   }
 
-  const setMapping = (fromKey: string, toKey: string | null) => {
-    setMappings((prev) => ({ ...prev, [fromKey]: toKey }));
+  const setMapping = (fromBucketId: string, toKey: string | null) => {
+    setMappings((prev) => ({ ...prev, [fromBucketId]: toKey }));
   };
 
   // When the new policy disallows no-preference signups, mapping a removed bucket to "no
   // preference" would leave affected signups/requests with a null requested_bucket_id that the
   // policy no longer permits new signups to have -- so every row needs an explicit bucket chosen
   // before this can be confirmed.
-  const canConfirm = !preventNoPreferenceSignups || removedBuckets.every((bucket) => mappings[bucket.key]);
+  const canConfirm = !preventNoPreferenceSignups || removedBuckets.every((bucket) => mappings[bucket.id]);
 
   const handleConfirm = async () => {
-    const bucketKeyMappings: BucketKeyMappingInput[] = Object.entries(mappings).map(([fromKey, toKey]) => ({
-      from_key: fromKey,
+    const bucketKeyMappings: BucketKeyMappingInput[] = Object.entries(mappings).map(([fromBucketId, toKey]) => ({
+      from_bucket_id: fromBucketId,
       to_key: toKey ?? undefined,
     }));
     setIsSubmitting(true);
@@ -90,13 +94,13 @@ function BucketKeyRemappingModal({
           </thead>
           <tbody>
             {removedBuckets.map((bucket) => (
-              <tr key={bucket.key}>
+              <tr key={bucket.id}>
                 <td>{bucket.name}</td>
                 <td>
                   <select
                     className="form-select"
-                    value={mappings[bucket.key] ?? ''}
-                    onChange={(e) => setMapping(bucket.key, e.target.value || null)}
+                    value={mappings[bucket.id] ?? ''}
+                    onChange={(e) => setMapping(bucket.id, e.target.value || null)}
                     disabled={isSubmitting}
                   >
                     {preventNoPreferenceSignups ? (

app/javascript/EventAdmin/useBucketKeyRemapping.tsx

@@ -2,17 +2,24 @@ import { useState, useCallback, useMemo, useRef } from 'react';
 import { BucketKeyMappingInput } from '../graphqlTypes.generated';
 import { BucketKeyRemappingModalProps } from './BucketKeyRemappingModal';
 
-type Bucket = { key: string; name?: string | null };
-type RegistrationPolicyLike = { buckets?: Bucket[]; prevent_no_preference_signups?: boolean } | null;
+type DraftBucket = { key: string; name?: string | null };
+// A bucket read from the event's already-persisted form response (as opposed to the current,
+// possibly-unsaved draft) always has a real id, since RegistrationPolicyBucket#as_json includes
+// it -- unlike a brand-new bucket in the draft, which has no id until the edit is saved.
+type PersistedBucket = DraftBucket & { id: string };
+type RegistrationPolicyLike<BucketType> = { buckets?: BucketType[]; prevent_no_preference_signups?: boolean } | null;
 
-function bucketsFromFormResponseAttrs(formResponseAttrs: { registration_policy?: unknown }): Bucket[] {
-  return (formResponseAttrs.registration_policy as RegistrationPolicyLike)?.buckets ?? [];
+function bucketsFromFormResponseAttrs<BucketType>(formResponseAttrs: { registration_policy?: unknown }): BucketType[] {
+  return (formResponseAttrs.registration_policy as RegistrationPolicyLike<BucketType>)?.buckets ?? [];
 }
 
 function preventNoPreferenceSignupsFromFormResponseAttrs(formResponseAttrs: {
   registration_policy?: unknown;
 }): boolean {
-  return (formResponseAttrs.registration_policy as RegistrationPolicyLike)?.prevent_no_preference_signups ?? false;
+  return (
+    (formResponseAttrs.registration_policy as RegistrationPolicyLike<DraftBucket>)?.prevent_no_preference_signups ??
+    false
+  );
 }
 
 type UseBucketKeyRemappingOptions = {
@@ -26,19 +33,19 @@ type UseBucketKeyRemappingOptions = {
 
 export default function useBucketKeyRemapping({ event, initialEvent, onSubmit }: UseBucketKeyRemappingOptions) {
   const [remappingModalVisible, setRemappingModalVisible] = useState(false);
-  const [removedBucketsNeedingRemapping, setRemovedBucketsNeedingRemapping] = useState<Bucket[]>([]);
+  const [removedBucketsNeedingRemapping, setRemovedBucketsNeedingRemapping] = useState<PersistedBucket[]>([]);
   const pendingResolveRef = useRef<(() => void) | null>(null);
   const pendingRejectRef = useRef<((reason?: unknown) => void) | null>(null);
 
-  const newPolicyBuckets = useMemo(() => bucketsFromFormResponseAttrs(event.form_response_attrs), [event]);
+  const newPolicyBuckets = useMemo(() => bucketsFromFormResponseAttrs<DraftBucket>(event.form_response_attrs), [event]);
   const preventNoPreferenceSignups = useMemo(
     () => preventNoPreferenceSignupsFromFormResponseAttrs(event.form_response_attrs),
     [event],
   );
 
   const updateEvent = useCallback(async () => {
     const currentBucketKeys = new Set(newPolicyBuckets.map((b) => b.key));
-    const originalBuckets = bucketsFromFormResponseAttrs(initialEvent.form_response_attrs);
+    const originalBuckets = bucketsFromFormResponseAttrs<PersistedBucket>(initialEvent.form_response_attrs);
     const keysWithRecords = new Set(initialEvent.bucket_keys_with_pending_signups_or_requests);
 
     const removedBuckets = originalBuckets.filter((b) => !currentBucketKeys.has(b.key) && keysWithRecords.has(b.key));

app/liquid_drops/registration_policy/bucket_drop.rb

@@ -6,8 +6,6 @@ class RegistrationPolicy::BucketDrop < Liquid::Drop
 
   # @!method id
   #   @return [Integer] The numeric database id of this bucket
-  # @!method key
-  #   @return [String] The unique string identifier for this bucket
   # @!method name
   #   @return [String] The name of this bucket
   # @!method description
@@ -31,7 +29,6 @@ class RegistrationPolicy::BucketDrop < Liquid::Drop
   #   @return [Boolean] Whether or not to allow other attendees to see that a person is in this
   #                     bucket in the signup summary page
   delegate :id,
-           :key,
            :name,
            :description,
            :minimum_slots,

test/javascript/EventAdmin/BucketKeyRemappingModal.test.tsx

@@ -53,7 +53,7 @@ describe('BucketKeyRemappingModal', () => {
       wrap(
         <BucketKeyRemappingModal
           visible
-          removedBuckets={[{ key: 'signups', name: 'Cats' }]}
+          removedBuckets={[{ id: '1', key: 'signups', name: 'Cats' }]}
           newPolicyBuckets={[{ key: 'dogs', name: 'Dogs' }]}
           preventNoPreferenceSignups={false}
           onConfirm={onConfirm}
@@ -65,7 +65,7 @@ describe('BucketKeyRemappingModal', () => {
 
     fireEvent.click(getByText('Apply and save event'));
 
-    await waitFor(() => expect(onConfirm).toHaveBeenCalledWith([{ from_key: 'signups', to_key: undefined }]));
+    await waitFor(() => expect(onConfirm).toHaveBeenCalledWith([{ from_bucket_id: '1', to_key: undefined }]));
   });
 
   test('submits the selected destination bucket when one is chosen', async () => {
@@ -75,7 +75,7 @@ describe('BucketKeyRemappingModal', () => {
       wrap(
         <BucketKeyRemappingModal
           visible
-          removedBuckets={[{ key: 'signups', name: 'Cats' }]}
+          removedBuckets={[{ id: '1', key: 'signups', name: 'Cats' }]}
           newPolicyBuckets={[{ key: 'dogs', name: 'Dogs' }]}
           preventNoPreferenceSignups={false}
           onConfirm={onConfirm}
@@ -88,7 +88,7 @@ describe('BucketKeyRemappingModal', () => {
     fireEvent.change(getByRole('combobox', { hidden: true }), { target: { value: 'dogs' } });
     fireEvent.click(getByText('Apply and save event'));
 
-    await waitFor(() => expect(onConfirm).toHaveBeenCalledWith([{ from_key: 'signups', to_key: 'dogs' }]));
+    await waitFor(() => expect(onConfirm).toHaveBeenCalledWith([{ from_bucket_id: '1', to_key: 'dogs' }]));
   });
 
   describe('when the new policy disallows no-preference signups', () => {
@@ -99,7 +99,7 @@ describe('BucketKeyRemappingModal', () => {
         wrap(
           <BucketKeyRemappingModal
             visible
-            removedBuckets={[{ key: 'signups', name: 'Cats' }]}
+            removedBuckets={[{ id: '1', key: 'signups', name: 'Cats' }]}
             newPolicyBuckets={[{ key: 'dogs', name: 'Dogs' }]}
             preventNoPreferenceSignups
             onConfirm={onConfirm}
@@ -121,7 +121,7 @@ describe('BucketKeyRemappingModal', () => {
         wrap(
           <BucketKeyRemappingModal
             visible
-            removedBuckets={[{ key: 'signups', name: 'Cats' }]}
+            removedBuckets={[{ id: '1', key: 'signups', name: 'Cats' }]}
             newPolicyBuckets={[{ key: 'dogs', name: 'Dogs' }]}
             preventNoPreferenceSignups
             onConfirm={onConfirm}
@@ -139,7 +139,7 @@ describe('BucketKeyRemappingModal', () => {
 
       fireEvent.click(getByText('Apply and save event'));
 
-      await waitFor(() => expect(onConfirm).toHaveBeenCalledWith([{ from_key: 'signups', to_key: 'dogs' }]));
+      await waitFor(() => expect(onConfirm).toHaveBeenCalledWith([{ from_bucket_id: '1', to_key: 'dogs' }]));
     });
   });
 });