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

fix(uninstall): Only clear credentials if the Cody app was uninstalled #2485

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jamesmcnamara
Copy link
Contributor

@jamesmcnamara jamesmcnamara commented Oct 19, 2024

Take two of CODY-1043 that was reverted in #2484.

The issue is that uninstall events are triggered when any plugin is uninstalled and not just when the plugin that registered the handler is uninstalled. This checks that the plugin being uninstalled is in fact Cody before running the handler.

Test plan

I have manually tested that when uninstalling other extensions one branch of the if runs, and when uninstalling Cody the other half does.

As discussed in #2434 it would be non-trivial to test this behavior because we will need to drive the installation and uninstallation of the plugin from an automated test driver, which will likely require a much more custom test kit.

@jamesmcnamara jamesmcnamara requested a review from a team October 19, 2024 00:34
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

Because the previous iteration of this caused a regression, let's add a test for this.

@jamesmcnamara
Copy link
Contributor Author

@dominiccooney Is this something that would be easy to test with the existing integration tests?

@dominiccooney
Copy link
Contributor

Is this something that would be easy to test with the existing integration tests?

@jamesmcnamara I would think about it this way:

  1. A unit test. How hard would it be to dispatch that event to the plugin when the plugin does and doesn't match?
  2. An integration test. This will be more valuable. To get some plugins installed I'd just look at how the test framework does it. For driving an uninstall, you could see if there are JetBrains APIs for this or failing that use the UI robot to actually go click on uninstall. You will need a second plugin to be the thing that you uninstall.

@@ -12,13 +12,18 @@ import com.sourcegraph.cody.agent.protocol.BillingProduct
import com.sourcegraph.cody.agent.protocol.TelemetryEventParameters
import com.sourcegraph.cody.config.CodyAuthenticationManager
import com.sourcegraph.cody.telemetry.TelemetryV2
import com.sourcegraph.config.ConfigUtil
import java.util.concurrent.TimeUnit

class UninstallListener : StartupActivity {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed it looks like it is currently broken:

image

@pkukielka
Copy link
Contributor

Is this something that would be easy to test with the existing integration tests?

@jamesmcnamara I would think about it this way:

  1. A unit test. How hard would it be to dispatch that event to the plugin when the plugin does and doesn't match?
  2. An integration test. This will be more valuable. To get some plugins installed I'd just look at how the test framework does it. For driving an uninstall, you could see if there are JetBrains APIs for this or failing that use the UI robot to actually go click on uninstall. You will need a second plugin to be the thing that you uninstall.

For unit test we can use PluginStateManager.fireState(pluginDescriptor, false) it is what PluginInstaller::prepareToUninstall calls under the hood. Actually it is possible that PluginInstaller::prepareToUninstall would work out of the box as wel.

@jamesmcnamara
Copy link
Contributor Author

@dominiccooney @pkukielka I've followed your advice and added a unit test using PluginInstaller::prepareToUninstall for both cases. I looked at writing an integration test but couldn't see how to do it easily.

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.

3 participants