[SR] Add client-side validation to repository form and link to snapshots from details by jen-huang · Pull Request #35238 · elastic/kibana

jen-huang · 2019-04-17T15:34:16Z

This PR adds client-side validation to repository form. The only rules that are currently implemented is checking that required fields are non-empty, however the setup lends itself well to check any repository setting with any arbitrary rule.

Example of form validation errors:

This PR also adds number of snapshots to repository details, and links to snapshots list filtered down to that repository:

…mmon lib

…e/snapshots-validation

…es and spaces

elasticmachine · 2019-04-17T15:34:18Z

Pinging @elastic/es-ui

jen-huang · 2019-04-17T15:35:57Z

...ns/snapshot_restore/public/app/sections/home/snapshot_list/snapshot_table/snapshot_table.tsx

      },
    ],
+    defaultQuery: filterToRepository
+      ? Query.parse(`repository:'${filterToRepository}'`, {


passing a query object with a strict schema is needed here because it kept parsing strings like repository:'with spaces' and repository:'with/slash' as dates and failed to work

Do you think it's worth filing an EUI bug?

added comment to existing bug: elastic/eui#1855

EuiSearchBar / Query treat single-quoted values as dates. You want to use double quotes instead (though your forward slash will still not be accepted until elastic/eui#1855 is resolved)

elasticmachine · 2019-04-17T16:05:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-17T17:18:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

💯 Tested locally, code looks great to me! Great work!!

cjcenizal · 2019-04-18T00:19:35Z

x-pack/plugins/snapshot_restore/common/lib/flatten.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export const flatten = (source: any, path: any[] = []): { [key: string]: any } => {


I think a couple brief unit tests around this wouldn't hurt. At the very least it would communicate to a dev what the expected inputs and outputs are. You might want to include a test case where one of the property names contains a period, just to make it clear that the path isn't always perfectly represented by the flattened key.

but this was written by @lukasolson, and is therefore impossible to break! just kidding 😆 I'll add some tests

cjcenizal · 2019-04-18T00:30:12Z

...ugins/snapshot_restore/public/app/components/repository_form/type_settings/hdfs_settings.tsx

            />
          }
          fullWidth
+          describedByIds={['hdfsRepositoryUriDescription']}


Nice catch!

cjcenizal · 2019-04-18T00:33:49Z

...t_restore/public/app/sections/home/repository_list/repository_details/repository_details.tsx

+          </h3>
+        </EuiTitle>
+        <EuiSpacer size="s" />
+        {Number.isInteger(snapshots.count) ? (


I know it's very unlikely this will happen, but Number.isInteger(0) evaluates to true, so maybe we should should change this condition to this:

Number(snapshots.count) > 0

This will return true for undefined and null and will work if count is provided as a string.

ah, this was actually done intentionally. sorry, I realize now that it is not very obvious.

if there was an issue with retrieving snapshot information (such as when there is an error on the repository), the server API will return null. otherwise if there were no issues with retrieving information and there are no snapshots, it will return 0

0 snapshots found means empty repository
No snapshot information means there was an issue with accessing the repository snapshot info

I'm open to copy suggestions for these scenarios!

Gotcha! Here's my suggestion:

When there are 0 snapshots, render "No snapshots in this repository" (I'm sure Gail will suggest better copy). No link, because I think there's no point if there are no snapshots.

When there are >0 snapshots, render "X snapshots found" as a link.

In all other scenarios, render "No snapshot information".

WDYT?

cjcenizal · 2019-04-18T00:34:39Z

...t_restore/public/app/sections/home/repository_list/repository_details/repository_details.tsx

+        ) : (
+          <FormattedMessage
+            id="xpack.snapshotRestore.repositoryDetails.noSnapshotsDescription"
+            defaultMessage="No snapshot information"


If count is 0, can we show a message saying "Repository contains no snapshots"?

cjcenizal · 2019-04-18T00:38:41Z

x-pack/plugins/snapshot_restore/public/app/sections/home/snapshot_list/snapshot_list.tsx

          repositories={repositories}
          reload={reload}
          openSnapshotDetails={openSnapshotDetails}
+          filterToRepository={filteredRepository}


Minor nit: I think a prop name of repositoryFilter would make more sense to me, since this is just the string that gets applied as a filter for the repository field. filterToRepository confused me a bit because it sounded like maybe it did more than that.

cjcenizal · 2019-04-18T00:39:47Z

...ns/snapshot_restore/public/app/sections/home/snapshot_list/snapshot_table/snapshot_table.tsx

      },
    ],
+    defaultQuery: filterToRepository
+      ? Query.parse(`repository:'${filterToRepository}'`, {


Do you think it's worth filing an EUI bug?

cjcenizal · 2019-04-18T00:40:26Z

x-pack/plugins/snapshot_restore/public/app/services/navigation/links.ts


 export function linkToRepository(repositoryName: string) {
-  return `#${BASE_PATH}/repositories/${repositoryName}`;
+  return `#${BASE_PATH}/repositories/${encodeURIComponent(repositoryName)}`;


Thanks for catching this, too!

cjcenizal · 2019-04-18T00:42:04Z

x-pack/plugins/snapshot_restore/public/app/services/validation/validate_repository.ts

+
+export const validateRepository = (repository: Repository): RepositoryValidation => {
+  const { name, type, settings } = repository;
+  const i18n = textService.i18n;


Super minor nit: this could also be const { i18n } = textService;

elasticmachine · 2019-04-18T23:27:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* [SR] Snapshot and restore plugin boilerplate (#32276) * Initial plugin set up * Set up client shell * Add initial repository list routes * Fix merge issues and some typings * Decouple server from plugin.ts files, tighten up typings * Use exported constant for required license * Translate plugin name, more typings * Fix more types, move list components under /home * Remove unused var * Change scss prefix * Uncouple unmount logic from routing shim, and some other PR feedback * [SR] Repository list and details UI (#33367) * Initial pass at repositories list UI * Add detail panel for file system repositories, and a generic detail panel with json settings view * Add detail components for other types * Add detail panel footer, rename `useStateValue` to `useAppState` * Fix detail panel footer * Fix unused vars * PR feedback * PR feedback * [SR] Refactor proposal (#33690) * Move app dependencies to its own context provider * Add index.ts barrel file for common types * Move Enums to constants.ts file * Refactor function component using `React.FunctionComponent<Props>` * Refactor service folder structure * Fix type import * Move REPOSITORY_DOC_PATHS from common to public constants * Move AppCore and AppPlugins interfaces back to shim and re-export them from app types * [SR] Create and edit repositories UI (#34020) * Add routing and placeholder form * Fix typings * Set up edit repository route, and basic form UI * Add typings for wrapCustomError, and copy extractCausedByChain from CCR wrapEsError * Throw errors that are already boomified * Create and edit for basic repository types (fs, url, source) * Add repository verification UI to table and details * Create and edit for plugin repository types (hdfs, azure, s3, gcs) * Fix linting * Fix test * Fix test * Remove unused import * Fix duplicate i18n key * Fix details opening on cancel edit, remove unnecessary Fragments, definition file for some EUI components to x-pack, rename saveError * Remove breaks * Adjust add and edit repo routes so they don't conflict with list route * Add repo plugin and types doc links to form * Bootstrap documentation service * Bootstrap text service and replace RepositoryTypeName component with it * Bootstrap breadcrumb service and replace usages * Bootstrap httpService, remove chrome and http from app dependencies(!) * Add request creator and replace all instances of useRequest and sendRequest with it * Fix typo * Simplify update repository and update repository setting methods * Adjust copy * Lint * Remove unused var * Remove unused import * [SR] Add API for retrieving snapshots. (#34598) * [SR] Single and multiple repository delete (#34593) * Add single/multi repository delete API and UI * Address PR feedback * [SR] Add SnapshotTable and SnapshotDetails. (#34837) * Remove associations between multiple repositories with a single snapshot. * Retrieve complete snapshot details in getAllHandler. * Fix cleanup function bug in useRequest hook. * Fix bug in useRequest which prevented old data from being cleared when subsequent requests returned errors. * Add initialValue config option to useRequest. * Add formatDate service to text module. * [SR] Fix linting and add (de)serialization for repositories (#35031) * Fix eslint issues and add (de)serialization for repositories * Add comment about flattening settings * [SR] Surface repository errors and index failures more prominently (#35042) * Add links to repositories from Snapshot Table and Snapshot Details. - Rename services/breadcrumbs to services/navigation and add linkToRepository function. - Refactor home component to update active tab when URL was changed. * Add warning callout to let user know when their repositories contain errors. * Sort failures by shard and add test for snapshot serialization. * Sort failures and indices. * Add filter for filtering snapshots by their repository. * Surface states with humanized text, icons, and tooltips where necessary. * Fix pluralization of seconds. * Surface failures tab even if there are none. - Display a '-' for missing times and durations. - Create DataPlaceholder component. * [SR] Polish repositories UX (#35123) * Refactor repository detail panel to load repository based directly on route param. * Display repository detail panel while table is loading. * Make 'Edit repository' table action a link instead of a button. * Render disabled EuiSelect as a readonly EuiFieldText. * Prepend HDFS URI with hdfs:// protocol. * Present scheme options for Read-Only URL repository as a select. * [SR] Add client-side validation to repository form and link to snapshots from details (#35238) * Add client side repository form validation, extract `flatten` into common lib * Add snapshot count to repository details and link to snapshot list * Reset validation when changing repository type * Fix snapshot list filter deep linking for repository names with slashes and spaces * Fix imports * PR feedback * [SR] Design and copywriting fixes (#35591) * Split repository form into two steps; move `clean_settings.ts` to server * Default to snapshots tab, adjust snapshot empty prompt, add app description * Add minimum timeout to list view requests to avoid flicker, use EuiEmptyPrompt for loading screen, add doc link to settings step * Add information about snapshots to delete repository behavior, add doc link for source only toggle, add size notation help text * Add main doc link * Copywriting and i18n fixes, and add some common settings to third party repo types * Add fields to third party repo detail panel * More copywriting fixes * Use spinner for duration and end time if snapshotting is still in progress * Show all repository type options, mark missing plugins * Revert "Show all repository type options, mark missing plugins" This reverts commit e34ee47. * Fix space * [SR] Add permissions UI and Cloud-specific repository type UI branch (#35833) * Add missing permissions UI and cloud-specific repository type UI branch * Add ES UI as owners of /snapshot_restore directory * Add no repository types callout for Cloud edge case * Redirect invalid section param to repositories * Add warning empty prompt if all repositories have errrors * Replace repository cards with EuiCard * Add snapshot doc link to repository error empty prompt * Remove auto-verification from list and get routes, add separate verification route, add manual verification to repository detail panel * Update copy and remove obsolete test * Remove unused scss files * Final changes to repository cards

* [SR] Snapshot and restore plugin boilerplate (elastic#32276) * Initial plugin set up * Set up client shell * Add initial repository list routes * Fix merge issues and some typings * Decouple server from plugin.ts files, tighten up typings * Use exported constant for required license * Translate plugin name, more typings * Fix more types, move list components under /home * Remove unused var * Change scss prefix * Uncouple unmount logic from routing shim, and some other PR feedback * [SR] Repository list and details UI (elastic#33367) * Initial pass at repositories list UI * Add detail panel for file system repositories, and a generic detail panel with json settings view * Add detail components for other types * Add detail panel footer, rename `useStateValue` to `useAppState` * Fix detail panel footer * Fix unused vars * PR feedback * PR feedback * [SR] Refactor proposal (elastic#33690) * Move app dependencies to its own context provider * Add index.ts barrel file for common types * Move Enums to constants.ts file * Refactor function component using `React.FunctionComponent<Props>` * Refactor service folder structure * Fix type import * Move REPOSITORY_DOC_PATHS from common to public constants * Move AppCore and AppPlugins interfaces back to shim and re-export them from app types * [SR] Create and edit repositories UI (elastic#34020) * Add routing and placeholder form * Fix typings * Set up edit repository route, and basic form UI * Add typings for wrapCustomError, and copy extractCausedByChain from CCR wrapEsError * Throw errors that are already boomified * Create and edit for basic repository types (fs, url, source) * Add repository verification UI to table and details * Create and edit for plugin repository types (hdfs, azure, s3, gcs) * Fix linting * Fix test * Fix test * Remove unused import * Fix duplicate i18n key * Fix details opening on cancel edit, remove unnecessary Fragments, definition file for some EUI components to x-pack, rename saveError * Remove breaks * Adjust add and edit repo routes so they don't conflict with list route * Add repo plugin and types doc links to form * Bootstrap documentation service * Bootstrap text service and replace RepositoryTypeName component with it * Bootstrap breadcrumb service and replace usages * Bootstrap httpService, remove chrome and http from app dependencies(!) * Add request creator and replace all instances of useRequest and sendRequest with it * Fix typo * Simplify update repository and update repository setting methods * Adjust copy * Lint * Remove unused var * Remove unused import * [SR] Add API for retrieving snapshots. (elastic#34598) * [SR] Single and multiple repository delete (elastic#34593) * Add single/multi repository delete API and UI * Address PR feedback * [SR] Add SnapshotTable and SnapshotDetails. (elastic#34837) * Remove associations between multiple repositories with a single snapshot. * Retrieve complete snapshot details in getAllHandler. * Fix cleanup function bug in useRequest hook. * Fix bug in useRequest which prevented old data from being cleared when subsequent requests returned errors. * Add initialValue config option to useRequest. * Add formatDate service to text module. * [SR] Fix linting and add (de)serialization for repositories (elastic#35031) * Fix eslint issues and add (de)serialization for repositories * Add comment about flattening settings * [SR] Surface repository errors and index failures more prominently (elastic#35042) * Add links to repositories from Snapshot Table and Snapshot Details. - Rename services/breadcrumbs to services/navigation and add linkToRepository function. - Refactor home component to update active tab when URL was changed. * Add warning callout to let user know when their repositories contain errors. * Sort failures by shard and add test for snapshot serialization. * Sort failures and indices. * Add filter for filtering snapshots by their repository. * Surface states with humanized text, icons, and tooltips where necessary. * Fix pluralization of seconds. * Surface failures tab even if there are none. - Display a '-' for missing times and durations. - Create DataPlaceholder component. * [SR] Polish repositories UX (elastic#35123) * Refactor repository detail panel to load repository based directly on route param. * Display repository detail panel while table is loading. * Make 'Edit repository' table action a link instead of a button. * Render disabled EuiSelect as a readonly EuiFieldText. * Prepend HDFS URI with hdfs:// protocol. * Present scheme options for Read-Only URL repository as a select. * [SR] Add client-side validation to repository form and link to snapshots from details (elastic#35238) * Add client side repository form validation, extract `flatten` into common lib * Add snapshot count to repository details and link to snapshot list * Reset validation when changing repository type * Fix snapshot list filter deep linking for repository names with slashes and spaces * Fix imports * PR feedback * [SR] Design and copywriting fixes (elastic#35591) * Split repository form into two steps; move `clean_settings.ts` to server * Default to snapshots tab, adjust snapshot empty prompt, add app description * Add minimum timeout to list view requests to avoid flicker, use EuiEmptyPrompt for loading screen, add doc link to settings step * Add information about snapshots to delete repository behavior, add doc link for source only toggle, add size notation help text * Add main doc link * Copywriting and i18n fixes, and add some common settings to third party repo types * Add fields to third party repo detail panel * More copywriting fixes * Use spinner for duration and end time if snapshotting is still in progress * Show all repository type options, mark missing plugins * Revert "Show all repository type options, mark missing plugins" This reverts commit e34ee47. * Fix space * [SR] Add permissions UI and Cloud-specific repository type UI branch (elastic#35833) * Add missing permissions UI and cloud-specific repository type UI branch * Add ES UI as owners of /snapshot_restore directory * Add no repository types callout for Cloud edge case * Redirect invalid section param to repositories * Add warning empty prompt if all repositories have errrors * Replace repository cards with EuiCard * Add snapshot doc link to repository error empty prompt * Remove auto-verification from list and get routes, add separate verification route, add manual verification to repository detail panel * Update copy and remove obsolete test * Remove unused scss files * Final changes to repository cards

* [SR] Snapshot and restore plugin boilerplate (#32276) * Initial plugin set up * Set up client shell * Add initial repository list routes * Fix merge issues and some typings * Decouple server from plugin.ts files, tighten up typings * Use exported constant for required license * Translate plugin name, more typings * Fix more types, move list components under /home * Remove unused var * Change scss prefix * Uncouple unmount logic from routing shim, and some other PR feedback * [SR] Repository list and details UI (#33367) * Initial pass at repositories list UI * Add detail panel for file system repositories, and a generic detail panel with json settings view * Add detail components for other types * Add detail panel footer, rename `useStateValue` to `useAppState` * Fix detail panel footer * Fix unused vars * PR feedback * PR feedback * [SR] Refactor proposal (#33690) * Move app dependencies to its own context provider * Add index.ts barrel file for common types * Move Enums to constants.ts file * Refactor function component using `React.FunctionComponent<Props>` * Refactor service folder structure * Fix type import * Move REPOSITORY_DOC_PATHS from common to public constants * Move AppCore and AppPlugins interfaces back to shim and re-export them from app types * [SR] Create and edit repositories UI (#34020) * Add routing and placeholder form * Fix typings * Set up edit repository route, and basic form UI * Add typings for wrapCustomError, and copy extractCausedByChain from CCR wrapEsError * Throw errors that are already boomified * Create and edit for basic repository types (fs, url, source) * Add repository verification UI to table and details * Create and edit for plugin repository types (hdfs, azure, s3, gcs) * Fix linting * Fix test * Fix test * Remove unused import * Fix duplicate i18n key * Fix details opening on cancel edit, remove unnecessary Fragments, definition file for some EUI components to x-pack, rename saveError * Remove breaks * Adjust add and edit repo routes so they don't conflict with list route * Add repo plugin and types doc links to form * Bootstrap documentation service * Bootstrap text service and replace RepositoryTypeName component with it * Bootstrap breadcrumb service and replace usages * Bootstrap httpService, remove chrome and http from app dependencies(!) * Add request creator and replace all instances of useRequest and sendRequest with it * Fix typo * Simplify update repository and update repository setting methods * Adjust copy * Lint * Remove unused var * Remove unused import * [SR] Add API for retrieving snapshots. (#34598) * [SR] Single and multiple repository delete (#34593) * Add single/multi repository delete API and UI * Address PR feedback * [SR] Add SnapshotTable and SnapshotDetails. (#34837) * Remove associations between multiple repositories with a single snapshot. * Retrieve complete snapshot details in getAllHandler. * Fix cleanup function bug in useRequest hook. * Fix bug in useRequest which prevented old data from being cleared when subsequent requests returned errors. * Add initialValue config option to useRequest. * Add formatDate service to text module. * [SR] Fix linting and add (de)serialization for repositories (#35031) * Fix eslint issues and add (de)serialization for repositories * Add comment about flattening settings * [SR] Surface repository errors and index failures more prominently (#35042) * Add links to repositories from Snapshot Table and Snapshot Details. - Rename services/breadcrumbs to services/navigation and add linkToRepository function. - Refactor home component to update active tab when URL was changed. * Add warning callout to let user know when their repositories contain errors. * Sort failures by shard and add test for snapshot serialization. * Sort failures and indices. * Add filter for filtering snapshots by their repository. * Surface states with humanized text, icons, and tooltips where necessary. * Fix pluralization of seconds. * Surface failures tab even if there are none. - Display a '-' for missing times and durations. - Create DataPlaceholder component. * [SR] Polish repositories UX (#35123) * Refactor repository detail panel to load repository based directly on route param. * Display repository detail panel while table is loading. * Make 'Edit repository' table action a link instead of a button. * Render disabled EuiSelect as a readonly EuiFieldText. * Prepend HDFS URI with hdfs:// protocol. * Present scheme options for Read-Only URL repository as a select. * [SR] Add client-side validation to repository form and link to snapshots from details (#35238) * Add client side repository form validation, extract `flatten` into common lib * Add snapshot count to repository details and link to snapshot list * Reset validation when changing repository type * Fix snapshot list filter deep linking for repository names with slashes and spaces * Fix imports * PR feedback * [SR] Design and copywriting fixes (#35591) * Split repository form into two steps; move `clean_settings.ts` to server * Default to snapshots tab, adjust snapshot empty prompt, add app description * Add minimum timeout to list view requests to avoid flicker, use EuiEmptyPrompt for loading screen, add doc link to settings step * Add information about snapshots to delete repository behavior, add doc link for source only toggle, add size notation help text * Add main doc link * Copywriting and i18n fixes, and add some common settings to third party repo types * Add fields to third party repo detail panel * More copywriting fixes * Use spinner for duration and end time if snapshotting is still in progress * Show all repository type options, mark missing plugins * Revert "Show all repository type options, mark missing plugins" This reverts commit e34ee47. * Fix space * [SR] Add permissions UI and Cloud-specific repository type UI branch (#35833) * Add missing permissions UI and cloud-specific repository type UI branch * Add ES UI as owners of /snapshot_restore directory * Add no repository types callout for Cloud edge case * Redirect invalid section param to repositories * Add warning empty prompt if all repositories have errrors * Replace repository cards with EuiCard * Add snapshot doc link to repository error empty prompt * Remove auto-verification from list and get routes, add separate verification route, add manual verification to repository detail panel * Update copy and remove obsolete test * Remove unused scss files * Final changes to repository cards

jen-huang added 6 commits April 16, 2019 12:26

Add client side repository form validation, extract flatten into co…

d39e520

…mmon lib

Merge remote-tracking branch 'upstream/feature/snapshots' into featur…

1682814

…e/snapshots-validation

Add snapshot count to repository details and link to snapshot list

04c94fb

Merge remote-tracking branch 'upstream/feature/snapshots' into featur…

ebeb759

…e/snapshots-validation

Reset validation when changing repository type

d7c007c

Fix snapshot list filter deep linking for repository names with slash…

91f944f

…es and spaces

jen-huang commented Apr 17, 2019

View reviewed changes

jen-huang requested a review from cjcenizal April 17, 2019 15:36

Fix imports

67791aa

cjcenizal approved these changes Apr 18, 2019

View reviewed changes

jen-huang mentioned this pull request Apr 18, 2019

EuiSearchBar doesn't support forward slashes when query prop is a string elastic/eui#1855

Closed

PR feedback

01865b2

jen-huang merged commit 4f0d17b into elastic:feature/snapshots Apr 18, 2019

jen-huang deleted the feature/snapshots-validation branch April 18, 2019 23:45

Conversation

jen-huang commented Apr 17, 2019

Uh oh!

elasticmachine commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chandlerprall Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 17, 2019

💚 Build Succeeded

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jen-huang Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 18, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chandlerprall Apr 19, 2019 •

edited

Loading

jen-huang Apr 18, 2019 •

edited

Loading