Skip to content

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

Draft
zackcl wants to merge 61 commits into
devfrom
feature/3313-batch-delete
Draft

zackcl wants to merge 61 commits into
devfrom
feature/3313-batch-delete

Conversation

@zackcl

@zackcl zackcl commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Waiting for #3304 to merge. Once it lands, this branch will be updated with the latest dev, merge conflicts resolved, and obsolete Mooclet dependencies removed from batch deletion and its tests. The combined changes will be verified before this PR is marked ready for review.


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

Changes

  • Add 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 batch module (core/batch-actions, the root-page directive, ~720 lines): selection state, reducer, effects, and facade. Each entity store attaches it through one wrapper so the three tables cannot drift apart.
  • Backend (~570 lines): one batch endpoint per controller, BatchDeleteService with per-item transactions, and an optional transaction parameter threaded through the existing delete services so single and batch deletion share one path. FeatureFlagService.findOneForDetails moved to FeatureFlagRepository unchanged so the flag delete can read inside its transaction.
  • Per-entity wiring (~740 lines): the same checkbox column, menu, and store plumbing repeated for Experiments, Feature Flags, and Segments.
  • Tests (2,823 of the 4,926 added lines): batch store/effects integration, root-page UI, backend unit, and backend integration suites.

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.

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.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@Nakagawa-master Nakagawa-master left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is a server-side authorization mismatch here that looks more fundamental than the stale-selection UI cases already discussed.

The current frontend permission matrix treats delete as an authority boundary:

  • ADMIN / CREATOR: delete experiments, flags, segments
  • USER_MANAGER: no experiment/flag delete, segment delete allowed
  • READER: no delete for any of the three

But the current backend path does not enforce that matrix. The class-level @Authorized() only authenticates: authorizationChecker ignores the roles argument and returns true after attaching any validated user. BatchDeleteService.delete/deleteOne then calls the delete services without a role/permission check.

The integration suite makes the behavior explicit:

test.each(Object.values(UserRole))(
  'matches single-delete behavior for the %s role',
  async (role) => {
    ...
    await request(app).delete(...).expect(200)
    await request(app).post(batchRoute).expect(200)
    ...
    expect(rows).toBeDeleted()
  }
)

and later specifically verifies a UserRole.READER can use the single-delete route successfully.

That contradicts the thread statement that “the batch API still checks the user's current database role on each request and rejects requests without delete permission with 403.” A stale UI is therefore not just able to show Delete after a downgrade; a Reader/User Manager can call the destructive endpoint directly and the server accepts it.

Because this PR introduces a bulk destructive surface, I would make the server—not the UI—the authority for this rule. Ideally use one shared backend policy for both single and batch deletes so the two routes cannot drift:

experiments: ADMIN | CREATOR
flags:       ADMIN | CREATOR
segments:    ADMIN | CREATOR | USER_MANAGER
READER:      none

(or whatever role matrix is actually intended).

The discriminating integration coverage would be:

  1. authenticate as each role using the persisted/current DB role;
  2. attempt both single and batch deletion for each entity;
  3. allowed combinations delete and audit normally;
  4. disallowed combinations return 403 and leave the target, owned lists/members, caches/external-sync side effects, and deletion audit unchanged;
  5. downgrade the same account after the UI has loaded and prove the next request is rejected without relying on a client refresh.

If the product intentionally allows every authenticated role to delete through the API, then the frontend UserPermission.*.delete values and the “server rejects with 403” assumption should be renamed/documented as presentation-only, because today they read as authorization that the server does not enforce.

@zackcl

zackcl commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator Author

@Nakagawa-master No deletion route has ever enforced a role, so this isn't something the PR introduces. An earlier revision of this branch did guard the batch endpoints, but I took it out so batch and single delete behave the same way instead of having two different rules for the same targets. The statement you quoted about the batch API returning 403 was accurate while that guard existed, and it's out of date now. I've opened #3326 to enforce the role matrix and the state rules on both routes.

This comment was marked as resolved.

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

3 participants