Feat/page reordering endpoints #108
Conversation
This commit adds a new `listAll` method to the File class. This method returns data of both type `file` as well as type `dir` from GitHub. Previously, the `list` method only returned data of type `file`. With the new method, we have information on both files and directories, which we need to render the nested sub-folders. Since we already have a separate API for retrieving individual file content, we need to limit the scope of this method - we should only retrieve content from directories. If the provided endpoint is an individual file instead of a directory, then we will throw a BadRequestError.
…r subfolder This commit adds the GET /v1/sites/:siteName/folders endpoint which retrieves the files and directories within the provided path directory. This endpoint will fail if the provided path is not a valid directory (for example, if you give it the path for an individual file, or if the provided path does not exist in the repo) and will respond with the appropriate error. The path is passed as a route query param `path` since the file/directory path contains the `/` character, which prevents it from being used as a route param.
previously, we would throw a NotFoundError if the path was invalid, but this sends the wrong signal to the frontend, which displays the fallback "Not Found" component. A BadRequestError more accurately reflects the situation
There was a problem hiding this comment.
As discussed, currently the distinction between files and folders is not conceptually clear on our backend. We currently use the File class to retrieve and list files (in Collection, Resource etc), but we should move that functionality to the separate Directory class.
As a first step, we can move this listAll functionality to the Directory class, and do the refactoring for collections and images at a later stage.
routes/folders.js
Outdated
|
|
||
| // Validation of path | ||
| const pathArr = path.split('/') | ||
| if (path.slice(0,1) !== '_' || pathArr.length > 2 || (pathArr.length === 2 && pathArr[1].includes('.'))) { |
There was a problem hiding this comment.
This validation maybe can be left to a "higher level"
There was a problem hiding this comment.
Validation has been shifted to the list method for the Directory class so that it is performed only conditionally (when the dirType is FolderType) - see commit 3619f17
Alexis pointed out that the `list` method for the Directory class was actually very similar to the newly-created `listAll` method for the File class, and given that we are trying to retrieve the contents of a folder on GitHub, it is conceptually clearer to use the Directory class for that purpose. This commit: - removes the `listAll` method from the File class - modifies the Directory class to accept a new FolderType class as a directory type - modifies the behavior of the `list` method for the Directory class so that it performs the appropriate validation for Directory instances which are of type FolderType
routes/folders.js
Outdated
| async function listFolderContent (req, res, next) { | ||
| const { accessToken } = req | ||
| const { siteName } = req.params | ||
| const { path } = req.query // paths begin with _ |
There was a problem hiding this comment.
nit: from our previous discussions, we're using this route to access both collections and nested folders (can also be generalised to other image, resource folders?), so the comment here // paths begin with _ is misleading
There was a problem hiding this comment.
fixed with c6f99d1
Overview
This PR adds a new endpoint,
GET /v1/sites/:siteName/folders, which returns all files and directories contained within a collection/folder or third-nav section/sub-folder. Previously, we had only returned data of typefile- with this change, we will return data of both typesfileanddir(refer to the GitHub contents API for sample responses - link). We do so by creating a newFolderTypeclass and expanding theDirectoryclass to acceptFolderTypeas a directory type.Note that this endpoint is intended for use with both top-level collection folders as well as nested sub-folders, since we have determined that we will need to accept and deal with nested sub-folders going forward (for images and files).
Rationale for using query params instead of path params
Since this endpoint is designed to be used with both top-level folders and nested folders, we cannot use path params because any nested folder will contain the
/character, which messes with Express' path routing mechanism. Therefore, to avoid routing confusion, we use query params here instead.Note: since our research has shown that users prefer the term
foldertocollection, andsub-foldertothird nav, we will be using these new terms more frequently going forward. As an interim, we can use the terms together (likecollection/folder) as I have done so above to facilitate the transition.