-
Notifications
You must be signed in to change notification settings - Fork 15
Implement Status-Based UI Restrictions with UI Refinements #2840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: experiment-design-refresh
Are you sure you want to change the base?
Implement Status-Based UI Restrictions with UI Refinements #2840
Conversation
…feature/2809-experiment-status-restrictions-3
…feature/2809-experiment-status-restrictions-3
…c display name as the payload table
…t in the common list table
…ser has update permission
…feature/2809-experiment-status-restrictions-3
…d segment's lists section cards
…processParentSegments selector
…mponents and fix import list modal headings
…feature/2809-experiment-status-restrictions-3
…icipant list table
There was a problem hiding this 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.
|
Good question. I think we should confirm this with April. |
…feature/2809-experiment-status-restrictions-3
|
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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@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. |
|
no no revert or anything it's ok. i'll focus on completing review today. |
| </app-common-section-card-action-buttons> | ||
|
|
||
| <!-- content --> | ||
| @if (isSectionCardExpanded) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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. |
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
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