Skip to content

Conversation

@zackcl
Copy link
Collaborator

@zackcl zackcl commented Jan 13, 2026

Resolves #2809

This PR primarily implements status- and permission-based UI restrictions.

Note: While working on the restrictions, several small but related UI inconsistencies were discovered while testing the same table components, and were refined to keep behavior consistent.

Here's an overview of changes:

Restricted menu items, edit fields, and action items on detail pages based on status and permissions, with dedicated tooltip support

  • Add experiment status restriction helper and apply across details page components (16bf64f)
  • Use getRawValue() to include disabled fields in experiment edit requests (ad163d6)
  • Add conditional action buttons with tooltip support to experiment table components (54fac94)
  • Hide Archive Experiment menu option to users without update permission (0c8211a)
  • Add Archived status to getDisabledFields (64e61d6)
  • Treat disabled Mooclet parameters form as valid so edit modal save stays enabled (1a12c71)
  • Hide action elements in archived experiments via shouldHideActions restriction (0e07c51)
  • Update overview details section card to display action buttons when user has update permission (43f4d43)
  • Apply action restrictions for feature flag's include/exclude lists and segment's lists section cards (23c1bbc)
  • Hide the import list menu option to users without segment update permission (dd89027)
  • Filter details page menu items by permissions instead of hiding menu button for readers (07be679)
  • Disable export buttons when table is empty across all list section cards (cd6be58)
  • Disable weight-edit button when conditions table is empty with custom tooltip (2aea695)

Refined section card table styles, applied text truncation with tooltip support to all tables, and added description to Used By and Lists tables for consistent pattern

  • Fix wrong section card table styles caused by Angular 20 update (fb23519)
  • Update section card table column widths (b3b1a7a)
  • Apply the same truncation behavior to condition description and metric display name as the payload table (68d6d78)
  • Add TextOverflowTooltip directive to table components for dynamic text truncation (eb472c0)
  • Apply tooltip for displaying values in the common list table (bc0ced2)
  • Fix status chip styling by removing toLowerCase() call in segment used-by table (f178827)
  • Adjust the column widths for conditions table (00c38ac)
  • Update no payloads message for the payloads table (0e73278)
  • Adjust conditions and metrics tables column widths (ae50b11)
  • Fix no-data row height for Reward Feedback table (fc969f6)
  • Update Reward Feedback table styles for consistency (90cef82)
  • refactor: Replace truncate pipe with global line-clamp utility classes and appTextOverflowTooltip directive (76147f1)
  • feat: Add description to segment-used-by table and ensure consistent description handling across all tables (3910475)
  • Include description field in segment exclusion queries (for used by table) (a721bc1)
  • Fix segment status chip styling by converting status to lowercase in processParentSegments selector (29fb64b)
  • Update participant list table to display description below name (7dfd10d)
  • Apply text truncation and tooltip pattern to all section card table components (ca9c04e)
  • Update reward feedback table style for consistency (4083759)
  • Update reward feedback table column widths and alignments (1fc7c8c)
  • Apply clamp and tooltip to reward feedback condition column text (8fdd058)
  • Apply text truncation and tooltip pattern to root and modal table components and fix import list modal headings (a8e94ec)
  • Fix payloads table no-data row padding and vertical align (1fb7cbc)
  • Fix the backend segment inclusion exclusion test failures (13769b6)
  • Update the last column heading and cell formatting in root tables for consistency (31d4753)
  • Set min-width for weight column in conditions table (90efe4b)

@zackcl zackcl requested review from bcb37 and danoswaltCL January 13, 2026 14:07
…feature/2809-experiment-status-restrictions-3
@zackcl zackcl marked this pull request as ready for review January 14, 2026 14:27
@zackcl zackcl marked this pull request as draft January 15, 2026 15:39
@zackcl zackcl marked this pull request as ready for review January 15, 2026 18:00
Copy link
Collaborator

@bcb37 bcb37 left a comment

Choose a reason for hiding this comment

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

This all looks good to me and works as far as I can tell. One question about the new behavior: do we want to disallow editing metrics (queries) in the 'Completed' state? Even though it's the old 'Cancelled' state, it feels more like 'Enrollment Complete' in terms with how it will be used, and researchers might want to look at different aggregations of the data.

