diff --git a/app/components/DocumentBreadcrumb.js b/app/components/DocumentBreadcrumb.js index 5f3fdebba..166c5b9b8 100644 --- a/app/components/DocumentBreadcrumb.js +++ b/app/components/DocumentBreadcrumb.js @@ -67,6 +67,7 @@ const DocumentBreadcrumb = ({ document, children, onlyText }: Props) => { id: document.collectionId, name: t("Deleted Collection"), color: "currentColor", + url: "deleted-collection", }; } @@ -89,7 +90,7 @@ const DocumentBreadcrumb = ({ document, children, onlyText }: Props) => { output.push({ icon: , title: collection.name, - to: collectionUrl(collection.id), + to: collectionUrl(collection.url), }); } diff --git a/app/models/Collection.js b/app/models/Collection.js index ce8f4a66c..c9b73b698 100644 --- a/app/models/Collection.js +++ b/app/models/Collection.js @@ -24,6 +24,7 @@ export default class Collection extends BaseModel { deletedAt: ?string; sort: { field: string, direction: "asc" | "desc" }; url: string; + urlId: string; @computed get isEmpty(): boolean { diff --git a/app/routes/authenticated.js b/app/routes/authenticated.js index 5bc7caa1f..aead763a4 100644 --- a/app/routes/authenticated.js +++ b/app/routes/authenticated.js @@ -51,9 +51,10 @@ export default function AuthenticatedRoutes() { - - - + + + + { + if (collection) { + const canonicalUrl = updateCollectionUrl(match.url, collection); + if (match.url !== canonicalUrl) { + history.replace(canonicalUrl); + } + } + }, [collection, history, id, match.url]); React.useEffect(() => { if (collection) { @@ -59,8 +81,10 @@ function CollectionScene() { React.useEffect(() => { setError(null); - documents.fetchPinned({ collectionId }); - }, [documents, collectionId]); + if (collection) { + documents.fetchPinned({ collectionId: collection.id }); + } + }, [documents, collection]); React.useEffect(() => { async function load() { @@ -68,7 +92,7 @@ function CollectionScene() { try { setError(null); setFetching(true); - await collections.fetch(collectionId); + await collections.fetch(id); } catch (err) { setError(err); } finally { @@ -77,7 +101,7 @@ function CollectionScene() { } } load(); - }, [collections, isFetching, collection, error, collectionId, can]); + }, [collections, isFetching, collection, error, id, can]); useUnmount(ui.clearActiveCollection); @@ -124,7 +148,7 @@ function CollectionScene() { source="collection" placeholder={`${t("Search in collection")}…`} label={`${t("Search in collection")}…`} - collectionId={collectionId} + collectionId={collection.id} /> {can.update && ( @@ -257,27 +281,27 @@ function CollectionScene() { )} - + {t("Documents")} - + {t("Recently updated")} - + {t("Recently published")} - + {t("Least recently updated")} {t("A–Z")} - + - + - + - + - + - + { const isMove = this.props.location.pathname.match(/move$/); const canRedirect = !revisionId && !isMove && !shareId; if (canRedirect) { - const canonicalUrl = updateDocumentUrl( - this.props.match.url, - document.url - ); + const canonicalUrl = updateDocumentUrl(this.props.match.url, document); if (this.props.location.pathname !== canonicalUrl) { this.props.history.replace(canonicalUrl); } diff --git a/app/scenes/Document/components/Document.js b/app/scenes/Document/components/Document.js index c5b2c0bf8..d96eef776 100644 --- a/app/scenes/Document/components/Document.js +++ b/app/scenes/Document/components/Document.js @@ -36,7 +36,6 @@ import { isCustomDomain } from "utils/domains"; import { emojiToUrl } from "utils/emoji"; import { meta } from "utils/keyboard"; import { - collectionUrl, documentMoveUrl, documentHistoryUrl, editDocumentUrl, @@ -291,15 +290,7 @@ class DocumentScene extends React.Component { }; goBack = () => { - let url; - if (this.props.document.url) { - url = this.props.document.url; - } else if (this.props.match.params.id) { - url = collectionUrl(this.props.match.params.id); - } - if (url) { - this.props.history.push(url); - } + this.props.history.push(this.props.document.url); }; render() { diff --git a/app/scenes/DocumentDelete.js b/app/scenes/DocumentDelete.js index c39198993..1b1ba0328 100644 --- a/app/scenes/DocumentDelete.js +++ b/app/scenes/DocumentDelete.js @@ -17,12 +17,13 @@ type Props = { function DocumentDelete({ document, onSubmit }: Props) { const { t } = useTranslation(); - const { ui, documents } = useStores(); + const { ui, documents, collections } = useStores(); const history = useHistory(); const [isDeleting, setDeleting] = React.useState(false); const [isArchiving, setArchiving] = React.useState(false); const { showToast } = ui; const canArchive = !document.isDraft && !document.isArchived; + const collection = collections.get(document.collectionId); const handleSubmit = React.useCallback( async (ev: SyntheticEvent<>) => { @@ -45,7 +46,7 @@ function DocumentDelete({ document, onSubmit }: Props) { } // otherwise, redirect to the collection home - history.push(collectionUrl(document.collectionId)); + history.push(collectionUrl(collection?.url || "/")); } onSubmit(); } catch (err) { @@ -54,7 +55,7 @@ function DocumentDelete({ document, onSubmit }: Props) { setDeleting(false); } }, - [showToast, onSubmit, ui, document, documents, history] + [showToast, onSubmit, ui, document, documents, history, collection] ); const handleArchive = React.useCallback( diff --git a/app/scenes/DocumentNew.js b/app/scenes/DocumentNew.js index 75645ba92..bceca9e16 100644 --- a/app/scenes/DocumentNew.js +++ b/app/scenes/DocumentNew.js @@ -1,58 +1,57 @@ // @flow -import { inject } from "mobx-react"; +import { observer } from "mobx-react"; import queryString from "query-string"; import * as React from "react"; -import { - type RouterHistory, - type Location, - type Match, -} from "react-router-dom"; -import DocumentsStore from "stores/DocumentsStore"; -import UiStore from "stores/UiStore"; +import { useEffect } from "react"; +import { useTranslation } from "react-i18next"; +import { useHistory, useLocation, useRouteMatch } from "react-router-dom"; import CenteredContent from "components/CenteredContent"; import Flex from "components/Flex"; import LoadingPlaceholder from "components/LoadingPlaceholder"; +import useStores from "hooks/useStores"; import { editDocumentUrl } from "utils/routeHelpers"; -type Props = { - history: RouterHistory, - location: Location, - documents: DocumentsStore, - ui: UiStore, - match: Match, -}; +function DocumentNew() { + const history = useHistory(); + const location = useLocation(); + const match = useRouteMatch(); + const { t } = useTranslation(); + const { documents, ui, collections } = useStores(); + const id = match.params.id || ""; -class DocumentNew extends React.Component { - async componentDidMount() { - const params = queryString.parse(this.props.location.search); + useEffect(() => { + async function createDocument() { + const params = queryString.parse(location.search); + try { + const collection = await collections.fetch(id); - try { - const document = await this.props.documents.create({ - collectionId: this.props.match.params.id, - parentDocumentId: params.parentDocumentId, - templateId: params.templateId, - template: params.template, - title: "", - text: "", - }); - this.props.history.replace(editDocumentUrl(document)); - } catch (err) { - this.props.ui.showToast("Couldn’t create the document, try again?", { - type: "error", - }); - this.props.history.goBack(); + const document = await documents.create({ + collectionId: collection.id, + parentDocumentId: params.parentDocumentId, + templateId: params.templateId, + template: params.template, + title: "", + text: "", + }); + + history.replace(editDocumentUrl(document)); + } catch (err) { + ui.showToast(t("Couldn’t create the document, try again?"), { + type: "error", + }); + history.goBack(); + } } - } + createDocument(); + }); - render() { - return ( - - - - - - ); - } + return ( + + + + + + ); } -export default inject("documents", "ui")(DocumentNew); +export default observer(DocumentNew); diff --git a/app/stores/BaseStore.js b/app/stores/BaseStore.js index 58dc071f5..a5e072cfa 100644 --- a/app/stores/BaseStore.js +++ b/app/stores/BaseStore.js @@ -139,7 +139,8 @@ export default class BaseStore { throw new Error(`Cannot fetch ${this.modelName}`); } - let item = this.data.get(id); + const item = this.data.get(id); + if (item && !options.force) return item; this.isFetching = true; diff --git a/app/stores/CollectionsStore.js b/app/stores/CollectionsStore.js index 11f200da5..1beb29db9 100644 --- a/app/stores/CollectionsStore.js +++ b/app/stores/CollectionsStore.js @@ -1,6 +1,6 @@ // @flow import invariant from "invariant"; -import { concat, last } from "lodash"; +import { concat, find, last } from "lodash"; import { computed, action } from "mobx"; import Collection from "models/Collection"; import BaseStore from "./BaseStore"; @@ -126,6 +126,30 @@ export default class CollectionsStore extends BaseStore { return result; } + @action + async fetch(id: string, options: Object = {}): Promise<*> { + const item = this.get(id) || this.getByUrl(id); + + if (item && !options.force) return item; + + this.isFetching = true; + + try { + const res = await client.post(`/collections.info`, { id }); + invariant(res && res.data, "Collection not available"); + + this.addPolicies(res.policies); + return this.add(res.data); + } catch (err) { + if (err.statusCode === 403) { + this.remove(id); + } + throw err; + } finally { + this.isFetching = false; + } + } + getPathForDocument(documentId: string): ?DocumentPath { return this.pathsToDocuments.find((path) => path.id === documentId); } @@ -135,6 +159,10 @@ export default class CollectionsStore extends BaseStore { if (path) return path.title; } + getByUrl(url: string): ?Collection { + return find(this.orderedData, (col: Collection) => url.endsWith(col.urlId)); + } + delete = async (collection: Collection) => { await super.delete(collection); diff --git a/app/utils/routeHelpers.js b/app/utils/routeHelpers.js index 85214ea16..f31557d3a 100644 --- a/app/utils/routeHelpers.js +++ b/app/utils/routeHelpers.js @@ -1,5 +1,6 @@ // @flow import queryString from "query-string"; +import Collection from "models/Collection"; import Document from "models/Document"; export function homeUrl(): string { @@ -11,13 +12,23 @@ export function starredUrl(): string { } export function newCollectionUrl(): string { - return "/collections/new"; + return "/collection/new"; } -export function collectionUrl(collectionId: string, section: ?string): string { - const path = `/collections/${collectionId}`; - if (section) return `${path}/${section}`; - return path; +export function collectionUrl(url: string, section: ?string): string { + if (section) return `${url}/${section}`; + return url; +} + +export function updateCollectionUrl( + oldUrl: string, + collection: Collection +): string { + // Update url to match the current one + return oldUrl.replace( + new RegExp("/collection/[0-9a-zA-Z-_~]*"), + collection.url + ); } export function documentUrl(doc: Document): string { @@ -42,14 +53,9 @@ export function documentHistoryUrl(doc: Document, revisionId?: string): string { * Replace full url's document part with the new one in case * the document slug has been updated */ -export function updateDocumentUrl(oldUrl: string, newUrl: string): string { +export function updateDocumentUrl(oldUrl: string, document: Document): string { // Update url to match the current one - const urlParts = oldUrl.trim().split("/"); - const actions = urlParts.slice(3); - if (actions[0]) { - return [newUrl, actions].join("/"); - } - return newUrl; + return oldUrl.replace(new RegExp("/doc/[0-9a-zA-Z-_~]*"), document.url); } export function newDocumentUrl( @@ -60,7 +66,7 @@ export function newDocumentUrl( template?: boolean, } ): string { - return `/collections/${collectionId}/new?${queryString.stringify(params)}`; + return `/collection/${collectionId}/new?${queryString.stringify(params)}`; } export function searchUrl( diff --git a/server/api/collections.js b/server/api/collections.js index a2657596d..70eec6d19 100644 --- a/server/api/collections.js +++ b/server/api/collections.js @@ -115,7 +115,7 @@ router.post("collections.create", auth(), async (ctx) => { router.post("collections.info", auth(), async (ctx) => { const { id } = ctx.body; - ctx.assertUuid(id, "id is required"); + ctx.assertPresent(id, "id is required"); const user = ctx.state.user; const collection = await Collection.scope({ diff --git a/server/models/Collection.js b/server/models/Collection.js index 62ab9f880..ea8d4a214 100644 --- a/server/models/Collection.js +++ b/server/models/Collection.js @@ -1,13 +1,13 @@ // @flow import { find, findIndex, concat, remove, uniq } from "lodash"; import randomstring from "randomstring"; -import slug from "slug"; +import isUUID from "validator/lib/isUUID"; +import { SLUG_URL_REGEX } from "../../shared/utils/routeHelpers"; import { Op, DataTypes, sequelize } from "../sequelize"; +import slugify from "../utils/slugify"; import CollectionUser from "./CollectionUser"; import Document from "./Document"; -slug.defaults.mode = "rfc3986"; - const Collection = sequelize.define( "collection", { @@ -72,7 +72,9 @@ const Collection = sequelize.define( }, getterMethods: { url() { - return `/collections/${this.id}`; + if (!this.name) return `/collection/untitled-${this.urlId}`; + + return `/collection/${slugify(this.name)}-${this.urlId}`; }, }, } @@ -223,6 +225,17 @@ Collection.addHook("afterCreate", (model: Collection, options) => { // Class methods +Collection.findByPk = async function (id, options = {}) { + if (isUUID(id)) { + return this.findOne({ where: { id }, ...options }); + } else if (id.match(SLUG_URL_REGEX)) { + return this.findOne({ + where: { urlId: id.match(SLUG_URL_REGEX)[1] }, + ...options, + }); + } +}; + // get all the membership relationshps a user could have with the collection Collection.membershipUserIds = async (collectionId: string) => { const collection = await Collection.scope("withAllMemberships").findByPk( diff --git a/server/models/Collection.test.js b/server/models/Collection.test.js index 3858d799c..7027bff4c 100644 --- a/server/models/Collection.test.js +++ b/server/models/Collection.test.js @@ -1,4 +1,5 @@ /* eslint-disable flowtype/require-valid-file-annotation */ +import randomstring from "randomstring"; import { v4 as uuidv4 } from "uuid"; import { Collection, Document } from "../models"; import { @@ -9,6 +10,7 @@ import { buildDocument, } from "../test/factories"; import { flushdb, seed } from "../test/support"; +import slugify from "../utils/slugify"; beforeEach(() => flushdb()); beforeEach(jest.resetAllMocks); @@ -16,7 +18,7 @@ beforeEach(jest.resetAllMocks); describe("#url", () => { test("should return correct url for the collection", () => { const collection = new Collection({ id: "1234" }); - expect(collection.url).toBe("/collections/1234"); + expect(collection.url).toBe(`/collection/untitled-${collection.urlId}`); }); }); @@ -416,3 +418,53 @@ describe("#membershipUserIds", () => { expect(membershipUserIds.length).toBe(6); }); }); + +describe("#findByPk", () => { + test("should return collection with collection Id", async () => { + const collection = await buildCollection(); + const response = await Collection.findByPk(collection.id); + + expect(response.id).toBe(collection.id); + }); + + test("should return collection when urlId is present", async () => { + const collection = await buildCollection(); + const id = `${slugify(collection.name)}-${collection.urlId}`; + + const response = await Collection.findByPk(id); + + expect(response.id).toBe(collection.id); + }); + + test("should return undefined when incorrect uuid type", async () => { + const collection = await buildCollection(); + const response = await Collection.findByPk(collection.id + "-incorrect"); + + expect(response).toBe(undefined); + }); + + test("should return undefined when incorrect urlId length", async () => { + const collection = await buildCollection(); + const id = `${slugify(collection.name)}-${collection.urlId}incorrect`; + + const response = await Collection.findByPk(id); + + expect(response).toBe(undefined); + }); + + test("should return null when no collection is found with uuid", async () => { + const response = await Collection.findByPk( + "a9e71a81-7342-4ea3-9889-9b9cc8f667da" + ); + + expect(response).toBe(null); + }); + + test("should return null when no collection is found with urlId", async () => { + const id = `${slugify("test collection")}-${randomstring.generate(15)}`; + + const response = await Collection.findByPk(id); + + expect(response).toBe(null); + }); +}); diff --git a/server/models/Document.js b/server/models/Document.js index daba170b9..269a2ee96 100644 --- a/server/models/Document.js +++ b/server/models/Document.js @@ -7,6 +7,7 @@ import MarkdownSerializer from "slate-md-serializer"; import isUUID from "validator/lib/isUUID"; import { MAX_TITLE_LENGTH } from "../../shared/constants"; import parseTitle from "../../shared/utils/parseTitle"; +import { SLUG_URL_REGEX } from "../../shared/utils/routeHelpers"; import unescape from "../../shared/utils/unescape"; import { Collection, User } from "../models"; import { DataTypes, sequelize } from "../sequelize"; @@ -14,7 +15,6 @@ import slugify from "../utils/slugify"; import Revision from "./Revision"; const Op = Sequelize.Op; -const URL_REGEX = /^[0-9a-zA-Z-_~]*-([a-zA-Z0-9]{10,15})$/; const serializer = new MarkdownSerializer(); export const DOCUMENT_VERSION = 2; @@ -216,10 +216,10 @@ Document.findByPk = async function (id, options = {}) { where: { id }, ...options, }); - } else if (id.match(URL_REGEX)) { + } else if (id.match(SLUG_URL_REGEX)) { return scope.findOne({ where: { - urlId: id.match(URL_REGEX)[1], + urlId: id.match(SLUG_URL_REGEX)[1], }, ...options, }); diff --git a/server/models/Document.test.js b/server/models/Document.test.js index c57fee545..891107c90 100644 --- a/server/models/Document.test.js +++ b/server/models/Document.test.js @@ -6,7 +6,8 @@ import { buildTeam, buildUser, } from "../test/factories"; -import { flushdb } from "../test/support"; +import { flushdb, seed } from "../test/support"; +import slugify from "../utils/slugify"; beforeEach(() => flushdb()); beforeEach(jest.resetAllMocks); @@ -307,3 +308,14 @@ describe("#delete", () => { expect(document.deletedAt).toBeTruthy(); }); }); + +describe("#findByPk", () => { + test("should return document when urlId is correct", async () => { + const { document } = await seed(); + const id = `${slugify(document.title)}-${document.urlId}`; + + const response = await Document.findByPk(id); + + expect(response.id).toBe(document.id); + }); +}); diff --git a/server/presenters/collection.js b/server/presenters/collection.js index 7a5737d34..6ae94e2d8 100644 --- a/server/presenters/collection.js +++ b/server/presenters/collection.js @@ -24,6 +24,7 @@ export default function present(collection: Collection) { const data = { id: collection.id, url: collection.url, + urlId: collection.urlId, name: collection.name, description: collection.description, sort: collection.sort, diff --git a/shared/i18n/locales/en_US/translation.json b/shared/i18n/locales/en_US/translation.json index 65b469d65..9d959e57d 100644 --- a/shared/i18n/locales/en_US/translation.json +++ b/shared/i18n/locales/en_US/translation.json @@ -309,6 +309,7 @@ "Deleting": "Deleting", "I’m sure – Delete": "I’m sure – Delete", "Archiving": "Archiving", + "Couldn’t create the document, try again?": "Couldn’t create the document, try again?", "Search documents": "Search documents", "No documents found for your filters.": "No documents found for your filters.", "You’ve not got any drafts at the moment.": "You’ve not got any drafts at the moment.", diff --git a/shared/utils/routeHelpers.js b/shared/utils/routeHelpers.js index 8bf94623f..dcf4122a8 100644 --- a/shared/utils/routeHelpers.js +++ b/shared/utils/routeHelpers.js @@ -61,3 +61,5 @@ export function settings(): string { export function groupSettings(): string { return `/settings/groups`; } + +export const SLUG_URL_REGEX = /^[0-9a-zA-Z-_~]*-([a-zA-Z0-9]{10,15})$/;