Skip to content

Commit

Permalink
Restore last connection date times (#14134)
Browse files Browse the repository at this point in the history
  • Loading branch information
DonJayamanne committed Aug 17, 2023
1 parent 341812a commit 677c27b
Show file tree
Hide file tree
Showing 10 changed files with 202 additions and 87 deletions.
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

0 comments on commit 677c27b

Please sign in to comment.