Skip to content
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

Display a warning icon when no enabled include lists defined #1892

Closed
wants to merge 2 commits into from

Conversation

zackcl
Copy link
Collaborator

@zackcl zackcl commented Sep 3, 2024

Resolves #1841, resolves #1838

This PR is for displaying a warning icon on the Feature Flags root page and details page when an enabled feature flag has no enabled include lists defined.

Changes made:

  • Updated the api/flags/paginated endpoint to include hasEnabledIncludeList in the response.
  • Updated the selectors for retrieving the warning state in feature-flags.selector.ts.
  • Modified this.featureFlagService.fetchFeatureFlags() to this.featureFlagService.fetchFeatureFlags(true) in the FeatureFlagRootSectionCardComponent. This ensures that flags are re-fetched from api/flags/paginated whenever the user navigates back to the root page.
  • Updated the warning tooltip message to "No include lists enabled."

@zackcl zackcl marked this pull request as ready for review September 3, 2024 15:13
@zackcl
Copy link
Collaborator Author

zackcl commented Sep 3, 2024

Although this PR appears to be functioning, I'm not entirely confident that this is the optimal/proper way to include hasEnabledIncludeList in the response from the api/flags/paginated endpoint, especially after addressing the test case failures in FeatureFlagService.test.ts.

I would greatly appreciate if someone could review the changes I made to the findPaginated() function to return hasEnabledIncludeList as well as the test cases. Please feel free to make suggestions or commit changes to this PR.

@@ -115,7 +115,7 @@ export class FeatureFlagService {
return this.featureFlagRepository.count();
}

public findPaginated(
public async findPaginated(
skip: number,
take: number,
logger: UpgradeLogger,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the return type
Promise<Array<FeatureFlag & { hasEnabledIncludeList: boolean }>> and update in the Controller accordingly

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add this change or we can revisit this after updating the Typeorm 3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants