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.

Sign up for GitHub

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

Jump to bottom

Fix Data Streams and Rollups Jobs deep-link bugs #70903

Merged
merged 4 commits into from
Jul 7, 2020
Merged

Fix Data Streams and Rollups Jobs deep-link bugs #70903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8692515
Add extractQueryParams to es_ui_shared/public/url.
cjcenizal Jul 6, 2020
c3c471d
Fix Data Streams bug in which clicking a data stream would apply a de…
cjcenizal Jul 6, 2020
cfe0b7a
Fix Rollup Job deep-link bug.
cjcenizal Jul 7, 2020
74bd3aa
Merge branch 'master' into bug/data-streams-deep-link-state
elasticmachine Jul 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/plugins/es_ui_shared/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ export {

export { Monaco, Forms };

export { extractQueryParams } from './url';
Copy link
Contributor

Choose a reason for hiding this comment

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

@cjcenizal @jloleysens I think we need to write some guidelines for our team about how to export from "es_ui_shared". I thought that we were aligned but I see we are not, and that's probably my mistake of not spreading the word.

The guidelines to follow, like we can see on L71, is to export namespace packages from the es_ui_shared folder. This implies to group items by "domain". Here I would have suggested having a "Routing" domain.

Each domain represents a top-level folder that lives inside the "packages_do_not_import" folder.

My main concerns are:

  • name collision. I don't want to have to tweak my interface names to not collide with other interfaces from other existing domains. If UpdateHandler makes sense in my context, I want to be able to use it, even if another domain is using it in a different context.

  • Pollute the public index.ts with all the interfaces, constants, functions from all the packages. This file will keep growing with time and it's much better to export "packages" like we do with

export { Monaco, Forms, ace };

If we need to know what a package contains, we then go and look at its barrel file inside its folder.

Until the guidelines are written I think it is important to keep an eye on that during code reviews. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sebelga , I do agree with this overall. I think it is best in as far as possible to follow the convention you've outlined (which is why I created #72034 in response to your comment :) ).

In this case naming of the function seemed so specific that I was not worried collision protection. That being said it would probably have been best if I'd also called it out here.

The other aspect of putting something that is fairly uniquely named inside of a module like Routing.extractQueryParams is the added step of unwrapping it later or updating all uses to have the module name prefix. On my initial review I just thought it might be overkill.

But again, in retrospect, I think it should have been called it attention 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 was not pointing you directly about the review 😊 Just sharing thoughts/concerns. Thanks for creating #72034 !

The thing about thinking "naming of the function seemed so specific that I was not worried collision protection" is that it is subject to interpretation so I think it is better to follow a pattern and stick to it.

For the extra unwrapping, yes there is an extra step but I think it is worth in the long run. True that we might need to update the code in a few places but I didn't find it too complex to do when I worked with Forms

// instead of this
import { FormWizard } from '../../shared_imports';

// we have this
import { Forms } from '../../shared_imports';
const { FormWizard } = Forms;

IMO it makes it very clear what reusable package(s) a file uses and we can understand the context of a specific handler/interface.


/** dummy plugin, we just want esUiShared to have its own bundle */
export function plugin() {
return new (class EsUiSharedPlugin {
Expand Down
29 changes: 29 additions & 0 deletions src/plugins/es_ui_shared/public/url/extract_query_params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { parse, ParsedQuery } from 'query-string';

export function extractQueryParams(queryString: string = ''): ParsedQuery<string> {
const hrefSplit = queryString.split('?');
if (!hrefSplit.length) {
return {};
}

return parse(hrefSplit[1], { sort: false });
}
20 changes: 20 additions & 0 deletions src/plugins/es_ui_shared/public/url/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

export { extractQueryParams } from './extract_query_params';
3 changes: 1 addition & 2 deletions x-pack/plugins/cross_cluster_replication/public/app/components/auto_follow_pattern_form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ import {
EuiTitle,
} from '@elastic/eui';

import { indices } from '../../../../../../src/plugins/es_ui_shared/public';
import { indexPatterns } from '../../../../../../src/plugins/data/public';

import { extractQueryParams, indices } from '../../shared_imports';
import { routing } from '../services/routing';
import { extractQueryParams } from '../services/query_params';
import { getRemoteClusterName } from '../services/get_remote_cluster_name';
import { API_STATUS } from '../constants';
import { SectionError } from './section_error';
Expand Down
7 changes: 3 additions & 4 deletions ...ross_cluster_replication/public/app/components/follower_index_form/follower_index_form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,21 @@ import {
EuiTitle,
} from '@elastic/eui';

import { indices } from '../../../../../../../src/plugins/es_ui_shared/public';
import { extractQueryParams, indices } from '../../../shared_imports';
import { indexNameValidator, leaderIndexValidator } from '../../services/input_validation';
import { routing } from '../../services/routing';
import { getFatalErrors } from '../../services/notifications';
import { loadIndices } from '../../services/api';
import { API_STATUS } from '../../constants';
import { getRemoteClusterName } from '../../services/get_remote_cluster_name';
import { RemoteClustersFormField } from '../remote_clusters_form_field';
import { SectionError } from '../section_error';
import { FormEntryRow } from '../form_entry_row';
import {
advancedSettingsFields,
emptyAdvancedSettings,
areAdvancedSettingsEdited,
} from './advanced_settings_fields';
import { extractQueryParams } from '../../services/query_params';
import { getRemoteClusterName } from '../../services/get_remote_cluster_name';
import { RemoteClustersFormField } from '../remote_clusters_form_field';

import { FollowerIndexRequestFlyout } from './follower_index_request_flyout';

Expand Down
2 changes: 1 addition & 1 deletion ...replication/public/app/sections/home/auto_follow_pattern_list/auto_follow_pattern_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
} from '@elastic/eui';

import { reactRouterNavigate } from '../../../../../../../../src/plugins/kibana_react/public';
import { extractQueryParams } from '../../../services/query_params';
import { extractQueryParams } from '../../../../shared_imports';
import { trackUiMetric, METRIC_TYPE } from '../../../services/track_ui_metric';
import { API_STATUS, UIM_AUTO_FOLLOW_PATTERN_LIST_LOAD } from '../../../constants';
import { SectionLoading, SectionError, SectionUnauthorized } from '../../../components';
Expand Down
2 changes: 1 addition & 1 deletion ...uster_replication/public/app/sections/home/follower_indices_list/follower_indices_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
} from '@elastic/eui';

import { reactRouterNavigate } from '../../../../../../../../src/plugins/kibana_react/public';
import { extractQueryParams } from '../../../services/query_params';
import { extractQueryParams } from '../../../../shared_imports';
import { trackUiMetric, METRIC_TYPE } from '../../../services/track_ui_metric';
import { API_STATUS, UIM_FOLLOWER_INDEX_LIST_LOAD } from '../../../constants';
import { SectionLoading, SectionError, SectionUnauthorized } from '../../../components';
Expand Down
2 changes: 1 addition & 1 deletion ...k/plugins/cross_cluster_replication/public/app/services/auto_follow_pattern_validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import React from 'react';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';

import { indices } from '../../../../../../src/plugins/es_ui_shared/public';
import { indexPatterns } from '../../../../../../src/plugins/data/public';
import { indices } from '../../shared_imports';

const {
indexNameBeginsWithPeriod,
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/cross_cluster_replication/public/app/services/input_validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

import React from 'react';
import { FormattedMessage } from '@kbn/i18n/react';
import { indices } from '../../../../../../src/plugins/es_ui_shared/public';

import { indices } from '../../shared_imports';

const isEmpty = (value) => {
return !value || !value.trim().length;
Expand Down
11 changes: 1 addition & 10 deletions ...ation/public/app/services/query_params.js → ...ster_replication/public/shared_imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { parse } from 'query-string';

export function extractQueryParams(queryString) {
const hrefSplit = queryString.split('?');
if (!hrefSplit.length) {
return {};
}

return parse(hrefSplit[1], { sort: false });
}
export { extractQueryParams, indices } from '../../../../src/plugins/es_ui_shared/public';
9 changes: 7 additions & 2 deletions ...s/index_management/public/application/sections/home/data_stream_list/data_stream_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { i18n } from '@kbn/i18n';
import { EuiTitle, EuiText, EuiSpacer, EuiEmptyPrompt, EuiLink } from '@elastic/eui';
import { ScopedHistory } from 'kibana/public';

import { reactRouterNavigate } from '../../../../shared_imports';
import { reactRouterNavigate, extractQueryParams } from '../../../../shared_imports';
import { useAppContext } from '../../../app_context';
import { SectionError, SectionLoading, Error } from '../../../components';
import { useLoadDataStreams } from '../../../services/api';
Expand All @@ -28,8 +28,11 @@ export const DataStreamList: React.FunctionComponent<RouteComponentProps<MatchPa
match: {
params: { dataStreamName },
},
location: { search },
history,
}) => {
const { isDeepLink } = extractQueryParams(search);

const {
core: { getUrlForApp },
plugins: { ingestManager },
Expand Down Expand Up @@ -144,7 +147,9 @@ export const DataStreamList: React.FunctionComponent<RouteComponentProps<MatchPa

<DataStreamTable
filters={
dataStreamName !== undefined ? `name=${decodePathFromReactRouter(dataStreamName)}` : ''
isDeepLink && dataStreamName !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I see the dataStreamName !== undefined logic existed before, but it might be worth changing here. This check is actually a lot more permissive than it needs to be (i.e., it tests true for all non-undefined values for dataStreamName). I would suggest dataStreamName != undefined.

No need to change if we have 100% confidence that dataStreamName will always be either a string or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, JL! Yes, I think we can be 100% confident it will always be either a string or undefined. It's provided by react router and is extracted from the path, so I'm not seeing any opportunities for it to be null, a number, or an object. I think we can leave as-is.

? `name=${decodePathFromReactRouter(dataStreamName)}`
: ''
}
dataStreams={dataStreams}
reload={reload}
Expand Down
1 change: 1 addition & 0 deletions ...s/index_management/public/application/sections/home/index_list/index_table/index_table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ export class IndexTable extends Component {
data-test-subj="dataStreamLink"
{...reactRouterNavigate(history, {
pathname: `/data_streams/${encodePathForReactRouter(value)}`,
search: '?isDeepLink=true',
})}
>
{value}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/index_management/public/shared_imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export {
sendRequest,
useRequest,
Forms,
extractQueryParams,
} from '../../../../src/plugins/es_ui_shared/public/';

export {
Expand Down
3 changes: 2 additions & 1 deletion ...gins/remote_clusters/public/application/sections/remote_cluster_add/remote_cluster_add.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import { FormattedMessage } from '@kbn/i18n/react';

import { EuiPageContent } from '@elastic/eui';

import { getRouter, redirect, extractQueryParams } from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { getRouter, redirect } from '../../services';
import { setBreadcrumbs } from '../../services/breadcrumb';
import { RemoteClusterPageTitle, RemoteClusterForm } from '../components';

Expand Down
3 changes: 2 additions & 1 deletion ...ns/remote_clusters/public/application/sections/remote_cluster_edit/remote_cluster_edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import {
} from '@elastic/eui';

import { reactRouterNavigate } from '../../../../../../../src/plugins/kibana_react/public';
import { extractQueryParams, getRouter, redirect } from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { getRouter, redirect } from '../../services';
import { setBreadcrumbs } from '../../services/breadcrumb';
import { RemoteClusterPageTitle, RemoteClusterForm, ConfiguredByNodeWarning } from '../components';

Expand Down
2 changes: 1 addition & 1 deletion ...ns/remote_clusters/public/application/sections/remote_cluster_list/remote_cluster_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
} from '@elastic/eui';

import { reactRouterNavigate } from '../../../../../../../src/plugins/kibana_react/public';
import { extractQueryParams } from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { setBreadcrumbs } from '../../services/breadcrumb';

import { RemoteClusterTable } from './remote_cluster_table';
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/remote_clusters/public/application/services/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ export { initRedirect, redirect } from './redirect';

export { isAddressValid, isPortValid } from './validate_address';

export { extractQueryParams } from './query_params';

export { setUserHasLeftApp, getUserHasLeftApp, registerRouter, getRouter } from './routing';

export { trackUiMetric, METRIC_TYPE } from './ui_metric';
8 changes: 2 additions & 6 deletions x-pack/plugins/remote_clusters/public/application/store/actions/add_cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@

import { i18n } from '@kbn/i18n';

import {
addCluster as sendAddClusterRequest,
getRouter,
extractQueryParams,
redirect,
} from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { addCluster as sendAddClusterRequest, getRouter, redirect } from '../../services';
import { fatalError, toasts } from '../../services/notification';

import {
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/remote_clusters/public/application/store/actions/detail_panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { extractQueryParams, getRouter } from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { getRouter } from '../../services';
import { OPEN_DETAIL_PANEL, CLOSE_DETAIL_PANEL } from '../action_types';

export const openDetailPanel = ({ name }) => (dispatch) => {
Expand Down
8 changes: 2 additions & 6 deletions x-pack/plugins/remote_clusters/public/application/store/actions/edit_cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,8 @@ import { i18n } from '@kbn/i18n';
import { toasts, fatalError } from '../../services/notification';
import { loadClusters } from './load_clusters';

import {
editCluster as sendEditClusterRequest,
extractQueryParams,
getRouter,
redirect,
} from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { editCluster as sendEditClusterRequest, getRouter, redirect } from '../../services';

import {
EDIT_CLUSTER_START,
Expand Down
11 changes: 1 addition & 10 deletions ...blic/application/services/query_params.js → .../remote_clusters/public/shared_imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { parse } from 'query-string';

export function extractQueryParams(queryString) {
const hrefSplit = queryString.split('?');
if (!hrefSplit.length) {
return {};
}

return parse(hrefSplit[1], { sort: false });
}
export { extractQueryParams, indices } from '../../../../src/plugins/es_ui_shared/public';
5 changes: 2 additions & 3 deletions x-pack/plugins/rollup/public/crud_app/sections/job_list/job_list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ import {

import { withKibana } from '../../../../../../../src/plugins/kibana_react/public';

import { getRouterLinkProps, extractQueryParams, listBreadcrumb } from '../../services';

import { extractQueryParams } from '../../../shared_imports';
import { getRouterLinkProps, listBreadcrumb } from '../../services';
import { JobTable } from './job_table';

import { DetailPanel } from './detail_panel';

const REFRESH_RATE_MS = 30000;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/rollup/public/crud_app/sections/job_list/job_table/job_table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ export class JobTable extends Component {
<EuiLink
onClick={() => {
trackUiMetric(METRIC_TYPE.CLICK, UIM_SHOW_DETAILS_CLICK);
openDetailPanel(job.id);
openDetailPanel(encodeURIComponent(job.id));
}}
>
{value}
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugins/rollup/public/crud_app/services/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ export { serializeJob, deserializeJob, deserializeJobs } from './jobs';

export { createNoticeableDelay } from './noticeable_delay';

export { extractQueryParams } from './query_params';

export {
setUserHasLeftApp,
getUserHasLeftApp,
Expand Down
23 changes: 0 additions & 23 deletions x-pack/plugins/rollup/public/crud_app/services/query_params.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion x-pack/plugins/rollup/public/crud_app/store/actions/create_job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export const createJob = (jobConfig) => async (dispatch) => {
// here, because it would partially obscure the detail panel.
getRouter().history.push({
pathname: `/job_list`,
search: `?job=${jobConfig.id}`,
search: `?job=${encodeURIComponent(jobConfig.id)}`,
});
};

Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/rollup/public/crud_app/store/actions/detail_panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { extractQueryParams, getRouter } from '../../services';
import { extractQueryParams } from '../../../shared_imports';
import { getRouter } from '../../services';
import { OPEN_DETAIL_PANEL, CLOSE_DETAIL_PANEL } from '../action_types';

export const openDetailPanel = ({ panelType, jobId }) => (dispatch) => {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/rollup/public/shared_imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@
* you may not use this file except in compliance with the Elastic License.
*/

export { indices } from '../../../../src/plugins/es_ui_shared/public';
export { extractQueryParams, indices } from '../../../../src/plugins/es_ui_shared/public';