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] Load curation settings at root /curations/ path #115690

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

byronhulcher
Copy link
Contributor

Summary

This will prevent us from showing the Suggestions table on the Curations Overview page when the user has disabled suggesstions.

Checklist

@byronhulcher byronhulcher requested a review from a team October 20, 2021 00:18
@byronhulcher byronhulcher added v7.16.0 auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes labels Oct 20, 2021
@byronhulcher byronhulcher self-assigned this Oct 20, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
enterpriseSearch 1.4MB 1.4MB +225.0B

History

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

cc @byronhulcher

@@ -126,14 +126,14 @@ describe('Curations', () => {

describe('loading state', () => {
it('renders a full-page loading state on initial page load', () => {
setMockValues({ ...values, dataLoading: true, curations: [] });
setMockValues({ ...values, dataLoading: true });
Copy link
Contributor Author

@byronhulcher byronhulcher Oct 20, 2021

Choose a reason for hiding this comment

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

Fun fact: this will mock both CurationsSettingsLogic.values.dataLoading and CurationsLogic.valaues.dataLoading at the same time, because they share a prop name, and all of our mocked properties share the same object. So there's actually not a way to test each of those individually. We'd need to 1) update the value name in one or both of these logic files or 2) refactor setMockValues so that it is possible to associate a mocked value with a specific logic.

Copy link
Member

Choose a reason for hiding this comment

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

"Fun"

Copy link
Member

Choose a reason for hiding this comment

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

I vote for refactoring setMockValues to use namespaces. It would fix this issue, and then we'd also no longer need to add comments to our tests specifying which logic values are associated with.

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

Hey Byron, I'm approving this. There is a small typo in a test description but looks good otherwise. Also in the process of pulling this down to test it but Optimizer is running slow, but I'm approving either way.

@@ -126,14 +126,14 @@ describe('Curations', () => {

describe('loading state', () => {
it('renders a full-page loading state on initial page load', () => {
setMockValues({ ...values, dataLoading: true, curations: [] });
setMockValues({ ...values, dataLoading: true });
Copy link
Member

Choose a reason for hiding this comment

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

"Fun"

@@ -126,14 +126,14 @@ describe('Curations', () => {

describe('loading state', () => {
it('renders a full-page loading state on initial page load', () => {
setMockValues({ ...values, dataLoading: true, curations: [] });
setMockValues({ ...values, dataLoading: true });
Copy link
Member

Choose a reason for hiding this comment

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

I vote for refactoring setMockValues to use namespaces. It would fix this issue, and then we'd also no longer need to add comments to our tests specifying which logic values are associated with.

expect(MOCK_ACTIONS.loadCurationsSettings).toHaveBeenCalledTimes(1);
});

it('skips loading curation settings when log retention is enabled', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('skips loading curation settings when log retention is enabled', () => {
it('skips loading curation settings when log retention is disabled', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ca74603

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants