-
Notifications
You must be signed in to change notification settings - Fork 14
fix: allow setting a blank org [IDE-1566] #1115
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
base: main
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @rrama, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug where setting a blank organization in the IDE did not take immediate effect, requiring a restart. The changes ensure that the system correctly interprets and applies empty or whitespace organization inputs, allowing users to seamlessly revert to their default organization without needing to restart the IDE. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request modifies the configuration update logic to allow setting a blank organization, which resets it to the user's default. The change in updateOrganization is correct and achieves this by removing the check for a non-empty organization string. The accompanying tests have been updated to verify this new behavior for both empty and whitespace strings. I've suggested a small refactoring in the test file to reduce code duplication by using a table-driven test, which will improve maintainability.
| t.Run("blank organization resets to default organization", func(t *testing.T) { | ||
| c := testutil.UnitTest(t) | ||
| di.TestInit(t) | ||
|
|
||
| defaultOrgId := "00000000-0000-0000-0000-000000000002" | ||
| // Set an immutable default value to avoid API calls | ||
| c.Engine().GetConfiguration().AddDefaultValue(configuration.DEFAULT_ORGANIZATION, configuration.ImmutableDefaultValueFunction(defaultOrgId)) | ||
|
|
||
| // Set to a specific org first | ||
| c.SetOrganization(expectedOrgId) | ||
| assert.Equal(t, expectedOrgId, c.Organization()) | ||
|
|
||
| UpdateSettings(c, types.Settings{Organization: " "}, analytics.TriggerSourceTest) | ||
| // Set to empty - should reset to default | ||
| // Include another setting to avoid early return on empty settings struct | ||
| UpdateSettings(c, types.Settings{Organization: "", RiskScoreThreshold: utils.Ptr(100)}, analytics.TriggerSourceTest) | ||
|
|
||
| assert.Equal(t, defaultOrgId, c.Organization()) | ||
| }) | ||
|
|
||
| t.Run("whitespace organization resets to default organization", func(t *testing.T) { | ||
| c := testutil.UnitTest(t) | ||
| di.TestInit(t) | ||
|
|
||
| defaultOrgId := "00000000-0000-0000-0000-000000000002" | ||
| // Set an immutable default value to avoid API calls | ||
| c.Engine().GetConfiguration().AddDefaultValue(configuration.DEFAULT_ORGANIZATION, configuration.ImmutableDefaultValueFunction(defaultOrgId)) | ||
|
|
||
| // Set to a specific org first | ||
| c.SetOrganization(expectedOrgId) | ||
| assert.Equal(t, expectedOrgId, c.Organization()) | ||
|
|
||
| // Set to single space - should be trimmed to empty and reset to default | ||
| UpdateSettings(c, types.Settings{Organization: " "}, analytics.TriggerSourceTest) | ||
|
|
||
| assert.Equal(t, defaultOrgId, c.Organization()) | ||
| }) |
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.
These two test cases, blank organization resets to default organization and whitespace organization resets to default organization, contain a lot of duplicated setup code. To improve maintainability and reduce redundancy, consider refactoring them into a single table-driven test. This would make the test cleaner and easier to extend with more 'blank' value cases in the future.
t.Run("setting a blank organization should reset to default", func(t *testing.T) {
defaultOrgId := "00000000-0000-0000-0000-000000000002"
testCases := []struct {
name string
settings types.Settings
}{
{
name: "empty string",
// Include another setting to avoid early return on empty settings struct
settings: types.Settings{Organization: "", RiskScoreThreshold: utils.Ptr(100)},
},
{
name: "whitespace string",
settings: types.Settings{Organization: " "},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
c := testutil.UnitTest(t)
di.TestInit(t)
// Set an immutable default value to avoid API calls
c.Engine().GetConfiguration().AddDefaultValue(configuration.DEFAULT_ORGANIZATION, configuration.ImmutableDefaultValueFunction(defaultOrgId))
// Set to a specific org first
c.SetOrganization(expectedOrgId)
assert.Equal(t, expectedOrgId, c.Organization())
// Update settings with blank org
UpdateSettings(c, tc.settings, analytics.TriggerSourceTest)
// Assert it resets to default
assert.Equal(t, defaultOrgId, c.Organization())
})
}
})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's almost as many lines of code, so probably won't do.
Description
Previously you had to restart the IDE to go back to actually using the blank org (the user's preferred org from the web UI).
Relies on snyk/go-application-framework#518
Checklist
make generate)make lint-fix)