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

Restore last connection date times #14134

Merged
merged 2 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/api.internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,21 @@ declare module './api' {
* If this is the only quick pick item in the list and this is true, then this item will be selected by default.
*/
default?: boolean;
/**
* The Jupyter Server command associated with this quick pick item.
*/
command?: JupyterServerCommand;
})[]
>
| (QuickPickItem & {
/**
* If this is the only quick pick item in the list and this is true, then this item will be selected by default.
*/
default?: boolean;
/**
* The Jupyter Server command associated with this quick pick item.
*/
command?: JupyterServerCommand;
})[];
}
}
3 changes: 3 additions & 0 deletions src/api.unstable.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ declare module './api' {
*/
default?: boolean;
})[];
/**
* @param item The original quick Pick returned by getQuickPickEntryItems will be passed into this method.
*/
handleQuickPick?(item: QuickPickItem, backEnabled: boolean): Promise<string | 'back' | undefined>;
/**
* Given the handle, returns the Jupyter Server information.
Expand Down
21 changes: 16 additions & 5 deletions src/kernels/jupyter/connection/jupyterServerProviderRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ class JupyterUriProviderAdaptor extends Disposables implements IJupyterUriProvid
);
}
}
async getQuickPickEntryItems(value?: string): Promise<(QuickPickItem & { default?: boolean | undefined })[]> {
async getQuickPickEntryItems(
value?: string
): Promise<(QuickPickItem & { default?: boolean | undefined; command?: JupyterServerCommand })[]> {
if (!this.provider.commandProvider) {
throw new Error(`No Jupyter Server Command Provider for ${this.provider.extensionId}#${this.provider.id}`);
}
Expand All @@ -116,8 +118,10 @@ class JupyterUriProviderAdaptor extends Disposables implements IJupyterUriProvid
return items.map((c) => {
return {
label: c.title,
detail: c.detail,
tooltip: c.tooltip,
default: c.picked === true
default: c.picked === true,
command: c
};
});
} catch (ex) {
Expand All @@ -130,14 +134,21 @@ class JupyterUriProviderAdaptor extends Disposables implements IJupyterUriProvid
token.dispose();
}
}
async handleQuickPick(item: QuickPickItem, _backEnabled: boolean): Promise<string | undefined> {
async handleQuickPick(
item: QuickPickItem & { command?: JupyterServerCommand },
_backEnabled: boolean
): Promise<string | undefined> {
if (!this.provider.commandProvider) {
throw new Error(`No Jupyter Server Command Provider for ${this.provider.extensionId}#${this.provider.id}`);
}
const token = new CancellationTokenSource();
try {
const items = await this.provider.commandProvider.getCommands('', token.token);
const command = items.find((c) => c.title === item.label) || this.commands.get(item.label);
let command: JupyterServerCommand | undefined =
'command' in item ? (item.command as JupyterServerCommand) : undefined;
if (!command) {
const items = await this.provider.commandProvider.getCommands('', token.token);
command = items.find((c) => c.title === item.label) || this.commands.get(item.label);
}
if (!command) {
throw new Error(
`Jupyter Server Command ${item.label} not found in Command Provider ${this.provider.extensionId}#${this.provider.id}`
Expand Down
13 changes: 7 additions & 6 deletions src/kernels/jupyter/connection/jupyterUriProviderRegistration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from '../types';
import { sendTelemetryEvent } from '../../../telemetry';
import { traceError, traceVerbose } from '../../../platform/logging';
import { IJupyterServerUri, IJupyterUriProvider } from '../../../api';
import { IJupyterServerUri, IJupyterUriProvider, JupyterServerCommand } from '../../../api';
import { Disposables } from '../../../platform/common/utils';
import { IServiceContainer } from '../../../platform/ioc/types';
import { IExtensionSyncActivationService } from '../../../platform/activation/types';
Expand Down Expand Up @@ -252,14 +252,15 @@ class JupyterUriProviderWrapper extends Disposables implements IInternalJupyterU
};
});
}
public async handleQuickPick(item: QuickPickItem, back: boolean): Promise<string | 'back' | undefined> {
public async handleQuickPick(
item: QuickPickItem & { original: QuickPickItem & { command?: JupyterServerCommand } },
back: boolean
): Promise<string | 'back' | undefined> {
if (!this.provider.handleQuickPick) {
return;
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
if ((item as any).original) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return this.provider.handleQuickPick((item as any).original, back);
if ('original' in item && item.original) {
return this.provider.handleQuickPick(item.original, back);
}
return this.provider.handleQuickPick(item, back);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { IKernelFinder, KernelConnectionMetadata, RemoteKernelConnectionMetadata
import { IApplicationShell } from '../../../platform/common/application/types';
import { InteractiveWindowView, JVSC_EXTENSION_ID, JupyterNotebookView } from '../../../platform/common/constants';
import { disposeAllDisposables } from '../../../platform/common/helpers';
import { IDisposable, IFeaturesManager } from '../../../platform/common/types';
import { IDisposable } from '../../../platform/common/types';
import { Common, DataScience } from '../../../platform/common/utils/localize';
import {
IMultiStepInput,
Expand Down Expand Up @@ -80,8 +80,6 @@ export class RemoteNotebookKernelSourceSelector implements IRemoteNotebookKernel
@inject(JupyterServerSelector) private readonly serverSelector: JupyterServerSelector,
@inject(JupyterConnection) private readonly jupyterConnection: JupyterConnection,
@inject(IConnectionDisplayDataProvider) private readonly displayDataProvider: IConnectionDisplayDataProvider,
@inject(IFeaturesManager)
private readonly features: IFeaturesManager,
@inject(IRemoteKernelFinderController)
private readonly kernelFinderController: IRemoteKernelFinderController,
@inject(IJupyterUriProviderRegistration)
Expand Down Expand Up @@ -144,42 +142,52 @@ export class RemoteNotebookKernelSourceSelector implements IRemoteNotebookKernel
| QuickPickItem
)[] = [];

const displayLastUsedTime = !this.features.features.enableProposedJupyterServerProviderApi;
const serversAndTimes: { server: IRemoteKernelFinder; time?: number }[] = [];
await Promise.all(
servers
.filter((s) => s.serverProviderHandle.id === provider.id)
.map(async (server) => {
// remote server
const lastUsedTime = displayLastUsedTime
? (await this.serverUriStorage.getAll()).find(
(item) =>
generateIdFromRemoteProvider(item.provider) ===
generateIdFromRemoteProvider(server.serverProviderHandle)
)
: undefined;
if ((token.isCancellationRequested || !lastUsedTime) && displayLastUsedTime) {
const lastUsedTime = (await this.serverUriStorage.getAll()).find(
(item) =>
generateIdFromRemoteProvider(item.provider) ===
generateIdFromRemoteProvider(server.serverProviderHandle)
);
if (token.isCancellationRequested) {
return;
}
quickPickServerItems.push({
type: KernelFinderEntityQuickPickType.KernelFinder,
kernelFinderInfo: server,
idAndHandle: server.serverProviderHandle,
label: server.displayName,
detail:
displayLastUsedTime && lastUsedTime?.time
? DataScience.jupyterSelectURIMRUDetail(new Date(lastUsedTime.time))
: undefined,
buttons: provider.removeHandle
? [
{
iconPath: new ThemeIcon('trash'),
tooltip: DataScience.removeRemoteJupyterServerEntryInQuickPick
}
]
: []
});
serversAndTimes.push({ server, time: lastUsedTime?.time });
})
);
serversAndTimes.sort((a, b) => {
if (!a.time && !b.time) {
return a.server.displayName.localeCompare(b.server.displayName);
}
if (!a.time && b.time) {
return 1;
}
if (a.time && !b.time) {
return -1;
}
return (a.time || 0) > (b.time || 0) ? -1 : 1;
});
serversAndTimes.forEach(({ server, time }) => {
quickPickServerItems.push({
type: KernelFinderEntityQuickPickType.KernelFinder,
kernelFinderInfo: server,
idAndHandle: server.serverProviderHandle,
label: server.displayName,
detail: time ? DataScience.jupyterSelectURIMRUDetail(new Date(time)) : undefined,
buttons: provider.removeHandle
? [
{
iconPath: new ThemeIcon('close'),
tooltip: DataScience.removeRemoteJupyterServerEntryInQuickPick
}
]
: []
});
});

if (provider.getQuickPickEntryItems && provider.handleQuickPick) {
if (quickPickServerItems.length > 0) {
Expand Down
11 changes: 2 additions & 9 deletions src/platform/common/featureManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ const deprecatedFeatures: DeprecatedFeatureInfo[] = [
export class FeatureManager implements IFeaturesManager {
private _onDidChangeFeatures = new Emitter<void>();
readonly onDidChangeFeatures = this._onDidChangeFeatures.event;
private _features: IFeatureSet = {
enableProposedJupyterServerProviderApi: false
};
private _features: IFeatureSet = {};
get features(): IFeatureSet {
return this._features;
}
Expand All @@ -74,12 +72,7 @@ export class FeatureManager implements IFeaturesManager {
}

private _updateFeatures() {
const enabled = this.workspace
.getConfiguration('jupyter')
.get<boolean>('enableProposedJupyterServerProviderApi', false);
this.features = {
enableProposedJupyterServerProviderApi: enabled === true
};
this.features = {};
}

public dispose() {
Expand Down
5 changes: 1 addition & 4 deletions src/platform/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export interface IJupyterSettings {
readonly experiments: IExperiments;
readonly logging: ILoggingSettings;
readonly allowUnauthorizedRemoteConnection: boolean;
readonly enableProposedJupyterServerProviderApi?: boolean;
readonly jupyterInterruptTimeout: number;
readonly jupyterLaunchTimeout: number;
readonly jupyterLaunchRetries: number;
Expand Down Expand Up @@ -258,9 +257,7 @@ export type DeprecatedFeatureInfo = {
setting?: DeprecatedSettingAndValue;
};

export interface IFeatureSet {
enableProposedJupyterServerProviderApi: boolean;
}
export interface IFeatureSet {}

export const IFeaturesManager = Symbol('IFeaturesManager');

Expand Down
108 changes: 99 additions & 9 deletions src/standalone/userJupyterServer/userServerUriProvider.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,9 @@ import { disposeAllDisposables } from '../../platform/common/helpers';
import { JVSC_EXTENSION_ID, Settings, UserJupyterServerPickerProviderId } from '../../platform/common/constants';
import { assert } from 'chai';
import { generateIdFromRemoteProvider } from '../../kernels/jupyter/jupyterUtils';
import { Common, DataScience } from '../../platform/common/utils/localize';
import { IJupyterPasswordConnectInfo, JupyterPasswordConnect } from './jupyterPasswordConnect';
import { IFileSystem } from '../../platform/common/platform/types';
import { JupyterServerCollection } from '../../api';
import { IJupyterServerUri, JupyterServerCollection } from '../../api';

/* eslint-disable @typescript-eslint/no-explicit-any, , */
suite('User Uri Provider', () => {
Expand Down Expand Up @@ -507,13 +506,6 @@ suite('User Uri Provider', () => {
if (server instanceof InputFlowAction || server === 'back') {
throw new Error('Server not returned');
}
verify(
applicationShell.showWarningMessage(
DataScience.insecureSessionMessage,
Common.bannerLabelYes,
Common.bannerLabelNo
)
).never();
assert.isFalse(secureConnectionStub.called);
const servers = await provider.getJupyterServers(token);
assert.isAtLeast(servers.length, 3, '2 migrated urls and one entered');
Expand All @@ -529,4 +521,102 @@ suite('User Uri Provider', () => {
assert.strictEqual(serversInNewStorage.length, 3);
assert.strictEqual(serversInNewStorage2.length, 3);
});
test('When pre-populating with https url and without password auth, the next step should be the displayName & back from displayName should get out of this UI flow (without displaying the Url picker)', async () => {
await testMigration();
getPasswordConnectionInfoStub.restore();
getPasswordConnectionInfoStub.reset();
const urlInputStub = sinon.stub(UserJupyterServerUriInput.prototype, 'getUrlFromUser');
urlInputStub.resolves();
sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: false });
const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections');
secureConnectionStub.resolves(false);
const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName');
displayNameStub.rejects(InputFlowAction.back);

const [cmd] = await provider.getCommands('https://localhost:3333', token);
const server = await provider.handleCommand(cmd, token);

assert.strictEqual(server, 'back');
assert.strictEqual(displayNameStub.callCount, 1);
assert.strictEqual(urlInputStub.callCount, 0); // Since a url was provided we should never prompt for this, even when clicking back in display name.
});
test('When pre-populating with https url and without password auth, the next step should be the displayName & cancel from displayName should get out of this UI flow (without displaying the Url picker)', async () => {
await testMigration();
getPasswordConnectionInfoStub.restore();
getPasswordConnectionInfoStub.reset();
const urlInputStub = sinon.stub(UserJupyterServerUriInput.prototype, 'getUrlFromUser');
urlInputStub.resolves();
sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: false });
const secureConnectionStub = sinon.stub(SecureConnectionValidator.prototype, 'promptToUseInsecureConnections');
secureConnectionStub.resolves(false);
const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName');
displayNameStub.rejects(InputFlowAction.cancel);

const [cmd] = await provider.getCommands('https://localhost:3333', token);
const server = await provider.handleCommand(cmd, token);

assert.isUndefined(server);
assert.strictEqual(displayNameStub.callCount, 1);
assert.strictEqual(urlInputStub.callCount, 0); // Since a url was provided we should never prompt for this, even when clicking back in display name.
});
test('When pre-populating with https url and without password auth, and the server is invalid the next step should be the url display', async () => {
await testMigration();
getPasswordConnectionInfoStub.restore();
getPasswordConnectionInfoStub.reset();
const urlInputStub = sinon.stub(UserJupyterServerUriInput.prototype, 'getUrlFromUser');
let getUrlFromUserCallCount = 0;
urlInputStub.callsFake(async (_initialValue, errorMessage, _) => {
switch (getUrlFromUserCallCount++) {
case 0: {
// Originally we should be called with an error message.
assert.isOk(errorMessage, 'Error Message should not be empty');
return {
jupyterServerUri: {
baseUrl: 'https://localhost:9999/',
displayName: 'ABCD',
token: ''
},
url: 'https://localhost:9999/?token=ABCD'
};
}
case 1: {
// There should be no error message displayed,
// We should have come here from the back button of the display name.
assert.isEmpty(errorMessage, 'Error Message should be empty');
// Lets get out of here.
throw InputFlowAction.back;
}
default:
throw new Error('Method should not be called again');
}
});
sinon.stub(JupyterPasswordConnect.prototype, 'getPasswordConnectionInfo').resolves({ requiresPassword: false });
const displayNameStub = sinon.stub(UserJupyterServerDisplayName.prototype, 'getDisplayName');
displayNameStub.rejects(InputFlowAction.back);
when(jupyterConnection.validateRemoteUri(anything(), anything(), true)).thenCall(
(_, uri: IJupyterServerUri) => {
if (!uri) {
return;
}
if (uri.baseUrl.startsWith('https://localhost:9999')) {
return;
}
throw new Error('Remote Connection Failure');
}
);

// Steps
// 1. First provide a url https://localhost:3333 to a server that is non-existent
// 2. Verify the error message is displayed and user is prompted to enter the Url again.
// 3. Next verify the user is prompted for a display name
// 4. When we click back button on display name ui, ensure we go back to Url ui.
// 5. Hitting back button on Url ui should exit out completely

const [cmd] = await provider.getCommands('https://localhost:3333', token);
const server = await provider.handleCommand(cmd, token);

assert.strictEqual(server, 'back');
assert.strictEqual(displayNameStub.callCount, 1);
assert.strictEqual(urlInputStub.callCount, 2); // Displayed twice, first time for error message, second time when hit back button from display UI.
});
});
Loading
Loading