Repository navigation
Add batch deletion to Experiments, Feature Flags, and Segments - #3323
Conversation
… ineligible items
…able after failures
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 }>( |
There was a problem hiding this comment.
maybe this should be withRootBatchDelete, as it looks like it couldn't be reused for other batch operations
There was a problem hiding this comment.
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 }>( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
overall i have a few asks that are generally in the form of clean-up operations...
|
|
@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: I kept the directory and |
|
@zackcl ty for the updates, will circle back to this once the 6.7 testing and deployment is done. |
|
questions on the scope of this and future unknown stories to extend this to other tables / operations
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
|
@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 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. |
|
@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? |
Resolves #3313 (For reference, I updated the selection checkboxes to remain visible without hover as @amurphy-cl sugggested)
Changes
Where the changes live
core/batch-actions): selection/list tracking and delete-specific actions, state, reducer helpers, effects, and facade, composed into the three existing NgRx feature stores.withRootBatchDeletecoordinates the reducer helpers, andbatchDeleteFinishedEffecthandles shared result notifications.CommonSelectionCheckboxComponentrenders both header and row checkboxes.RootBatchDeleteDirectiveconnects root-page menus to the existing confirmation dialog.BatchDeleteServicewith 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.