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

[App Search] Added all Document related routes and logic #83324

Merged
merged 10 commits into from
Nov 17, 2020
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { resetContext } from 'kea';

import { mockHttpValues } from '../../../__mocks__';
jest.mock('../../../shared/http', () => ({
HttpLogic: { values: mockHttpValues },
}));
const { http } = mockHttpValues;

jest.mock('../engine', () => ({
EngineLogic: { values: { engineName: 'engine1' } },
}));

jest.mock('../../../shared/flash_messages', () => ({
setQueuedSuccessMessage: jest.fn(),
flashAPIErrors: jest.fn(),
}));
import { setQueuedSuccessMessage, flashAPIErrors } from '../../../shared/flash_messages';

import { DocumentDetailLogic } from './document_detail_logic';

describe('DocumentDetailLogic', () => {
const DEFAULT_VALUES = {
dataLoading: true,
fields: [],
};

const mount = (defaults?: object) => {
if (!defaults) {
resetContext({});
} else {
resetContext({
defaults: {
enterprise_search: {
app_search: {
document_detail_logic: {
...defaults,
},
},
},
},
});
}
DocumentDetailLogic.mount();
};

beforeEach(() => {
jest.clearAllMocks();
});

describe('actions', () => {
describe('setFields', () => {
it('should set fields to the provided value and dataLoading to false', () => {
const fields = [{ name: 'foo', value: ['foo'], type: 'string' }];

mount({
dataLoading: true,
fields: [],
});

DocumentDetailLogic.actions.setFields(fields);

expect(DocumentDetailLogic.values).toEqual({
...DEFAULT_VALUES,
dataLoading: false,
fields,
});
});
});

describe('getDocumentDetails', () => {
it('will call an API endpoint and then store the result', async () => {
const fields = [{ name: 'name', value: 'python', type: 'string' }];
jest.spyOn(DocumentDetailLogic.actions, 'setFields');
const promise = Promise.resolve({ fields });
http.get.mockReturnValue(promise);

DocumentDetailLogic.actions.getDocumentDetails('1');

expect(http.get).toHaveBeenCalledWith(`/api/app_search/engines/engine1/documents/1`);
await promise;
expect(DocumentDetailLogic.actions.setFields).toHaveBeenCalledWith(fields);
});

it('handles errors', async () => {
mount();
const promise = Promise.reject('An error occurred');
http.get.mockReturnValue(promise);

try {
DocumentDetailLogic.actions.getDocumentDetails('1');
await promise;
} catch {
// Do nothing
}
expect(flashAPIErrors).toHaveBeenCalledWith('An error occurred');
});
});

describe('deleteDocument', () => {
let confirmSpy: any;
let promise: Promise<any>;

beforeEach(() => {
confirmSpy = jest.spyOn(window, 'confirm');
confirmSpy.mockImplementation(jest.fn(() => true));
promise = Promise.resolve({});
http.delete.mockReturnValue(promise);
});

afterEach(() => {
confirmSpy.mockRestore();
});

it('will call an API endpoint and show a success message', async () => {
mount();
DocumentDetailLogic.actions.deleteDocument('1');

expect(http.delete).toHaveBeenCalledWith(`/api/app_search/engines/engine1/documents/1`);
await promise;
expect(setQueuedSuccessMessage).toHaveBeenCalledWith(
'Successfully marked document for deletion. It will be deleted momentarily.'
);
});

it('will do nothing if not confirmed', async () => {
mount();
window.confirm = () => false;

DocumentDetailLogic.actions.deleteDocument('1');

expect(http.delete).not.toHaveBeenCalled();
await promise;
});

it('handles errors', async () => {
mount();
promise = Promise.reject('An error occured');
http.delete.mockReturnValue(promise);

try {
DocumentDetailLogic.actions.deleteDocument('1');
await promise;
} catch {
// Do nothing
}
expect(flashAPIErrors).toHaveBeenCalledWith('An error occured');
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { kea, MakeLogicType } from 'kea';
import { i18n } from '@kbn/i18n';

import { HttpLogic } from '../../../shared/http';
import { EngineLogic } from '../engine';
import { flashAPIErrors, setQueuedSuccessMessage } from '../../../shared/flash_messages';
import { FieldDetails } from './types';

interface DocumentDetailLogicValues {
dataLoading: boolean;
fields: FieldDetails[];
}

interface DocumentDetailLogicActions {
setFields(fields: FieldDetails[]): { fields: FieldDetails[] };
deleteDocument(documentId: string): { documentId: string };
getDocumentDetails(documentId: string): { documentId: string };
}

type DocumentDetailLogicType = MakeLogicType<DocumentDetailLogicValues, DocumentDetailLogicActions>;

const CONFIRM_DELETE = i18n.translate(
'xpack.enterpriseSearch.appSearch.documentDetail.confirmDelete',
{
defaultMessage: 'Are you sure you want to delete this document?',
}
);
const DELETE_SUCCESS = i18n.translate(
Comment on lines +28 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

[Not blocking] No strong preference either way, but any thoughts on moving copy out to a constants.ts file? 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I know we've done that elsewhere, and I did consider it. I guess in this particular case I didn't have a strong reason to move them out so I just left them in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a hunch over time we'll find it easier to quickly read logic files the less there is at the top of them (e.g. imports -> type defs -> straight to logic def), but definitely not a blocker as I mentioned

'xpack.enterpriseSearch.appSearch.documentDetail.deleteSuccess',
{
defaultMessage: 'Successfully marked document for deletion. It will be deleted momentarily.',
}
);

export const DocumentDetailLogic = kea<DocumentDetailLogicType>({
path: ['enterprise_search', 'app_search', 'document_detail_logic'],
actions: () => ({
setFields: (fields) => ({ fields }),
getDocumentDetails: (documentId) => ({ documentId }),
deleteDocument: (documentId) => ({ documentId }),
}),
reducers: () => ({
dataLoading: [
true,
{
setFields: () => false,
},
],
fields: [
[],
{
setFields: (_, { fields }) => fields,
},
],
}),
listeners: ({ actions }) => ({
getDocumentDetails: async ({ documentId }) => {
const { engineName } = EngineLogic.values;

try {
const { http } = HttpLogic.values;
// TODO: Handle 404s
const response = await http.get(
`/api/app_search/engines/${engineName}/documents/${documentId}`
Copy link
Member Author

Choose a reason for hiding this comment

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

Future note to myself. We'll need to handle 404s somehow. It's not handled here.

);
actions.setFields(response.fields);
} catch (e) {
flashAPIErrors(e);
}
},
deleteDocument: async ({ documentId }) => {
const { engineName } = EngineLogic.values;

if (window.confirm(CONFIRM_DELETE)) {
try {
const { http } = HttpLogic.values;
await http.delete(`/api/app_search/engines/${engineName}/documents/${documentId}`);
setQueuedSuccessMessage(DELETE_SUCCESS);
Copy link
Member Author

Choose a reason for hiding this comment

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

This used to use a server generated message, but that is not i18n compliant so I moved the message inline here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo, great call! I actually have a note buried somewhere in my backlog to figure out what to do with i18n and errors/messages coming in from the server 😬 For easy stuff like this I'm a huge +1 to pulling them out like this. For the more complex wildcard stuff, we'll have to ask the Kibana team for guidance I think

// TODO Handle routing after success
Copy link
Member Author

Choose a reason for hiding this comment

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

The old code used a history.push to navigate back to the document list view after a successful delete.

I wonder if that is the way we should continue doing it or not.

Copy link
Contributor

@cee-chen cee-chen Nov 12, 2020

Choose a reason for hiding this comment

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

You can use KibanaLogic.values.navigateToUrl() to replace history.push() - it works correctly with Kibana's SPA history (doesn't rerender top-level app which resets Kea state) and also automatically prefixes our app route for us.

I'll try to write up a cheat sheet/reference list of conversions like that at some point soon for our migration QOL

Copy link
Member Author

Choose a reason for hiding this comment

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

I will re-add this when I have the UI wired up with 404 handling. It will be easier to test.

} catch (e) {
flashAPIErrors(e);
}
}
},
}),
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { resetContext } from 'kea';

import { DocumentsLogic } from './documents_logic';

describe('DocumentsLogic', () => {
const DEFAULT_VALUES = {
isDocumentCreationOpen: false,
};

const mount = (defaults?: object) => {
if (!defaults) {
resetContext({});
} else {
resetContext({
defaults: {
enterprise_search: {
app_search: {
documents_logic: {
...defaults,
},
},
},
},
});
}
DocumentsLogic.mount();
};

describe('actions', () => {
describe('openDocumentCreation', () => {
it('should toggle isDocumentCreationOpen to true', () => {
mount({
isDocumentCreationOpen: false,
});

DocumentsLogic.actions.openDocumentCreation();

expect(DocumentsLogic.values).toEqual({
...DEFAULT_VALUES,
isDocumentCreationOpen: true,
});
});
});

describe('closeDocumentCreation', () => {
it('should toggle isDocumentCreationOpen to false', () => {
mount({
isDocumentCreationOpen: true,
});

DocumentsLogic.actions.closeDocumentCreation();

expect(DocumentsLogic.values).toEqual({
...DEFAULT_VALUES,
isDocumentCreationOpen: false,
});
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { kea, MakeLogicType } from 'kea';

interface DocumentsLogicValues {
isDocumentCreationOpen: boolean;
}

interface DocumentsLogicActions {
closeDocumentCreation(): void;
openDocumentCreation(): void;
}

type DocumentsLogicType = MakeLogicType<DocumentsLogicValues, DocumentsLogicActions>;

export const DocumentsLogic = kea<DocumentsLogicType>({
path: ['enterprise_search', 'app_search', 'documents_logic'],
actions: () => ({
openDocumentCreation: true,
closeDocumentCreation: true,
}),
reducers: () => ({
isDocumentCreationOpen: [
false,
{
openDocumentCreation: () => true,
closeDocumentCreation: () => false,
},
],
}),
});
JasonStoltz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export { DocumentDetailLogic } from './document_detail_logic';
export { DocumentsLogic } from './documents_logic';
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export interface FieldDetails {
name: string;
value: string | string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

[not a change request, just me thinking out loud] Someday in a very distant future it might be nice to clean that up a bit more in a server transform - e.g. by doing isArray(value) ? value : [value] we know for sure we're always dealing w/ arrays

Copy link
Member Author

Choose a reason for hiding this comment

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

💯 Yeah that would be ideal

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a simple fix for this at the Logic level for now. This could be refactored later, but I think it's worth at least this small amount of effort because it will greatly simplify things downstream in the UI: 772e801

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh awesome, love it! I have a very slight preference to storing / testing normalizeFields in a utils file to separate out the array logic from getDocumentDetails test (in theory a unit test is easier to follow if it handles 1 thing at a time), but it's a very slight preference and I'm fine with merging as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted it. We actually need to know if it's an array or not on the front-end.

a40fcb9

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh no worries, I was hoping something like that wouldn't come up, but alas. Some day

type: string;
}
Loading