Skip to content

Add batch deletion to Experiments, Feature Flags, and Segments - #3323

Merged
danoswaltCL merged 67 commits into
devfrom
feature/3313-batch-delete
Nov 2, 2026
Merged

danoswaltCL merged 67 commits into
devfrom
feature/3313-batch-delete

Conversation

@zackcl

@zackcl zackcl commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

Resolves #3313 (For reference, I updated the selection checkboxes to remain visible without hover as @amurphy-cl sugggested)

Changes

  • Add always-visible row selection, select-all, and batch delete menus to the three root-page tables, excluding Global Segments. Show selection controls only to users with delete permission.
  • Preserve selections across searching, filtering, sorting, and scrolling; clear them when leaving the root page.
  • Apply consistent UI-only experiment deletion rules to root and details pages, hiding or disabling Delete for Preview, Scheduled, Running, and Paused. Retain the existing UI restrictions for Enabled feature flags and Used segments.
  • Reuse the existing text-validation confirmation modal, table loading indicators, and snackbar/error handling.
  • Add batch deletion endpoints with per-item transactions and results, continuing past missing items and stopping after execution failures. Use existing database timeout settings without a batch-specific admission deadline or lock-wait override.
  • Align batch authentication and deletion policies with the existing single-item APIs: role, state, usage, and segment-type restrictions remain UI-only. Preserve existing single-item endpoint permissions and response formats.
  • Write experiment and feature-flag deletion audit logs within their deletion transactions.
  • Fix root-table tag interactions, including experiment tag expansion and immediate segment tag filtering, and adjust pagination/loading handling for batch deletion and query changes. Also make the section-card menu button's disabled-state tooltip reachable by keyboard, which affects every card that uses that shared component.

Where the changes live

  • Shared frontend logic (core/batch-actions): selection/list tracking and delete-specific actions, state, reducer helpers, effects, and facade, composed into the three existing NgRx feature stores. withRootBatchDelete coordinates the reducer helpers, and batchDeleteFinishedEffect handles shared result notifications.
  • Shared UI: CommonSelectionCheckboxComponent renders both header and row checkboxes. RootBatchDeleteDirective connects root-page menus to the existing confirmation dialog.
  • Per-entity wiring: table columns, permissions, selection bindings, and root-only list refreshes. Experiment decision-point and metrics refreshes remain in the experiment effect.
  • Backend: one batch endpoint per controller and BatchDeleteService with per-item transactions. Existing deletion services accept an optional transaction executor, and repository reads use the supplied transaction manager. Experiment deletion uses the native path introduced by removed mooclet infrastructure, implemented native thompson-sampling … #3304; obsolete Mooclet batch dependencies and tests have been removed.
  • Tests (2,793 of the 4,949 added lines): shared selection and response validation, store/effects integration, rendered root-page UI, backend units, and database integration, including native experiment-data cleanup and rollback.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Nakagawa-master

This comment was marked as off-topic.

@zackcl

This comment was marked as resolved.

This comment was marked as resolved.

<ng-container matColumnDef="select">
<th mat-header-cell *matHeaderCellDef class="batch-select-column">
<!-- Any selection makes the next click clear; indeterminate supplies the mixed appearance. -->
<mat-checkbox

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these checkboxes are implemented exactly the same in the 3 places, and have a lot going on that would be better encapsulated as common-components, probably one for batch-action header, and one for batch-action row. additionally we'd have an easier time updating for other operations later, and/or dropping them into other tables for batch-actions there.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted CommonSelectionCheckboxComponent and reused it for both header and row checkboxes across all three tables. It owns the Material bindings, styling, tooltip/label rendering, and click/keyboard event isolation.

One presentational component covers both uses through its checked, indeterminate, and disabled inputs. Selection calculations and permissions remain with the parent tables, so the component has no deletion or NgRx dependency. The existing rendered UI tests cover both uses.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i described the reason for wanting the common-checkbox much too broadly, as this new common-checkbox-component is mostly a wrapper around the existing mat-checkbox, which isn't quite what i meant and doesn't solve what i was trying to describe, which is the repeated chunks of implementation code in the root table template. it definitely seems like those chunks of code should be able to be collapsed into one common batch-checkbox-column sort of component that encapsulates this basic selection logic.

const message = batchResultMessage('flags', counts, (key, params) => this.translate.instant(key, params));
if (!counts.hasErrors) this.notificationService.showSuccess(message);
else if (counts.deleted || counts.absent) this.notificationService.showWarning(message);
else this.notificationService.showError(message);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be missing what is in the segments / experiments "finish-batch", does it not need this little bit also?

const pathname = (this.router.url || '').split('?')[0].split('#')[0];
        return [
          // Detail selectors share the rows array; replace it only while the root table is displayed.
          ...(pathname === '/segments'
            ? [SegmentsActions.actionFetchSegments({ fromStarting: true, batchRefresh: true })]
            : []),
        ];.

These seem pretty much identical across all 3, maybe this is something that also deserves to be a common helper function kind of a thing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the /featureflags root-page check, so completion no longer requests the flags list after leaving that page.

Moved result counting, translation, and notification severity into batchDeleteFinishedEffect for all three entities. Entity-specific follow-up actions remain in their effects, including the experiment decision-point/metrics refreshes. Notifications still appear off-root. Extended the existing integration coverage to flags for leaving during deletion or refresh.

