[Enterprise Search] Added Logic for the Credentials View

[JasonStoltz]

Summary

This PR adds the logic and API endpoints necessary for the Credentials View.

The logic file was ported largely as is from https://github.com/elastic/ent-search/blob/master/app/javascript/app_search/Credentials/CredentialsLogic.ts

I've added a number of comments inline explaining some of the thought behind the changes I made.

The main things that I added were:

• credentials_logic.ts
• new routes in /routes/app_search/credentials.ts

Everything else is supporting code.

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[JasonStoltz]

I added one of these tests to each action. My reasoning is this:

1. As I was testing, it helped ensure for me that I had covered all of the necessary values with tests. It will also serve as a reminder to add test coverage as we make updates as well.
2. Avoid unintentional values changes in actions

[cee-chen]

Follow up to my comment here: #77626 (comment)

I strongly think we should not have extra tests specifically for should not change any other values. It's hard to read/understand, doesn't add anything significantly to line coverage, and bloats the file. That said, it's fully possible to check for this in your existing active tests. Simply do this:

// example assertion for "should remove an engine from the active api token's engine list" test

        expect(CredentialsLogic.values).toEqual({
          ...DEFAULT_VALUES,
          activeApiToken: {
            ...newToken,
            engines: ['anotherEngine'],
          },
        });

Instead of:

kibana/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts

Lines 144 to 147 in 91392ae

	expect(CredentialsLogic.values.activeApiToken).toEqual({
	...newToken,
	engines: ['anotherEngine'],
	});

... and bam, you've proved other values haven't changed at the same time you have asserted your expected values do change, in a fixed 3 lines instead of 9+ 🤷 (also cuts down on test times / time spent mounting extra tests, etc.)

FWIW, we also don't have to check the full expect(SomeLogic.values) for every action/value block, just the first one. Follow up edge case/conditional branch tests can simply check SomeLogic.values.someValue after that since the first test asserts unchanged values for us.

[JasonStoltz]

It gets a little weird when you have a bunch of values that change when you fire an action though, because then you can't just use DEFAULT_VALUES.

        expect(CredentialsLogic.values).toEqual({
          ...DEFAULT_VALUES,
          someOtherThing: 'foo',
          anotherValueThatChanged: 'bar',
          activeApiToken: {
            ...newToken,
            engines: ['anotherEngine'],
          },
        });

You'd have to put in assertions for anotherValueThatChanged and someOtherThing as well, as seen above. At which point it's like, well I have a separate test for anotherValueThatChanged and someOtherThing below, why do I need both if it's being asserted here?

One way to avoid that is to use expect.any to make it clear that that is not what's under test currently, and we don't care what is provided here.

        expect(CredentialsLogic.values).toEqual({
          ...DEFAULT_VALUES,
          someOtherThing: expect.any(string),
          anotherValueThatChanged: expect.any(string),
          activeApiToken: {
            ...newToken,
            engines: ['anotherEngine'],
          },
        });

Which is what I arrived at, and I ultimately thought it would be clearer to just move that into a separate test so it's explicit what is being tested. I may have taken it a step too far though.

I don't have a strong opinion here. I think it IS important to assert that the other values do not change somehow though. The entire updated state (the values) are returned from every action and hence part of the public interface of that action and should be asserted. If you prefer the inline assertion on the first test, I find that acceptable.

I wonder if something like this would work....

const values = {
   ...DEFAULT_VALUES,
   someOtherThing: expect.any(string),
   anotherValueThatChanged: expect.any(string),
   activeApiToken: expect.any(object)
}

....          
        expect(CredentialsLogic.values).toEqual({
          ...values,
          activeApiToken: {
            ...newToken,
            engines: ['anotherEngine'],
          },
        });

...
        expect(CredentialsLogic.values).toEqual({
          ...values,
          anotherValueThatChanged: 'foo'
        });

...

        expect(CredentialsLogic.values).toEqual({
          ...values,
          someOtherThing: 'foo'
        });

Thoughts?

[cee-chen]

You totally beat me to this lol, I spent a few more hrs last night reading through the test file and definitely get now why my proposed solution doesn't work well for all the test suites (esp. the ones dealing w/ a bunch of deeply nested obj children).

You'd have to put in assertions for anotherValueThatChanged and someOtherThing as well, as seen above. At which point it's like, well I have a separate test for anotherValueThatChanged and someOtherThing below, why do I need both if it's being asserted here?

