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

[cee-chen]

Summary

I strongly recommend following along by commit! I was originally going to have f2b6446 as a separate / standalone PR but then I realized it made sense to include in this PR for as part of QA. I can still separate it out if folks prefer.

Screencaps

Promoting & demoting documents

Hiding & showing documents

Restore defaults

QA

• Create/Go to a curation page
• Promoting
  • Confirm that promoting documents from the organic documents list works as expected
  • Confirm that demoting documents from the promoted documents list works as expected
  • Confirm that dragging & dropping promoted documents to reorder works as expected
  • Confirm that the "Demote all" button works as expected
• Hiding
  • Confirm that hiding documents from the organic documents list works as expected
  • Confirm that showing documents from the hidden documents list works as expected
  • Confirm that the "Restore all" button works as expected
• Restore defaults
  • Promote & hide any number of documents
  • Confirm that the "Restore defaults" button clears all promoted & hidden documents

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

NOTE: Some axe failures are being generated by react-beautiful-dnd (e.g. div[data-rbd-drag-handle-draggable-id="park_yosemite"], but this is either an axe false positive (since the corresponding ID does appear to exist), or likely out of scope for our team and should be addressed at the EUI/library level.

[cee-chen]

Jest failures aren't part of our plugin - odd

[cee-chen]

@elasticmachine merge upstream

[cee-chen]

Definitely did not open this PR and then walk away yesterday & totally forget to leave comments

[cee-chen]

We use the Restore defaults button in 3-4 different views in App Search, so I'm hoping to pull this out to a shared i18n constants file soon (alongside other reusable actions such as Edit, Save, etc.)

[cee-chen]

Note - the actual listener assertion is being done in an afterEach block on line 363.

[JasonStoltz]

Ohhhh. That's super clever.

[cee-chen]

I can't take the credit haha, I think our rspec tests also do this a lot pattern-wise in our server-side code :)

[cee-chen]

Extra context here: promotedIds has a straight setX action whereas hiddenIds does not. This is because promoted IDs needs to support re-ordering by EUI's drag/drop components, and the hidden ID list does not.

[cee-chen]

UX context for this: I figured hitting a reset button should provide a larger and more impactful loading screen since it's an important enough action to require a confirmation prompt.

[cee-chen]

So, I want to provide some context - I don't love this and find it somewhat repetitive, but I also don't super see an easy Kea way of adding a side effect that's equivalent to "if promotedIDs or hiddenIds changes, call actions.updateCuration (XHR call)".

Do y'all have any other ideas? Definitely open to them if so!

[JasonStoltz]

Nothing off of the top of my head

[cee-chen]

This is mostly from https://elastic.github.io/eui/#/display/drag-and-drop#custom-drag-handle

[cee-chen]

This will open the add result flyout in the next PR.

[cee-chen]

Just want to highlight this whole file 😬

This was the original issue I discussed in our sync a week or two ago where the documents coming back for the promoted and hidden lists were not in the standard format that our Result component wants (the organic list was in the correct format however).

This is in a separate utils.ts file from the top-level one because I'm hoping we can fix this at the server-level someday and then just delete this entire file / helpers entirely.

[cee-chen]

Also want to highlight this, non-meta engines don't need to show an engine in the Result card UI, so technically we don't need to try and get that data. If this ever changes we can work around it, but for now I opted to just cast the type 'incorrectly'.

[JasonStoltz]

I'm fairly certain you'll hate this, but thought it might be a cool way to dry up these tests.

              
              const itShouldChangePromotedIdsState = (from, to, callback) => {
                it('should set promotedIds state & promotedDocumentsLoading to true', () => {
                   mount({ promotedIds: from });

                   callback();

                   expect(CurationLogic.values).toEqual({
                     ...DEFAULT_VALUES,
                     promotedIds: to,
                     promotedDocumentsLoading: true,
                   });
                 });
               }

               describe('addPromotedId', () => {
                 itShouldChangePromotedIdsState(['hello'], ['hello', 'world'], () => {
                   CurationLogic.actions.addPromotedId('world');
                 });
               }

               describe('setPromotedIds', () => {
                 itShouldChangePromotedIdsState([], ['hello', 'world'], () => {
                   CurationLogic.actions.addPromotedId(['hello', 'world']);
                 });
               }

               describe('removePromotedId', () => {
                 itShouldChangePromotedIdsState(['hello', 'deleteme', 'world'], ['hello', 'world'], () => {
                   CurationLogic.actions.removePromotedId('deleteme');
                 });
               }

Or some variant of that

             it('should set promotedIds state & promotedDocumentsLoading to true', () => {
                 expectAction(() => {
                   CurationLogic.actions.removePromotedId('deleteme');
                 }).toChangePromotedIds(
                   ['hello', 'deleteme', 'world'],
                   ['hello', 'world']
                 );
               }

Just riffing now, what if we had a generic "expectAction" we could use in specs:

             it('should set promotedIds state & promotedDocumentsLoading to true', () => {
                 expectAction(() => {
                   CurationLogic.actions.removePromotedId('deleteme');
                 }).toChangeState(
                   {
                     promotedIds: ['hello', 'deleteme', 'world'],
                     promotedDocumentsLoading: false
                   },
                   {
                     promotedIds: ['hello', 'world'],
                     promotedDocumentsLoading: true
                   }
                 );
               }

It would handle mounting with defaults, and then running expectations on values.

[cee-chen]

Your last iteration feels super readable/straightforward to me! I like that a lot and would be super open to changing that wholesale across our app. I'd have maybe one minor change request, which is changing the passed arg to an object and using before: { ... }, after: { ... } keys to make it more evident as to what's happening.

[cee-chen]

Actually, I should clarify - this isn't a change request for the this PR right?

[JasonStoltz]

Nope

[JasonStoltz]

without mutating the original array

Excellent, that's super important in Kea.

[JasonStoltz]

Nice work

[JasonStoltz]

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

[JasonStoltz]

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

[cee-chen]

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 😄

[JasonStoltz]

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.

[cee-chen]

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.

[cee-chen]

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.

[JasonStoltz]

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);

[JasonStoltz]

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

[cee-chen]

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:

kibana/x-pack/plugins/enterprise_search/public/applications/app_search/components/curations/curation/curation.tsx

Line 72 in f4da063

<FlashMessages />

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!

[JasonStoltz]

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.

[cee-chen]

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!

[JasonStoltz]

Oh sweet, that renderProp shortcut is great.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: b0ae5fe
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
enterpriseSearch	1301	1305	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
enterpriseSearch	2.1MB	2.1MB	+10.4KB

History

• 💔 Build #113706 failed b02a63e
• 💔 Build #113543 failed f2b6446

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @JasonStoltz

[cee-chen]

Merging for expedience, but happy to come back to this, particularly if anyone thinks of something for #94769 (comment)!

[kibanamachine]

💚 Backport successful

✅ 7.x / #94883

Successful backport PRs will be merged automatically after passing CI.