const distinct = new Map(rows.map((row) => [row.id, row]));
if (distinct.size !== rows.length) result = { ...result, [config.rowsKey]: [...distinct.values()] };
}
// Tombstones also protect against late detail/stat responses, whose IDs are not tied to a root query.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's a tombstone

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it means the removedIds collection: IDs the server confirmed were deleted or already absent. Keeping those IDs prevents an older list/detail/stat response from putting the removed data back into client state. Unknown deletion outcomes are not included.

Replaced the jargon in the comment with that explanation.

}

/** Attach the shared selection lifecycle while preserving each entity's existing detail reducers. */
export function withRootBatch<S extends { rootBatch: RootBatchState }>(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should be withRootBatchDelete, as it looks like it couldn't be reused for other batch operations

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed it to withRootBatchDelete. I also audited the related action/state/facade factories, completion and result helpers, and root directive, and made their delete-specific names explicit.

}

/** Attach the shared selection lifecycle while preserving each entity's existing detail reducers. */
export function withRootBatch<S extends { rootBatch: RootBatchState }>(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is a very chunky function, it's hard to read, can we please break this down into smaller functions? it's sort of acting like a reducer I guess? many of the ifs can have their inner logic pulled out as a smaller distinct function and make this easier to read both here and in the tests.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the wrapper into reduceBatchDeleteAction, deduplicateRows, removeConfirmedData, and resetPaginationAfterDeletion. The wrapper now shows the processing order while those helpers contain the individual operations.

It is a higher-order reducer composed with each entity's existing reducer. Kept the execution order and no-op state identity unchanged, and extended the existing integration assertions for ignored late responses and unrelated actions.

@danoswaltCL

Copy link
Copy Markdown
Collaborator

overall i have a few asks that are generally in the form of clean-up operations...

  • what can be DRY'd up even more? there's a good centralized setup that does largely do that, but there's still quite a bit of code that is copied in 3 places. especially see the suggestion for the common checkbox components, if nothing else I think that's a must for making this extensible to other operations and other section card tables, we're not going to reimplement this every time and it seems pretty much like a thing that can independently handle its own business. claude spun up something reasonable for me, so I think it's decently straightforward.

  • there are some places where the naming of things seems general towards batch operations (like withRootBatch, but contain 'delete'-specific code that would need more than a small refactor as soon as we introduce other batch operation, can we have claude give a quick sweep to doublecheck the naming of things I guess to be clear about what's generalized and what is going to probably just going to be for delete?

  • i think my only real head-scratcher here is the organization of the batch ngrx stuff... it seems to be mimicking the regular store patterns but it's kind of doing its own thing, and maybe there's a reason for that but can you explain the organization?

@zackcl

zackcl commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

@danoswaltCL Addressed the repeated checkbox and completion-notification code, split the reducer wrapper into smaller helpers, and audited the naming to distinguish selection UI from delete-specific orchestration.

On the NgRx organization: core/batch-actions contains shared functions composed into the existing Experiments, Feature Flags, and Segments stores; it is not a fourth registered feature store. Each entity owns its rootBatch state and prefixed actions. withRootBatchDelete combines the shared lifecycle with that entity's reducer, while the shared effect functions run inside its existing effect class. This avoids copying the same selection/deletion lifecycle into three stores.

I kept the directory and rootBatch field names to avoid unnecessary file/state-shape changes; the concrete types and factories now explicitly identify deletion. The common checkbox itself is independent of deletion and NgRx, so it can be reused for other selection controls.

@danoswaltCL

Copy link
Copy Markdown
Collaborator

@zackcl ty for the updates, will circle back to this once the 6.7 testing and deployment is done.

@danoswaltCL

Copy link
Copy Markdown
Collaborator

questions on the scope of this and future unknown stories to extend this to other tables / operations

  • is this meant to be a basis for all future batch-delete-selections on tables, or really just root tables / paginated-based tables? if so, I think there's still non-trivial stuff that needs DRY'd up or else there might be a lot of copy-paste (see the common component comment) for each table needed.

  • are you sure adding another operation isn't going to require a lot of refactoring?

If we're okay with potentially medium-large refactors when other things come up that we'll want to use this for, then I'm mostly good with this except for the suggestion that the common-checkbox-component be rethought as a different common component that's batch-specific.... if you don't mind, I can put up a branch off this to show what i mean?

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

# Conflicts:
#	packages/frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/experiment-root-page/experiment-root-page-content/experiment-root-section-card/experiment-root-section-card.component.html
#	packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-root-page/feature-flag-root-page-content/feature-flag-root-section-card/feature-flag-root-section-card.component.html
#	packages/frontend/projects/upgrade/src/app/features/dashboard/segments/pages/segment-root-page/segment-root-page-content/segment-root-section-card/segment-root-section-card.component.html
@danoswaltCL

Copy link
Copy Markdown
Collaborator

@zackcl please see my commit 969b825.

As mentioned above, this commit removes the common checkbox and instead does a common batch select column. That allows the parent templates to flatten a lot of template code into <app-common-batch-select-column [batch]="batch" rowLabelKey="batch.selection.experiments.row" /> and hopefully future tables can mostly drop it in that way also.

It also generalized a couple minor things like en.json keys that said batch-delete when they weren't specific to delete.

Let me know if this is ok and all still works as you had it.

@zackcl

zackcl commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

@danoswaltCL Looks good to me. I verified the behavior locally, and the frontend tests, both typechecks, and Angular build pass. Thanks!

I can't approve my own PR, so could you approve it if it looks good to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Batch Delete to Experiments, Feature Flags, and Segments Root Pages

4 participants