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

[Curation] Add promoted/hidden documents section & logic + Restore defaults button #94769

Merged
merged 9 commits into from
Mar 18, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,17 @@ export const RESULT_ACTIONS_DIRECTIONS = i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.curations.resultActionsDescription',
{ defaultMessage: 'Promote results by clicking the star, hide them by clicking the eye.' }
);
export const HIDE_DOCUMENT_ACTION = {
title: i18n.translate('xpack.enterpriseSearch.appSearch.engine.curations.hideButtonLabel', {
defaultMessage: 'Hide this result',
}),
iconType: 'eyeClosed',
iconColor: 'danger',
};
export const SHOW_DOCUMENT_ACTION = {
title: i18n.translate('xpack.enterpriseSearch.appSearch.engine.curations.showButtonLabel', {
defaultMessage: 'Show this result',
}),
iconType: 'eye',
iconColor: 'primary',
};
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { Loading } from '../../../../shared/loading';
import { MANAGE_CURATION_TITLE } from '../constants';

import { CurationLogic } from './curation_logic';
import { OrganicDocuments } from './documents';
import { OrganicDocuments, HiddenDocuments } from './documents';
import { ActiveQuerySelect, ManageQueriesModal } from './queries';

interface Props {
Expand Down Expand Up @@ -61,7 +61,8 @@ export const Curation: React.FC<Props> = ({ curationsBreadcrumb }) => {

{/* TODO: PromotedDocuments section */}
<OrganicDocuments />
{/* TODO: HiddenDocuments section */}
<EuiSpacer />
<HiddenDocuments />

{/* TODO: AddResult flyout */}
</>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { setMockValues, setMockActions } from '../../../../../__mocks__';

import React from 'react';

import { shallow } from 'enzyme';

import { EuiEmptyPrompt, EuiButtonEmpty } from '@elastic/eui';

import { DataPanel } from '../../../data_panel';
import { CurationResult } from '../results';

import { HiddenDocuments } from './';

describe('HiddenDocuments', () => {
const values = {
curation: {
hidden: [
{ id: 'mock-document-1' },
{ id: 'mock-document-2' },
{ id: 'mock-document-3' },
{ id: 'mock-document-4' },
{ id: 'mock-document-5' },
],
},
hiddenDocumentsLoading: false,
};
const actions = {
removeHiddenId: jest.fn(),
clearHiddenIds: jest.fn(),
};

beforeEach(() => {
jest.clearAllMocks();
setMockValues(values);
setMockActions(actions);
});

it('renders a list of hidden documents', () => {
const wrapper = shallow(<HiddenDocuments />);

expect(wrapper.find(CurationResult)).toHaveLength(5);
});

it('renders an empty state & hides the panel actions when empty', () => {
setMockValues({ ...values, curation: { hidden: [] } });
const wrapper = shallow(<HiddenDocuments />);

expect(wrapper.find(EuiEmptyPrompt)).toHaveLength(1);
expect(wrapper.find(DataPanel).prop('action')).toBe(false);
});

it('renders a loading state', () => {
setMockValues({ ...values, hiddenDocumentsLoading: true });
const wrapper = shallow(<HiddenDocuments />);

expect(wrapper.find(DataPanel).prop('isLoading')).toEqual(true);
});

describe('actions', () => {
it('renders results with an action button that un-hides the result', () => {
const wrapper = shallow(<HiddenDocuments />);
const result = wrapper.find(CurationResult).last();
result.prop('actions')[0].onClick();

expect(actions.removeHiddenId).toHaveBeenCalledWith('mock-document-5');
});

it('renders a restore all button that un-hides all hidden results', () => {
const wrapper = shallow(<HiddenDocuments />);
const panelActions = shallow(wrapper.find(DataPanel).prop('action') as React.ReactElement);

panelActions.find(EuiButtonEmpty).simulate('click');
expect(actions.clearHiddenIds).toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';

import { useValues, useActions } from 'kea';

import { EuiButtonEmpty, EuiFlexGroup, EuiFlexItem, EuiEmptyPrompt } from '@elastic/eui';
import { i18n } from '@kbn/i18n';

import { DataPanel } from '../../../data_panel';

import { SHOW_DOCUMENT_ACTION } from '../../constants';
import { CurationLogic } from '../curation_logic';
import { AddResultButton, CurationResult, convertToResultFormat } from '../results';

export const HiddenDocuments: React.FC = () => {
const { clearHiddenIds, removeHiddenId } = useActions(CurationLogic);
const { curation, hiddenDocumentsLoading } = useValues(CurationLogic);

const documents = curation.hidden;
const hasDocuments = documents.length > 0;

return (
<DataPanel
filled
iconType="eyeClosed"
title={
<h2>
{i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.curations.hiddenDocuments.title',
{ defaultMessage: 'Hidden documents' }
)}
</h2>
}
subtitle={i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.curations.hiddenDocuments.description',
{ defaultMessage: 'Hidden documents will not appear in organic results.' }
)}
action={
hasDocuments && (
<EuiFlexGroup gutterSize="s">
<EuiFlexItem>
<AddResultButton />
</EuiFlexItem>
<EuiFlexItem>
<EuiButtonEmpty onClick={clearHiddenIds} iconType="menuUp" size="s">
{i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.curations.hiddenDocuments.removeAllButtonLabel',
{ defaultMessage: 'Restore all' }
)}
</EuiButtonEmpty>
</EuiFlexItem>
</EuiFlexGroup>
)
}
isLoading={hiddenDocumentsLoading}
>
{hasDocuments ? (
documents.map((document) => (
<CurationResult
key={document.id}
result={convertToResultFormat(document)}
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth writing as assertion in your tests that confirm that the result is converted before passed to CurationResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, no - that's the point of unit tests and having split convertToResultFormat to its own file/set of tests - it gets tested separately and doesn't muddy this test (and is also much easier to remove if/when we no longer need the helper).

And of course in an ideal world we would have E2E tests as well which would crash with Javascript type errors if the <Result> component wasn't getting the correct data structure it expects, but for now we can QA that manually 😄

Copy link
Member

@JasonStoltz JasonStoltz Mar 18, 2021

Choose a reason for hiding this comment

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

I know when we get into testing philosophy the waters can get a bit muddy. Some counter points:

A key part of unit testing is being able to refactor a unit with confidence that it is still working as expected. A key thing that this unit does is format a result and pass it to CurationResult. As it is, you could lose that key behavior in a refactor, right? Which means you cannot refactor with confidence.

If unit tests are supposed to be living documentation, isn't it important to document this behavior?

IMO, no - that's the point of unit tests and having split convertToResultFormat to its own file/set of tests - it gets tested separately and doesn't muddy this test

IMO, that's only half the equation. You test the behavior of convertToResultFormat separately, and here, you would mock it and confirm that it is called. It's the same concept with shallow rendering, right? You mock the components that you are rendering, since they are tested elsewhere, but you still assert that they have been called / rendered.

If this rendered a wrapper component that performed convertToResultFormat automatically on Result, like <CurationResultFromDocument result={document} />, then I would say you might not have to assert anything. You would almost certainly write an assertion on the CurationResultFromDocument test to confirm that it calls convertToResultFormat and formats the result before passing it to CurationResult. You're losing that here, right? As it is, you're basically composing these two functions together ... CurationResult and convertToResultFormat to get a certain effect, but you're not actually testing that that effect is happening.

I am just thinking out loud. I don't expect you to make any changes here. I just wanted to play devil's advocate a bit since we've already opened this can of worms.

Copy link
Contributor Author

@cee-chen cee-chen Mar 18, 2021

Choose a reason for hiding this comment

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

There's a lot going on here, so I'm going to split up my responses separately to try to help with readability.

As it is, you're basically composing these two functions together ... CurationResult and convertToResultFormat to get a certain effect, but you're not actually testing that that effect is happening.

That's correct! And that's how unit testing should work. Testing that components work together is the point of integration, E2E testing, or manual testing.

Let me give you an example (that I've actually run into in the past with previous developers and have had to do my best to try and dissuade people from doing). Let's say I'm doing this instead:

import { invert } from lodash;

<CurationResult result={invert(document)} />

So basically, I'm using a 3rd party fn instead of my 1st party convertToResultFormat fn.

In my unit test, do you think it's worth the time of either the developer writing the tests or reading the tests to get the result prop and check that each key/value pair has correctly been inverted? Or how about cloneDeep as an example instead - should I iterate into each nested object to check for mutation? That seems like a pretty big waste of time, right?

So the answer is no, because that is not the responsibility of my unit. We have to trust that the lodash author(s) correctly wrote their helper functions and have their own set of unit tests that covers the invert or cloneDeep fns. We can reaffirm that the library is integrating as expected with out code via integration/E2E tests, but we absolutely do not want to get into the business of unit testing other people's code for them.

The same principle applies to convertToResultFormat - the only difference is that that helper/fn belongs to us instead of some 3rd party. However, if we have correctly written separate unit tests for our helper, we can now refer to those unit tests and trust that convertToResultFormat will give us the format we need (by reading the tests and confirming the returned response). We then verify the connection/integration with manual or E2E tests.

I hope that makes sense - let me know if not. I'll address some of your other comments (refactoring with confidence, shallow rendering) separately in a follow up comment here.

Copy link
Contributor Author

@cee-chen cee-chen Mar 18, 2021

Choose a reason for hiding this comment

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

On refactoring with confidence:

A key part of unit testing is being able to refactor a unit with confidence that it is still working as expected. A key thing that this unit does is format a result and pass it to CurationResult. As it is, you could lose that key behavior in a refactor, right? Which means you cannot refactor with confidence.

Absolutely I could. Let's say I'm refactoring convertToResultFormat itself. Let's say I switched my .forEach to a plain for loop because I'm feeling old school. All the unit tests within utils.test.ts should still pass (if I haven't messed up the indexes in my for loop because I haven't written one in a million years 😅 ). Refactoring the unit should be easily assertable by the unit test.

I'm not 100% sure I'm reading this correctly here, but if the refactor you're referring to is the upcoming one where we fix our data structure and remove the need for convertToResultFormat entirely, then I'd say that's a bit of a false equivalency. Removing this utility (the eventual goal) is not a refactor, it's straight up an intentional behavior change.

Even so, we could make that change with near 100% confidence still. All it takes is deleting the entire helper file, removing the convertToResultFormat(document) and just passing in result={document}. If we made a mistake along the way:

  1. Typescript would pick that up - it would register that document is not the correct type that result expects
  2. E2E tests would pick up that up via reported console errors, and as I've mentioned before, are better tests for catching bugs/regressions in the first place

I start to worry that because we're (currently) only writing unit tests, we're relying too much on them & not seeing the bigger picture of letting more responsibility lie with a full and robust set of tests & tools (unit, integration, E2E, visual diffs, typescript/linters, etc.). I think we should only feel fully confident once we have a wider variety of tests, and that's why I've deliberately booked time for us post-MVP to write E2E tests - it's not a nice to have, it's essential, and it will absolutely save us time on unit tests attempting to be integration tests.

Copy link
Member

@JasonStoltz JasonStoltz Mar 18, 2021

Choose a reason for hiding this comment

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

it will absolutely save us time on unit tests attempting to be integration tests

I don't think this is a test trying to be an integration test, I think this is just a test trying to assert that the unit under test is functioning normally.

Let me ask you this ... if instead of HiddenDocument, you were writing this component:

const CurationResultFromDocument: React.FC = ({ document }) => {
  <CurationResult={convertToResultFormat(document)} />
}

How would you test this?

Would you simply write the following and call it a day?

expect(wrapper.find(CurationResult).exists()).toBe(true)

I'm guessing not, I'm guessing you'd probably write this:

const formattedResult = {};
jest.mock('convertToResultFormat').mockReturn(formattedResult)

expect(wrapper.find(CurationResult).prop('result')).toEqual(formattedResult);

Copy link
Member

Choose a reason for hiding this comment

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

Like if you just wrote the prior, you'd be glossing over a key behavior of this unit.

Copy link
Contributor Author

@cee-chen cee-chen Mar 18, 2021

Choose a reason for hiding this comment

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

On deciding what to assert on:

If unit tests are supposed to be living documentation, isn't it important to document this behavior? [...] You test the behavior of convertToResultFormat separately, and here, you would mock it and confirm that it is called.

I LOVE this question because I don't have a super good answer for this. Absolutely I could have added an
expect(convertToResultFormat).toHaveBeenCalledWith(someDocument);
assertion (and I'd definitely still be down for it, if y'all think it helps understanding).

Here's the key though, right, so I'm writing these unit tests to help others understand the code - the design of it and the intention of it. And I'm writing it through the lens of my understanding of the code. And React components are particularly difficult, 1. because anytime you're attempting to test UI it's a clusterfuck, and 2. because they're not really true "units", and you kind of have to go in manually and discern the different functionalities and purposes of various components and onClicks and so on.

So in this particular case when I was writing tests for the <HiddenDocuments> component, I was like - okay, what's the main purpose of this UI? And I decided it was 1. showing a list of hidden results, and 2. providing the ability to un-hide results. So I focused on that, and then from there I used code coverage reports to cover any remaining lines.

While I was doing that, I didn't think much about the use of convertToResultFormat here, I think for a few reasons:

  1. The big one was that it didn't strike me as a primary purpose of the view. Yes, it's required to get the component working in the front-end, but in a perfect world it wouldn't have to be there (a la the <OrganicDocuments> view).
  2. I think almost because of that, I left out an assertion on it on purpose, because once the data structure is fixed on the server, we can just remove it and not have to subsequently spend time updating unit tests (i.e., falling into the trap of writing too many fragile/inconsequential assertions that makes updating code a chore).
  3. I also thought the importance of its requirement/existence was sufficiently documented in the utility file itself, and that it would be easy enough for developers to follow the definition to find more info & context without duplicating documentation/testing in multiple places.

That being said, maybe my interpretation was wrong and it's useful for other developers to see in the unit test assertions that the function is even there.

And honestly that's one of the joys and difficulties of writing unit tests for me - is that because we're writing it with other people in mind and not computers, it's has a decent amount of room for subjectivity. I don't have a silver bullet answer for this, and I definitely think that's what the code review process is for, but I do have a couple general paths I personally tend to follow that I'd love to get thoughts on:

  • To continue riffing on this specific example - If the purpose of the file or unit is to call a helper at the end, then absolutely I'll assert that the helper got called.
  • But if it's not the main purpose, then it depends on the complexity of the file I'm testing. If there's a lot going on, I don't think it makes sense to assert on every possible little thing - you just want the primary intention of the code, right? Because that makes it easier for others to read and understand, rather than trying to parse through 5+ assertions to determine which was the meaningful one.
  • Generally if it's a busy or complex file or component I also tend to skip things that get passed as-is without any extra branching or logic. So like in this case, convertToResultFormat() is being called without any conditionals or any extra logic around what's being passed to it. So in the same way I wouldn't necessarily check that a cloneDeep got called in the middle of a busy function - I assume it works and I prefer to assert on the beginning/end.
  • However, this is definitely a double-edged sword and why it's so important to split up code modularly. The longer & busier it gets, the harder it becomes to write truly atomic unit tests & assert on things that matter. It's definitely really hard to do this for React views though, which transitions me perfectly to my segment:

On unit testing React components/views:

As I've alluded to earlier, unit testing React views is tricky. I'm definitely not confident my approach is 100% the right approach either to be clear, I've kind of settled on this while trying to strike a sane and readable balance to myself/my future self/other developers. I'm definitely open to talking/diving more into how we approach component testing as a team and discussing potential changes (e.g. jest.snapshot instead of manual assertions, etc.)

It's the same concept with shallow rendering, right? You mock the components that you are rendering, since they are tested elsewhere, but you still assert that they have been called / rendered.

This is actually a really great analogy and plays into what I'm saying above, because wait! We don't always. Take the <FlashMessages /> component in the Curation view:

It's there - did I assert on it? No, and we don't assert on it in almost any other view either. Because it's incidental to this view, it's not the primary purpose of it, and it doesn't have any complex logic around it - it's a very basic pass-through that handles its own logic & has its own unit tests, and that is typically better tested for in this view via non-unit tests (either E2E testing or visual diff testing for example).

We definitely should assert on views that are either conditional (to cover logic branches) or crucial to the view (for example, if the view is a flyout, I'd definitely check that an EuiFlyout is being rendered) - but it's the same principle I mentioned above, where it's absolutely possible for there to be too much noise to be helpful to others, and as such we're constantly making subjective decisions about what's important to highlight in our unit test "documentation" and what's not.

I hope that makes more sense now as to my reasoning and I'm definitely open to adding an assertion if you think it helps your understanding - because that's definitely the goal of unit tests for me.

And that's my last set of comments!! Thanks for sticking with me, I had a lot of fun writing this out and I hope you similarly enjoyed (or at least didn't hate) reading this!

Copy link
Member

Choose a reason for hiding this comment

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

Your answer is on-point. 100% agree. Thank you for writing this out. I think of the things the same way, and I think my original question was just wondering out loud if this fell into the category of "things that are crucial to the view".

However, this is definitely a double-edged sword and why it's so important to split up code modularly. The longer & busier it gets, the harder it becomes to write truly atomic unit tests & assert on things that matter.

Yes, I agree!

Anyway, just leave this as is, now that we've talked about it I'd agree that it's not necessarily crucial to the view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shit sorry Jason, I swear I like hard refreshed the page and GitHub wasn't showing me any new responses until like a second ago 😱 I super did not mean to just carry on like I was ignoring your responses lol, very sorry 🙇‍♀️

You misunderstand me. I'm saying you should do something like this (pseudo-code):

Ahhhh!! Yeah!!! Now I look like either an idiot or a genius with my 3rd comment (probably more idiot). Totally on the same page as you now, apologies for taking 2 overly-long comments to get there lol. 🤦‍♀️

I had a lot of fun writing too much and appreciate you giving me the chance to do so! Love having discussions like these on subjective shenanigans and syncing up on them. Thanks a million!

actions={[
{
...SHOW_DOCUMENT_ACTION,
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like how you group these together in a constant and then spread them here 💯

onClick: () => removeHiddenId(document.id),
},
]}
/>
))
) : (
<EuiEmptyPrompt
titleSize="s"
title={
<h3>
{i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.curations.hiddenDocuments.emptyTitle',
{ defaultMessage: 'No documents are being hidden for this query' }
)}
</h3>
}
body={i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.curations.hiddenDocuments.emptyDescription',
{
defaultMessage:
'Hide documents by clicking the eye icon on the organic results above, or search and hide a result manually.',
}
)}
actions={<AddResultButton />}
/>
)}
</DataPanel>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
*/

export { OrganicDocuments } from './organic_documents';
export { HiddenDocuments } from './hidden_documents';