-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat: Add gen ai org and project settings to Compass for CompassWeb usage COMPASS-8377 #6434
Conversation
packages/compass-preferences-model/src/compass-web-preferences-access.ts
Outdated
Show resolved
Hide resolved
Object.keys(_attributes).length === 1 && | ||
'optInDataExplorerGenAIFeatures' in _attributes | ||
) { | ||
return Promise.resolve(this._preferences.savePreferences(_attributes)); |
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.
At the moment this doesn't actually save the preference to the user's AppUser in Atlas since it requires a post request to mms to do that. We'll need to update the PreferencesStorage that CompassWeb uses to make the post request, and then only update the preference when the request succeeds.
preferencesStorage: new InMemoryStorage(preferencesOverrides),
I'm thinking we'll want to create a I'm thinking we could either do that work in a separate ticket/pr, or include it here.CompassWebPreferencesStorage
for that, it would effectively be an InMemoryStorage
with the same single preference optInDataExplorerGenAIFeatures
override when the updatePreferences
function is called, which would then perform the post request.
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.
Actually, one thing that hit me when looking at how we did this the initial hello
check is that we don't make preferences themselves to hit the endpoint, right? We send the request and separately update the preferences, so maybe while we're still dealing with just one extra preference here that requires a backend request, we should follow the pattern. What do you think?
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 way more sense, thanks for the suggestion. Keep the existing InMemoryStorage
and have the preference update POST request happen in the AtlasAIService and then call this savePreferences
from there when it's successful. @ruchitharajaghatta does that sound good to 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.
Chatting with Ruchitha, we'll do it in a follow up pr where we actually use the setting.
export type AtlasOrgPreferences = { | ||
enableGenAIFeaturesAtlasOrg?: boolean; | ||
}; | ||
|
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.
nit: Can we move these definitions higher up, above export type AllPreferences = UserPreferences &
on line 116 to follow the other preferences?
Object.keys(_attributes).length === 1 && | ||
'optInDataExplorerGenAIFeatures' in _attributes | ||
) { | ||
return Promise.resolve(this._preferences.savePreferences(_attributes)); |
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.
Chatting with Ruchitha, we'll do it in a follow up pr where we actually use the setting.
Description
Add the settings to Compass' preferences schema. We'll want to update how CompassWeb handles its preferences (they're currently read-only) to be able to save the user setting when a user will opt-in (the opt-in modal is added in the next ticket).
The org settings override the group (project) settings. This ticket does not involve passing these in yet from the mms frontend, just default them to off.
See tech design for more info https://docs.google.com/document/d/1to8ghEzI600WxQm08T5nzNfFLkeLDx55EtYxDb4Lvno/edit#bookmark=id.d6b8bffb4yih
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes