From 6c82142643a1ecd677a1a5e9c024a73638ba2c23 Mon Sep 17 00:00:00 2001 From: Florian Scholdei Date: Tue, 2 Aug 2022 10:30:07 +0200 Subject: [PATCH] Fix paging for too large page numbers (#2097) On some pages with pagination, the user is led to believe that no data is available if a page with page number which it too high is accessed. However, since we show the page number to the outside and the user can access it through the URL, we must also provide appropriate handling. The underlying data can change and so can the number of pages. Now, if a bookmark was saved from an older version, the link should still lead to a destination. --- gradle/changelog/fix_paging.yaml | 2 + .../src/__snapshots__/storyshots.test.ts.snap | 2 +- .../src/markdown/MarkdownView.stories.tsx | 2 +- .../roles/containers/RepositoryRoles.tsx | 61 +++++++++++-------- .../groups/components/table/GroupTable.tsx | 49 ++++++++------- .../src/groups/containers/Groups.tsx | 54 ++++++++++------ .../src/repos/containers/Changesets.tsx | 42 ++++++------- .../src/repos/containers/ChangesetsRoot.tsx | 5 +- scm-ui/ui-webapp/src/search/Hits.tsx | 6 +- scm-ui/ui-webapp/src/search/Results.tsx | 15 ++++- .../src/users/components/table/UserTable.tsx | 49 ++++++++------- .../ui-webapp/src/users/containers/Users.tsx | 56 +++++++++-------- 12 files changed, 189 insertions(+), 154 deletions(-) create mode 100644 gradle/changelog/fix_paging.yaml diff --git a/gradle/changelog/fix_paging.yaml b/gradle/changelog/fix_paging.yaml new file mode 100644 index 0000000000..bb9c476082 --- /dev/null +++ b/gradle/changelog/fix_paging.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Fix paging for too large page numbers ([#2097](https://github.com/scm-manager/scm-manager/pull/2097)) diff --git a/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap b/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap index 18331a4410..bdf7daec5f 100644 --- a/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap +++ b/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap @@ -4996,7 +4996,7 @@ exports[`Storyshots MarkdownView Custom code renderer 1`] = ` } } > - To render plantuml as images within markdown, please install the scm-markdown-plantuml-plguin + To render plantuml as images within markdown, please install the scm-markdown-plantuml-plugin
           actor Foo1
diff --git a/scm-ui/ui-components/src/markdown/MarkdownView.stories.tsx b/scm-ui/ui-components/src/markdown/MarkdownView.stories.tsx
index 326164fa05..724d76de9a 100644
--- a/scm-ui/ui-components/src/markdown/MarkdownView.stories.tsx
+++ b/scm-ui/ui-components/src/markdown/MarkdownView.stories.tsx
@@ -101,7 +101,7 @@ storiesOf("MarkdownView", module)
       return (
         

- To render plantuml as images within markdown, please install the scm-markdown-plantuml-plguin + To render plantuml as images within markdown, please install the scm-markdown-plantuml-plugin

{value}
diff --git a/scm-ui/ui-webapp/src/admin/roles/containers/RepositoryRoles.tsx b/scm-ui/ui-webapp/src/admin/roles/containers/RepositoryRoles.tsx index f3c64062c1..d732f34d15 100644 --- a/scm-ui/ui-webapp/src/admin/roles/containers/RepositoryRoles.tsx +++ b/scm-ui/ui-webapp/src/admin/roles/containers/RepositoryRoles.tsx @@ -22,8 +22,9 @@ * SOFTWARE. */ import React, { FC } from "react"; -import { useParams } from "react-router-dom"; +import { Redirect, useParams } from "react-router-dom"; import { useTranslation } from "react-i18next"; +import { RepositoryRoleCollection } from "@scm-manager/ui-types"; import { CreateButton, ErrorNotification, @@ -32,11 +33,33 @@ import { Notification, Subtitle, Title, - urls + urls, } from "@scm-manager/ui-components"; import PermissionRoleTable from "../components/PermissionRoleTable"; import { useRepositoryRoles } from "@scm-manager/ui-api"; +type RepositoryRolesPageProps = { + data?: RepositoryRoleCollection; + page: number; + baseUrl: string; +}; + +const RepositoryRolesPage: FC = ({ data, page, baseUrl }) => { + const [t] = useTranslation("users"); + const roles = data?._embedded?.repositoryRoles; + + if (!data || !roles || roles.length === 0) { + return {t("repositoryRole.overview.noPermissionRoles")}; + } + + return ( + <> + + + + ); +}; + type Props = { baseUrl: string; }; @@ -44,29 +67,9 @@ type Props = { const RepositoryRoles: FC = ({ baseUrl }) => { const params = useParams(); const page = urls.getPageFromMatch({ params }); - const { isLoading: loading, error, data: list } = useRepositoryRoles({ page: page - 1 }); + const { isLoading: loading, error, data } = useRepositoryRoles({ page: page - 1 }); const [t] = useTranslation("admin"); - const roles = list?._embedded.repositoryRoles; - const canAddRoles = !!list?._links.create; - - const renderPermissionsTable = () => { - if (list && roles && roles.length > 0) { - return ( - <> - - - - ); - } - return {t("repositoryRole.overview.noPermissionRoles")}; - }; - - const renderCreateButton = () => { - if (canAddRoles) { - return ; - } - return null; - }; + const canAddRoles = !!data?._links.create; if (error) { return ; @@ -76,12 +79,18 @@ const RepositoryRoles: FC = ({ baseUrl }) => { return ; } + if (data && data.pageTotal < page && page > 1) { + return ; + } + return ( <> <Subtitle subtitle={t("repositoryRole.overview.title")} /> - {renderPermissionsTable()} - {renderCreateButton()} + <RepositoryRolesPage data={data} page={page} baseUrl={baseUrl} /> + {canAddRoles ? ( + <CreateButton label={t("repositoryRole.overview.createButton")} link={`${baseUrl}/create`} /> + ) : null} </> ); }; diff --git a/scm-ui/ui-webapp/src/groups/components/table/GroupTable.tsx b/scm-ui/ui-webapp/src/groups/components/table/GroupTable.tsx index 27b42cff74..4027bf6110 100644 --- a/scm-ui/ui-webapp/src/groups/components/table/GroupTable.tsx +++ b/scm-ui/ui-webapp/src/groups/components/table/GroupTable.tsx @@ -21,34 +21,33 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ -import React from "react"; -import { WithTranslation, withTranslation } from "react-i18next"; -import GroupRow from "./GroupRow"; +import React, { FC } from "react"; +import { useTranslation } from "react-i18next"; import { Group } from "@scm-manager/ui-types"; +import GroupRow from "./GroupRow"; -type Props = WithTranslation & { +type Props = { groups: Group[]; }; -class GroupTable extends React.Component<Props> { - render() { - const { groups, t } = this.props; - return ( - <table className="card-table table is-hoverable is-fullwidth"> - <thead> - <tr> - <th>{t("group.name")}</th> - <th className="is-hidden-mobile">{t("group.description")}</th> - </tr> - </thead> - <tbody> - {groups.map((group, index) => { - return <GroupRow key={index} group={group} />; - })} - </tbody> - </table> - ); - } -} +const GroupTable: FC<Props> = ({ groups }) => { + const [t] = useTranslation("groups"); -export default withTranslation("groups")(GroupTable); + return ( + <table className="card-table table is-hoverable is-fullwidth"> + <thead> + <tr> + <th>{t("group.name")}</th> + <th className="is-hidden-mobile">{t("group.description")}</th> + </tr> + </thead> + <tbody> + {groups.map((group, index) => { + return <GroupRow key={index} group={group} />; + })} + </tbody> + </table> + ); +}; + +export default GroupTable; diff --git a/scm-ui/ui-webapp/src/groups/containers/Groups.tsx b/scm-ui/ui-webapp/src/groups/containers/Groups.tsx index f6f922c4a1..7e22da2855 100644 --- a/scm-ui/ui-webapp/src/groups/containers/Groups.tsx +++ b/scm-ui/ui-webapp/src/groups/containers/Groups.tsx @@ -22,8 +22,10 @@ * SOFTWARE. */ import React, { FC } from "react"; +import { Redirect, useLocation, useParams } from "react-router-dom"; import { useTranslation } from "react-i18next"; -import { useLocation, useParams } from "react-router-dom"; +import { useGroups } from "@scm-manager/ui-api"; +import { Group, GroupCollection } from "@scm-manager/ui-types"; import { CreateButton, LinkPaginator, @@ -31,32 +33,44 @@ import { OverviewPageActions, Page, PageActions, - urls + urls, } from "@scm-manager/ui-components"; import { GroupTable } from "./../components/table"; -import { useGroups } from "@scm-manager/ui-api"; + +type GroupPageProps = { + data?: GroupCollection; + groups?: Group[]; + page: number; + search?: string; +}; + +const GroupPage: FC<GroupPageProps> = ({ data, groups, page, search }) => { + const [t] = useTranslation("groups"); + + if (!data || !groups || groups.length === 0) { + return <Notification type="info">{t("groups.noGroups")}</Notification>; + } + + return ( + <> + <GroupTable groups={groups} /> + <LinkPaginator collection={data} page={page} filter={search} /> + </> + ); +}; const Groups: FC = () => { const location = useLocation(); const params = useParams(); const search = urls.getQueryStringFromLocation(location); const page = urls.getPageFromMatch({ params }); - const { isLoading, error, data: list } = useGroups({ search, page: page - 1 }); + const { isLoading, error, data } = useGroups({ search, page: page - 1 }); const [t] = useTranslation("groups"); - const groups = list?._embedded.groups; - const canCreateGroups = !!list?._links.create; - - const renderGroupTable = () => { - if (list && groups && groups.length > 0) { - return ( - <> - <GroupTable groups={groups} /> - <LinkPaginator collection={list} page={page} filter={urls.getQueryStringFromLocation(location)} /> - </> - ); - } - return <Notification type="info">{t("groups.noGroups")}</Notification>; - }; + const groups = data?._embedded?.groups; + const canCreateGroups = !!data?._links.create; + if (data && data.pageTotal < page && page > 1) { + return <Redirect to={`/groups/${data.pageTotal}`} />; + } return ( <Page @@ -65,8 +79,8 @@ const Groups: FC = () => { loading={isLoading || !groups} error={error || undefined} > - {renderGroupTable()} - {canCreateGroups ? <CreateButton label={t("groups.createButton")} link="/groups/create" /> : null} + <GroupPage data={data} groups={groups} page={page} search={search} /> + {canCreateGroups ? <CreateButton link="/groups/create" label={t("groups.createButton")} /> : null} <PageActions> <OverviewPageActions showCreateButton={canCreateGroups} diff --git a/scm-ui/ui-webapp/src/repos/containers/Changesets.tsx b/scm-ui/ui-webapp/src/repos/containers/Changesets.tsx index 9749159803..99aeb5705c 100644 --- a/scm-ui/ui-webapp/src/repos/containers/Changesets.tsx +++ b/scm-ui/ui-webapp/src/repos/containers/Changesets.tsx @@ -22,46 +22,34 @@ * SOFTWARE. */ import React, { FC } from "react"; -import { useRouteMatch } from "react-router-dom"; +import { Redirect, useRouteMatch } from "react-router-dom"; import { useTranslation } from "react-i18next"; -import { Branch, ChangesetCollection, Repository } from "@scm-manager/ui-types"; +import { useChangesets } from "@scm-manager/ui-api"; +import { Branch, Repository } from "@scm-manager/ui-types"; import { ChangesetList, ErrorNotification, LinkPaginator, Loading, Notification, - urls + urls, } from "@scm-manager/ui-components"; -import { useChangesets } from "@scm-manager/ui-api"; - -type Props = { - repository: Repository; - branch?: Branch; -}; - -type ChangesetProps = Props & { - error: Error | null; - isLoading: boolean; - data?: ChangesetCollection; -}; export const usePage = () => { const match = useRouteMatch(); return urls.getPageFromMatch(match); }; -const Changesets: FC<Props> = ({ repository, branch }) => { - const page = usePage(); - - const { isLoading, error, data } = useChangesets(repository, { branch, page: page - 1 }); - - return <ChangesetsPanel repository={repository} branch={branch} error={error} isLoading={isLoading} data={data} />; +type Props = { + repository: Repository; + branch?: Branch; + url: string; }; -export const ChangesetsPanel: FC<ChangesetProps> = ({ repository, error, isLoading, data }) => { - const [t] = useTranslation("repos"); +const Changesets: FC<Props> = ({ repository, branch, url }) => { const page = usePage(); + const { isLoading, error, data } = useChangesets(repository, { branch, page: page - 1 }); + const [t] = useTranslation("repos"); const changesets = data?._embedded?.changesets; if (error) { @@ -72,7 +60,11 @@ export const ChangesetsPanel: FC<ChangesetProps> = ({ repository, error, isLoadi return <Loading />; } - if (!changesets || changesets.length === 0) { + if (data && data.pageTotal < page && page > 1) { + return <Redirect to={`${urls.unescapeUrlForRoute(url)}/${data.pageTotal}`} />; + } + + if (!data || !changesets || changesets.length === 0) { return <Notification type="info">{t("changesets.noChangesets")}</Notification>; } @@ -82,7 +74,7 @@ export const ChangesetsPanel: FC<ChangesetProps> = ({ repository, error, isLoadi <ChangesetList repository={repository} changesets={changesets} /> </div> <div className="panel-footer"> - <LinkPaginator page={page} collection={data} /> + <LinkPaginator collection={data} page={page} /> </div> </div> ); diff --git a/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx b/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx index ae02fb92d1..dc9aecafeb 100644 --- a/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx +++ b/scm-ui/ui-webapp/src/repos/containers/ChangesetsRoot.tsx @@ -59,8 +59,7 @@ const ChangesetRoot: FC<Props> = ({ repository, baseUrl, branches, selectedBranc const onSelectBranch = (branch?: Branch) => { if (branch) { - const url = `${baseUrl}/branch/${encodeURIComponent(branch.name)}/changesets/`; - history.push(url); + history.push(`${baseUrl}/branch/${encodeURIComponent(branch.name)}/changesets/`); } else { history.push(`${baseUrl}/changesets/`); } @@ -75,7 +74,7 @@ const ChangesetRoot: FC<Props> = ({ repository, baseUrl, branches, selectedBranc switchViewLink={evaluateSwitchViewLink()} /> <Route path={`${url}/:page?`}> - <Changesets repository={repository} branch={branches?.filter(b => b.name === selectedBranch)[0]} /> + <Changesets repository={repository} branch={branches?.filter(b => b.name === selectedBranch)[0]} url={url} /> </Route> </> ); diff --git a/scm-ui/ui-webapp/src/search/Hits.tsx b/scm-ui/ui-webapp/src/search/Hits.tsx index e6cbb431b5..eff27eef23 100644 --- a/scm-ui/ui-webapp/src/search/Hits.tsx +++ b/scm-ui/ui-webapp/src/search/Hits.tsx @@ -34,7 +34,7 @@ import { ExtensionPoint, extensionPoints } from "@scm-manager/ui-extensions"; type Props = { type: string; - hits: HitType[]; + hits?: HitType[]; }; const hitComponents: { [name: string]: FC<HitProps> } = { @@ -69,12 +69,12 @@ const HitComponent: FC<HitComponentProps> = ({ hit, type }) => ( const NoHits: FC = () => { const [t] = useTranslation("commons"); - return <Notification>{t("search.noHits")}</Notification>; + return <Notification type="info">{t("search.noHits")}</Notification>; }; const Hits: FC<Props> = ({ type, hits }) => ( <div className="panel-block"> - {hits.length > 0 ? ( + {hits && hits.length > 0 ? ( hits.map((hit, c) => <HitComponent key={`${type}_${c}_${hit.score}`} hit={hit} type={type} />) ) : ( <NoHits /> diff --git a/scm-ui/ui-webapp/src/search/Results.tsx b/scm-ui/ui-webapp/src/search/Results.tsx index d40839b050..654dd4ee85 100644 --- a/scm-ui/ui-webapp/src/search/Results.tsx +++ b/scm-ui/ui-webapp/src/search/Results.tsx @@ -26,6 +26,7 @@ import React, { FC } from "react"; import { QueryResult } from "@scm-manager/ui-types"; import Hits from "./Hits"; import { LinkPaginator } from "@scm-manager/ui-components"; +import { Redirect, useLocation } from "react-router-dom"; type Props = { result: QueryResult; @@ -35,9 +36,21 @@ type Props = { }; const Results: FC<Props> = ({ result, type, page, query }) => { + const location = useLocation(); + const hits = result?._embedded?.hits; + + let pathname = location.pathname; + if (!pathname.endsWith("/")) { + pathname = pathname.substring(0, pathname.lastIndexOf("/") + 1); + } + + if (result && result.pageTotal < page && page > 1) { + return <Redirect to={`${pathname}${result.pageTotal}${location.search}`} />; + } + return ( <div className="panel"> - <Hits type={type} hits={result._embedded.hits} /> + <Hits type={type} hits={hits} /> <div className="panel-footer"> <LinkPaginator collection={result} page={page} filter={query} /> </div> diff --git a/scm-ui/ui-webapp/src/users/components/table/UserTable.tsx b/scm-ui/ui-webapp/src/users/components/table/UserTable.tsx index 586c01dfda..8240982499 100644 --- a/scm-ui/ui-webapp/src/users/components/table/UserTable.tsx +++ b/scm-ui/ui-webapp/src/users/components/table/UserTable.tsx @@ -21,35 +21,34 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ -import React from "react"; -import { WithTranslation, withTranslation } from "react-i18next"; +import React, { FC } from "react"; +import { useTranslation } from "react-i18next"; import { User } from "@scm-manager/ui-types"; import UserRow from "./UserRow"; -type Props = WithTranslation & { +type Props = { users: User[]; }; -class UserTable extends React.Component<Props> { - render() { - const { users, t } = this.props; - return ( - <table className="card-table table is-hoverable is-fullwidth"> - <thead> - <tr> - <th>{t("user.name")}</th> - <th className="is-hidden-mobile">{t("user.displayName")}</th> - <th className="is-hidden-mobile">{t("user.mail")}</th> - </tr> - </thead> - <tbody> - {users.map((user, index) => { - return <UserRow key={index} user={user} />; - })} - </tbody> - </table> - ); - } -} +const UserTable: FC<Props> = ({ users }) => { + const [t] = useTranslation("users"); -export default withTranslation("users")(UserTable); + return ( + <table className="card-table table is-hoverable is-fullwidth"> + <thead> + <tr> + <th>{t("user.name")}</th> + <th className="is-hidden-mobile">{t("user.displayName")}</th> + <th className="is-hidden-mobile">{t("user.mail")}</th> + </tr> + </thead> + <tbody> + {users.map((user, index) => { + return <UserRow key={index} user={user} />; + })} + </tbody> + </table> + ); +}; + +export default UserTable; diff --git a/scm-ui/ui-webapp/src/users/containers/Users.tsx b/scm-ui/ui-webapp/src/users/containers/Users.tsx index bdbb5e8875..e1e69ccd86 100644 --- a/scm-ui/ui-webapp/src/users/containers/Users.tsx +++ b/scm-ui/ui-webapp/src/users/containers/Users.tsx @@ -22,8 +22,10 @@ * SOFTWARE. */ import React, { FC } from "react"; +import { Redirect, useLocation, useParams } from "react-router-dom"; import { useTranslation } from "react-i18next"; -import { useLocation, useParams } from "react-router-dom"; +import { useUsers } from "@scm-manager/ui-api"; +import { User, UserCollection } from "@scm-manager/ui-types"; import { CreateButton, LinkPaginator, @@ -31,10 +33,31 @@ import { OverviewPageActions, Page, PageActions, - urls + urls, } from "@scm-manager/ui-components"; import { UserTable } from "./../components/table"; -import { useUsers } from "@scm-manager/ui-api"; + +type UserPageProps = { + data?: UserCollection; + users?: User[]; + page: number; + search?: string; +}; + +const UserPage: FC<UserPageProps> = ({ data, users, page, search }) => { + const [t] = useTranslation("users"); + + if (!data || !users || users.length === 0) { + return <Notification type="info">{t("users.noUsers")}</Notification>; + } + + return ( + <> + <UserTable users={users} /> + <LinkPaginator collection={data} page={page} filter={search} /> + </> + ); +}; const Users: FC = () => { const location = useLocation(); @@ -42,28 +65,13 @@ const Users: FC = () => { const search = urls.getQueryStringFromLocation(location); const page = urls.getPageFromMatch({ params }); const { isLoading, error, data } = useUsers({ page: page - 1, search }); - const users = data?._embedded.users; const [t] = useTranslation("users"); + const users = data?._embedded?.users; const canAddUsers = !!data?._links.create; - const renderUserTable = () => { - if (data && users && users.length > 0) { - return ( - <> - <UserTable users={users} /> - <LinkPaginator collection={data} page={page} filter={urls.getQueryStringFromLocation(location)} /> - </> - ); - } - return <Notification type="info">{t("users.noUsers")}</Notification>; - }; - - const renderCreateButton = () => { - if (canAddUsers) { - return <CreateButton link="/users/create" label={t("users.createButton")} />; - } - return null; - }; + if (data && data.pageTotal < page && page > 1) { + return <Redirect to={`/users/${data.pageTotal}`} />; + } return ( <Page @@ -72,8 +80,8 @@ const Users: FC = () => { loading={isLoading || !users} error={error || undefined} > - {renderUserTable()} - {renderCreateButton()} + <UserPage data={data} users={users} page={page} search={search} /> + {canAddUsers ? <CreateButton link="/users/create" label={t("users.createButton")} /> : null} <PageActions> <OverviewPageActions showCreateButton={canAddUsers}