Skip to content

Add get and update user plugin preferences rpcs#2471

Merged
gilwong00 merged 8 commits intomainfrom
gwong/GRW-1522
Oct 3, 2023
Merged

Add get and update user plugin preferences rpcs#2471
gilwong00 merged 8 commits intomainfrom
gwong/GRW-1522

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 bufdev and mfridman and removed request for bufdev October 2, 2023 17:37
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

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).

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.

}
// 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
}

// 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