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

Add get and update user plugin preferences rpcs #2471

Merged
merged 8 commits into from
Oct 3, 2023
Merged

Conversation

gilwong00
Copy link
Contributor

This adds two new endpoints GetUserPluginPreferences and UpdateUserPluginPreferences to the UserService

@gilwong00 gilwong00 requested a review from bufdev as a code owner October 2, 2023 17:37
@gilwong00 gilwong00 requested review from mfridman and bufdev and removed request for bufdev October 2, 2023 17:37
@@ -53,6 +54,19 @@ message OrganizationUser {
OrganizationRoleSource organization_role_source = 4;
}

message UserSelectedPlugin {
Copy link
Member

Choose a reason for hiding this comment

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

Is there an existing type for this? What is a user-selected plugin? Needs docs explaining what this is at the minimum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can actually dont need this type anymore. Removed.

string name = 2;
}

message UserPluginPreference {
Copy link
Member

Choose a reason for hiding this comment

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

What is this? needs docs at the minimum

Copy link
Member

Choose a reason for hiding this comment

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

That is this used for? The documentation just says what the fields are, I'm not sure what this structure is for, and how it fits into the Buf product.


message UserPluginPreference {
string user_id = 1;
PluginLanguage selected_language = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Asymmetry between UserSelectedPlugin having Selected in it vs. this not, likely could be explained by understanding what API is and is for via docs

@gilwong00 gilwong00 requested a review from bufdev October 2, 2023 23:35
@gilwong00
Copy link
Contributor Author

@bufdev cleaned up this PR and added more docs around the new types

@@ -53,6 +54,19 @@ message OrganizationUser {
OrganizationRoleSource organization_role_source = 4;
}

// UserPluginPreference is a nested structure that contains the
Copy link
Member

Choose a reason for hiding this comment

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

Please use complete sentences (with periods).

@@ -53,6 +54,19 @@ message OrganizationUser {
OrganizationRoleSource organization_role_source = 4;
}

// UserPluginPreference is a nested structure that contains the
Copy link
Member

Choose a reason for hiding this comment

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

What is nested in this context?

message UserPluginPreference {
// The id of the user
string user_id = 1;
// The selected language that the user has indicidated
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what a preferred language for plugins is. What is this used for? How does this fit into the Buf product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the new Generated SDKs tab. We changed the flow so a user first selects their preferred language and then we display a list of plugins pertaining to that language. We want to persist the language and the plugins the user selected so the next time they visit this page we dont need to have them go through the selection process again

Copy link
Member

Choose a reason for hiding this comment

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

The point here is that any context should be added to the docs - I should be able to read these docs and understand immediately what is going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Added a explanation that this type is meant for Generated SDKs.

@@ -105,6 +119,12 @@ service UserService {
}
// UpdateUserSettings update the user settings including description.
rpc UpdateUserSettings(UpdateUserSettingsRequest) returns (UpdateUserSettingsResponse);
// GetUserPluginPreferences gets plugin preferences for the user.
Copy link
Member

Choose a reason for hiding this comment

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

What are plugin preferences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plugin preferences is the language and the list of plugins a user has selected in the new Generated SDKs tab

Copy link
Member

Choose a reason for hiding this comment

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

Should be in docs

@gilwong00 gilwong00 requested a review from bufdev October 3, 2023 17:55
@@ -53,6 +54,17 @@ message OrganizationUser {
OrganizationRoleSource organization_role_source = 4;
}

// UserPluginPreference contains the users selected language as well as a list of plugins selected for that language.
// This is used for Generate SDKs where the user can choose a language and corresponding plugins to be persisted.
Copy link
Member

Choose a reason for hiding this comment

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

I still don't know what this means - what does it mean for a plugin to be persisted? What does it mean to choose a language?

I'd check out https://github.com/bazelbuild/remote-apis/blob/main/build/bazel/remote/asset/v1/remote_asset.proto as a good example of Protobuf API documentation.

@gilwong00 gilwong00 requested a review from bufdev October 3, 2023 19:21
@gilwong00 gilwong00 merged commit e4afbcf into main Oct 3, 2023
@gilwong00 gilwong00 deleted the gwong/GRW-1522 branch October 3, 2023 20:56
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.

2 participants