refactor: [M3-8064] - Clean up Database Feature Flagging Logic#10435
Conversation
bnussman-akamai
requested review from
abailly-akamai and
mjac0bs
and removed request for
a team
|
Coverage Report: ❌ |
abailly-akamai
left a comment
abailly-akamai
left a comment
There was a problem hiding this comment.
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.
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.
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.
Is specifying whether the flag is off relevant anymore? We could probably omit those words now.
There was a problem hiding this comment.
Good call, removed ✅
mjac0bs
left a comment
mjac0bs
left a comment
There was a problem hiding this comment.
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>
5 participants
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-incustomer 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.databaseson in production and come back and useisFeatureEnabledV2with the flagAs an Author I have considered 🤔