-
Notifications
You must be signed in to change notification settings - Fork 357
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
refactor: [M3-8064] - Clean up Database Feature Flagging Logic #10435
refactor: [M3-8064] - Clean up Database Feature Flagging Logic #10435
Conversation
Coverage Report: ❌ |
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.
Do we need to do the same for the GoTo menu? it's an edge case but while we're at it...
* (which is restricted users with no billing access), | ||
* we must check if they can load Database Engines as a workaround. | ||
* If these users can successfully fetch database engines, we will | ||
* show databases. |
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.
Can you clarify why we'd want to show dbaas for restricted users with no billing access?
This PR just moves the fetching to this util (so I don't think we're fetching more than before) but it would be nice to clarify cause I don't think that's something we do for other entities?
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 tried to improve the clarification. In summary, Databases are end of sale so they only should show for users who we know for sure have the account capability. The restricted user case is kind of wacky because a restricted user may or may not have access to GET /v4/account
Added Databases to the GoTo menu ✅
export const useIsDatabasesEnabled = () => { | ||
const { data: account } = useAccount(); | ||
|
||
// If the flag is off AND we don't have permission to GET /v4/account, |
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.
Is specifying whether the flag is off relevant anymore? We could probably omit those words now.
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 call, removed ✅
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.
Confirmed:
- Databases are visible for an unrestricted user with the API capability
- Databases are visible for a restricted user without access to /account the API capability
- Without the capability, Databases is not visible in the nav, top menu, etc.
It looks like you have a merge conflict to resolve, but otherwise LGTM. Nits for typos.
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com> Co-authored-by: Jaalah Ramos <125309814+jaalah-akamai@users.noreply.github.com>
Description 📝
isFeatureEnabled
👋Preview 📷
Note
No UI changes expected
How to test 🧪
Prerequisites
Important
Please test as an unrestricted user and a restricted user
dbaas-eos-opt-in
customer tag needed)Note
To gracefully migrate off of
isFeatureEnabled
, we no longer check forflags.databases
. The logic will be completely based on the API's customer capabilities. After we release this, we can turnflags.databases
on in production and come back and useisFeatureEnabledV2
with the flagAs an Author I have considered 🤔