Definitely a good point, and also potentially a indication that (for some, definitely not all) tests, it may be worth considering whether it would be easier to just group all the changes values in a combined single test/assertion, rather than splitting them out 🤔 That being said, I totally get why not though for a lot of the more complex tests - also this is not me requesting a change to the current PR, just potentially food for thought for future logic tests.

All that being said, I also like the proposed compromise a lot! Would love to give it a shot - feel free to timebox though, if it starts taking too long to refactor or creates unexpected issues I'd also be fine leaving the current should not change tests as-is.

[JasonStoltz]

I removed the should not change tests and implemented the solution proposed above: 6347c61

[JasonStoltz]

I found some oddities around this logic file that I just noted in TODOs for now. This is a good example. Everywhere else in the code, when we change the activeApiToken, we also update activeApiTokenIsExisting. They are manually kept in sync. Here, we do not update activeApiTokenIsExisting.

I think the ultimate solution is to change activeApiTokenIsExisting to be a selector, so we don't need to try to keep it in sync everywhere.

[cee-chen]

I think this is a super good example of unit tests catching a design flaw in code! I'd be +++ for fixing this behavior - this does indeed sound like the exact use-case for a selector.

Also while we're here, would we have any objections to renaming activeApiTokenIsExisting? It's fairly awkward-sounding - maybe doesActiveApiTokenExist or even just hasActiveApiToken instead?

[cee-chen]

BTW, would activeTokenRawName be another good use-case to convert to a selector?

EDIT: After starting at this further it actually kinda feels to me like activeTokenRawName should be the reducer and activeTokenName should be the selector, since it undergoes a format. So something like activeTokenName is the raw name and activeTokenFormattedName is the final name 😛 Probably too annoying/large a change at this point tho so feel free to disregard unless you really like that idea

[JasonStoltz]

Since we know this works as is, and this PR is already big, I'd like to address this is a follow-up PR.

[cee-chen]

Sounds great! No rush at all!

[JasonStoltz]

This shouldn't ever happen theoretically, because this gets triggered after a user updates and saves a token. If this happens, something went wrong. I am just documenting the behavior as written, though.

[cee-chen]

Good point - I can't quite figure out if we should change this or not either 🤔 I'm fine kicking the can down on the road on this one I think / marking as tech debt for later

[JasonStoltz]

Since we know this works as is, and this PR is already big, I'd like to address this is a follow-up PR.

[JasonStoltz]

Another oddity I found.

[cee-chen]

Would be ++ for fixing. After some thought (maybe it's just very late) not totally sure how activeApiTokenRawName works as a selector tho - shouldn't activeApiTokenName be the selector instead? 🤷

[JasonStoltz]

Since we know this works as is, and this PR is already big, I'd like to address this is a follow-up PR.

[JasonStoltz]

Oh yeah, now that I think about it...

When a user enters a token name and then saves it, the token name is formatted before it's persisted to activeApiToken.name.

activeApiTokenRawName persists the unformatted name that the user had entered. I'm guessing this is used to back the form field, so is transient and only required until the user presses save.

[cee-chen]

Ran tests on the credentials file and noticed we had a just few more uncovered lines to go. I know it's possibly unhealthy to chase those sweet sweet green 100%s with too much fervor, but in this case I think it's not too much extra effort to cover the lines we need (and in one case, reveals unnecessary code).

Let me know what you think!

[JasonStoltz]

Ran tests on the credentials file and noticed we had a just few more uncovered lines to go.

Ah yes, I meant to run the coverage report but I ended up forgetting. I need to get in the habit of checking. I definitely feel the need to scratch that itch as well.

[JasonStoltz]

@constancecchen Ready for a second look

[cee-chen]

Changes look great!! Just two super minor comments left from me 🤞

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 5437fbd

Metrics [docs]

✅ unchanged

History

• 💚 Build #77017 succeeded a7db3ed
• 💚 Build #76576 succeeded 175198d
• 💚 Build #76564 succeeded c58121b
• 💔 Build #76336 failed b7349e5
• 💔 Build #76241 failed 77e519b

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

[cee-chen]

🎉 Wahoo!!! This looks amazing. Thanks so much as always for your patience with my review process Jason!

[cee-chen]

Totally forgot to mention this too, but this one's super optional so feel free to ignore or do it another PR if you like it :) This might be a good candidate for moving to credentials/types, but it's also used right here in this file, so whatever works for you!

[JasonStoltz]

Good call. I will put this one up in my next PR.

[JasonStoltz]

@constancecchen Thanks for the review!

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 77626 or prevent reminders by adding the backport:skip label.