@zackcl
Copy link
Collaborator Author

zackcl commented Jan 15, 2026

Good question. I think we should confirm this with April.

…feature/2809-experiment-status-restrictions-3
@danoswaltCL
Copy link
Collaborator

yeah i'd agree, metrics queries definitely make sense to remain editable

.leftJoin('featureFlagSegmentExclusion.segment', 'segment')
.leftJoinAndSelect('segment.subSegments', 'subSegments')
.addSelect('featureFlag.name')
.addSelect('featureFlag.description')
Copy link
Collaborator

Choose a reason for hiding this comment

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

where do the descriptions relate to permissions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. This part isn’t directly related to permissions.

The description field was added to support a small UI consistency improvement. On the root pages we already show name + description in the tables, so I applied the same pattern to the Segment Used By and Lists tables as well. To support that, the backend query needed to include the description.

If you feel strongly that this shouldn’t live in this PR, happy to discuss the best way to handle it, but the intent was to keep the table patterns consistent rather than introduce a new behavior.

@danoswaltCL
Copy link
Collaborator

@zackcl no more styling/fixup commits please! it's looking good for sure and I'm wading my way through but this is a lot to look through already with just the permissions-related code. I have no idea what all this changes, it touches so many things.

@zackcl zackcl changed the title Implement Status-Based UI Restrictions Implement Status-Based UI Restrictions with UI Refinements Jan 16, 2026
@zackcl
Copy link
Collaborator Author

zackcl commented Jan 16, 2026

@zackcl no more styling/fixup commits please! it's looking good for sure and I'm wading my way through but this is a lot to look through already with just the permissions-related code. I have no idea what all this changes, it touches so many things.

Good call, and sorry for the confusion and the review overhead, I should’ve explained this earlier.

This PR started as status/permission-based UI restrictions. While testing those changes end-to-end, I ran into several related UI inconsistencies in the same table components and addressed them to keep the behavior consistent. That ended up expanding the scope more than expected.

I’ve updated the PR title and added a detailed overview to clearly separate the restriction work from the UI refinements. I won’t add any more styling/fixup commits to this PR, and I’m happy to revert or adjust any UI changes that feel unnecessary.

@danoswaltCL
Copy link
Collaborator

no no revert or anything it's ok. i'll focus on completing review today.

</app-common-section-card-action-buttons>

<!-- content -->
@if (isSectionCardExpanded) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we haven't migrated the rest of the codebase to this new syntax yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used @if/@let to simplify the template while I was already touching this file.

I noticed that @if is already used in edit-condition-weights-modal.component.html, so I assumed it was OK to use the new syntax here as well. Are we avoiding introducing @if/@let until we do a broader migration?

If so, I can revert the @if blocks back to *ngIf to keep things consistent, but I’d like to keep the @let bindings since they reduce duplication and make the template easier to read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah we've talked about waiting so we're not mixing things, but we should switch over soon enough so disregard.

…y and consistency (will be visible on details page)
@@ -0,0 +1,110 @@
/**
* Pure helper functions for determining UI restrictions based on experiment status.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why pure functions instead of a normal service class? This is a little out of place in angular, to be importing functions into components.

Copy link
Collaborator Author

@zackcl zackcl Jan 16, 2026

Choose a reason for hiding this comment

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

I kept these as pure functions because the logic is fully deterministic and has no dependencies, so it felt more like a util than a DI service, and it keeps the VM mapping pretty minimal across several components.

I agree the current *.service.ts naming is confusing though. How about I rename/move this to something like experiment-status-restrictions.utils.ts to better reflect that it’s just pure helpers? Does that work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're right that these don't need to be in a service, but they I think the best place for them is to turn them into selectors. I think they fit perfectly for that, that's where 'pure utility functions' make most sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense. These are essentially derived UI state, so selectors are a good fit conceptually.

Given the size of this PR, I’d prefer to keep the current pure helpers for now and follow up with a small PR to move this into selectors if that feels like the right direction. Thanks for the suggestion.

@danoswaltCL
Copy link
Collaborator

I am still going through but I doubt I'll have other major requests. I think the vm$ approach is kinda nice btw, I don't think that's a pattern we've really used explicitly but perhaps we should have been, especially for the section cards makes it easy to keep changes within the reactive/observable chain.

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.

4 participants