Skip to content

Refactor AdminConsole vs. OptionalFeatureService #6828

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

labkey-adam
Copy link
Contributor

@labkey-adam labkey-adam commented Jul 8, 2025

Rationale

In the past, optional feature handling was arbitrarily split between AdminConsole (feature flag registration & retrieval) and OptionalFeatureService (feature flag enabling/disability & listener handling). This PR moves all handling to the service for consistency and implements some other clean up.

Changes

  • Move AdminConsole.OptionalFeatureFlag to a top-level class
  • Move optional feature registration and retrieval methods from AdminConsole to OptionalFeatureService/Impl
  • Move OptionalFeatureServiceImpl to core module
  • Update .gitattributes
  • DevTools action that flags PostgreSQL-only modules that still provide SQL Server upgrade scripts
  • Address miscellaneous warnings

All tests use APIs to set/unset deprecated/experimental/optional feature flags. We'd like to add a simple test that exercises the UI pages.

Tasks 📍

@labkey-chrisj
Copy link
Contributor

Manual testing notes

  • Managed to discover that the 'fuzz name of database columns' feature royally breaks the way we create biologics projects with data in them
  • exp/optional features seem unchanged in their behavior
  • I should be able to put together a quick selenium test to validate properties set/unset via the UI

@labkey-adam
Copy link
Contributor Author

Manual testing notes

  • Managed to discover that the 'fuzz name of database columns' feature royally breaks the way we create biologics projects with data in them
  • exp/optional features seem unchanged in their behavior
  • I should be able to put together a quick selenium test to validate properties set/unset via the UI

Thanks! And yeah, I provide no guarantees about or insight into the suitability of any particular experimental/optional feature 😄

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

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

Code changes look good. Not sure where the test coverage will live - if added to this PR it should get a separate review.

@@ -1590,7 +1590,7 @@ public static WebPartFactory getPortalPart(String name)
}

// The maintained view map is now case-insensitive, so this is identical to the above method
@Deprecated // Call getPortalPart() instead
@Deprecated // Call getPortalPart() instead - TODO: DELETE, unused
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be deleted now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll delete in a follow-on PR to reduce build breaks in others' pending PRs

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.

3 participants