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.

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

Fix: Display custom error page when folder is missing in course materials #7612

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion app/controllers/course/material/materials_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
# frozen_string_literal: true
class Course::Material::MaterialsController < Course::Material::Controller
load_and_authorize_resource :material, through: :folder, class: 'Course::Material'
load_and_authorize_resource :material, through: :folder, class: 'Course::Material', except: :load_default

def load_default
Copy link
Contributor

Choose a reason for hiding this comment

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

You're loading the root folder, not a file, so this method does not belong in Course::Material::MaterialsController. It should live in Course::Material::FoldersController instead.

You will need to skip_load_and_authorize_resource :folder for the method you create in Course::Material::FoldersController

The perk of doing that is that you can simply set the @folder and then fallback to the default show Folder method, which will do its own authorization checks.

This will also align json responses (all folder responses return the same json schema)

@folder = Course::Material::Folder.where(course_id: current_course.id, parent_id: nil).order(:created_at).first
Copy link
Contributor

Choose a reason for hiding this comment

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

Use existing Course.root_folder method @folder = current_course.root_folder instead of re-writing verbose logic.

if @folder
render json: @folder, status: :ok

Check warning on line 8 in app/controllers/course/material/materials_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/material/materials_controller.rb#L6-L8

Added lines #L6 - L8 were not covered by tests
else
render json: { error: 'No folders available' }, status: :not_found

Check warning on line 10 in app/controllers/course/material/materials_controller.rb

View check run for this annotation

Codecov / codecov/patch

app/controllers/course/material/materials_controller.rb#L10

Added line #L10 was not covered by tests
end
end

def show
authorize!(:read_owner, @material.folder)
Expand Down
12 changes: 11 additions & 1 deletion client/app/api/course/Materials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { FileListData } from 'types/course/material/files';

import { APIResponse } from 'api/types';

import { FolderMiniEntity } from '../../types/course/material/folders';

import BaseCourseAPI from './Base';

const getShouldDownloadFromContentDisposition = (
Expand All @@ -15,8 +17,12 @@ const getShouldDownloadFromContentDisposition = (
};

export default class MaterialsAPI extends BaseCourseAPI {
get #materialPrefix(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, you shouldn't be introducing folder logic into MaterialsAPI, it should go into client/app/api/course/MaterialFolders.js instead.

At the same time, good to rewrite the file into TypeScript.

return `/courses/${this.courseId}/materials`;
}

get #urlPrefix(): string {
return `/courses/${this.courseId}/materials/folders`;
return `${this.#materialPrefix}/folders`;
}

fetch(folderId: number, materialId: number): APIResponse<FileListData> {
Expand All @@ -25,6 +31,10 @@ export default class MaterialsAPI extends BaseCourseAPI {
);
}

fetchDefault(): APIResponse<FolderMiniEntity> {
return this.client.get(`${this.#materialPrefix}/load_default`);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also use urlPrefix to fetch the default folder, e.g. /materials/folders, your routes.rb will have to update accordingly

}

/**
* Attempts to download the file at the given `url` as a `Blob` and returns
* its URL and disposition. Remember to `revoke` the URL when no longer needed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ import { Typography } from '@mui/material';

import Page from 'lib/components/core/layouts/Page';

interface BaseDownloadFilePageProps {
interface BaseRetrieveMaterialPageProps {
illustration: ReactNode;
title: string;
description: string;
children?: ReactNode;
}

const BaseDownloadFilePage = (
props: BaseDownloadFilePageProps,
const BaseRetrieveMaterialPage = (
props: BaseRetrieveMaterialPageProps,
): JSX.Element => (
<Page className="h-full m-auto flex flex-col items-center justify-center text-center">
{props.illustration}
Expand All @@ -32,4 +32,4 @@ const BaseDownloadFilePage = (
</Page>
);

export default BaseDownloadFilePage;
export default BaseRetrieveMaterialPage;
10 changes: 5 additions & 5 deletions client/app/bundles/course/material/files/DownloadingFilePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import Link from 'lib/components/core/Link';
import useEffectOnce from 'lib/hooks/useEffectOnce';
import useTranslation from 'lib/hooks/useTranslation';

import BaseDownloadFilePage from './components/BaseDownloadFilePage';
import BaseRetrieveMaterialPage from '../component/BaseRetrieveMaterialPage';

const DEFAULT_FILE_NAME = 'file';

Expand Down Expand Up @@ -51,7 +51,7 @@ const SuccessDownloadingFilePage = (
const { t } = useTranslation();

return (
<BaseDownloadFilePage
<BaseRetrieveMaterialPage
description={t(translations.downloadingDescription)}
illustration={
<DownloadingOutlined className="text-[6rem]" color="success" />
Expand All @@ -61,7 +61,7 @@ const SuccessDownloadingFilePage = (
<Link className="mt-10" href={props.url}>
{t(translations.tryDownloadingAgain)}
</Link>
</BaseDownloadFilePage>
</BaseRetrieveMaterialPage>
);
};

Expand All @@ -71,7 +71,7 @@ const ErrorStartingDownloadFilePage = (
const { t } = useTranslation();

return (
<BaseDownloadFilePage
<BaseRetrieveMaterialPage
description={t(translations.clickToDownloadFileDescription)}
illustration={
<div className="relative">
Expand All @@ -93,7 +93,7 @@ const ErrorStartingDownloadFilePage = (
>
{props.name}
</Button>
</BaseDownloadFilePage>
</BaseRetrieveMaterialPage>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Cancel, InsertDriveFileOutlined } from '@mui/icons-material';
import Link from 'lib/components/core/Link';
import useTranslation from 'lib/hooks/useTranslation';

import BaseDownloadFilePage from './components/BaseDownloadFilePage';
import BaseRetrieveMaterialPage from '../component/BaseRetrieveMaterialPage';

const translations = defineMessages({
problemRetrievingFile: {
Expand All @@ -30,7 +30,7 @@ const ErrorRetrievingFilePage = (): JSX.Element => {
const workbinURL = `/courses/${params.courseId}/materials/folders/${params.folderId}`;

return (
<BaseDownloadFilePage
<BaseRetrieveMaterialPage
description={t(translations.problemRetrievingFileDescription)}
illustration={
<div className="relative">
Expand All @@ -47,7 +47,7 @@ const ErrorRetrievingFilePage = (): JSX.Element => {
<Link className="mt-10" to={workbinURL}>
{t(translations.goToTheWorkbin)}
phungmanhcuong marked this conversation as resolved.
Show resolved Hide resolved
</Link>
</BaseDownloadFilePage>
</BaseRetrieveMaterialPage>
);
};

Expand Down
15 changes: 15 additions & 0 deletions client/app/bundles/course/material/folderLoader.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { LoaderFunction, redirect } from 'react-router-dom';
import { getIdFromUnknown } from 'utilities';

import CourseAPI from 'api/course';

const folderLoader: LoaderFunction = async ({ params }) => {
const folderId = getIdFromUnknown(params?.folderId);
if (!folderId) return redirect('/');
phungmanhcuong marked this conversation as resolved.
Show resolved Hide resolved

const { data } = await CourseAPI.folders.fetch(folderId);

return data;
cysjonathan marked this conversation as resolved.
Show resolved Hide resolved
};

export default folderLoader;
24 changes: 19 additions & 5 deletions client/app/bundles/course/material/folders/handles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,25 @@ import { getIdFromUnknown } from 'utilities';
import CourseAPI from 'api/course';
import { CrumbPath, DataHandle } from 'lib/hooks/router/dynamicNest';

export const loadDefaultMaterialId = async (): Promise<number> => {
const {
data: { id },
} = await CourseAPI.materials.fetchDefault();
return id;
};

const getFolderTitle = async (
courseUrl: string,
courseId: string,
folderId: number,
): Promise<CrumbPath> => {
const { data } = await CourseAPI.folders.fetch(folderId);
const courseUrl = `/courses/${courseId}`;
let data;
try {
({ data } = await CourseAPI.folders.fetch(folderId));
} catch (error) {
const defaultMaterialId = await loadDefaultMaterialId();
({ data } = await CourseAPI.folders.fetch(defaultMaterialId));
Comment on lines +22 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

You should align your backend API so you don't need to make 3 API calls just to get the root folder (2 calls are sufficient).

  1. fetch invalid folder payload (throws error)
  2. fetch valid root folderId
  3. fetch valid root folder payload

}

const workbinUrl = `${courseUrl}/materials/folders/${data.breadcrumbs[0].id}`;

Expand All @@ -32,13 +46,13 @@ const getFolderTitle = async (
* e.g., `useDynamicNest` cannot know if we move out from Folder 2 to Folder 1 from the URL.
*/
export const folderHandle: DataHandle = (match) => {
const courseId = match.params?.courseId;
const folderId = getIdFromUnknown(match.params?.folderId);
if (!courseId) throw new Error(`Invalid course id: ${courseId}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant check.

Your code will never throw this error unless courseId is an empty string (You should have used getIdFromUnknown on the courseId).

In any case, folderId will be invalid when the courseId is invalid.

if (!folderId) throw new Error(`Invalid folder id: ${folderId}`);

const courseUrl = `/courses/${match.params.courseId}`;

return {
shouldRevalidate: true,
getData: () => getFolderTitle(courseUrl, folderId),
getData: () => getFolderTitle(courseId, folderId),
};
};
29 changes: 15 additions & 14 deletions client/app/bundles/course/material/folders/operations.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Operation } from 'store';
import {
FolderData,
FolderFormData,
MaterialFormData,
MaterialUploadFormData,
Expand Down Expand Up @@ -62,20 +63,20 @@ const formatMaterialAttributes = (data: MaterialFormData): FormData => {
return payload;
};

export function loadFolder(folderId: number): Operation<SaveFolderAction> {
return async (dispatch) =>
CourseAPI.folders.fetch(folderId).then((response) => {
const data = response.data;
return dispatch(
actions.saveFolder(
data.currFolderInfo,
data.subfolders,
data.materials,
data.advanceStartAt,
data.permissions,
),
);
});
export function dispatchFolderData(
data: FolderData,
): Operation<SaveFolderAction> {
return async (dispatch) => {
return dispatch(
actions.saveFolder(
data.currFolderInfo,
data.subfolders,
data.materials,
data.advanceStartAt,
data.permissions,
),
);
};
Comment on lines +66 to +79
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really a need to modify this?

}

export function createFolder(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import React, { useEffect, useState } from 'react';
import { defineMessages } from 'react-intl';
import { useParams } from 'react-router-dom';
import { Cancel, FolderOutlined } from '@mui/icons-material';

import { loadDefaultMaterialId } from 'course/material/folders/handles';
import Link from 'lib/components/core/Link';
import useTranslation from 'lib/hooks/useTranslation';

import BaseRetrieveMaterialPage from '../../component/BaseRetrieveMaterialPage';

const translations = defineMessages({
problemRetrievingFolder: {
id: 'course.material.folders.ErrorRetrievingFolderPage.problemRetrievingFolder',
defaultMessage: 'Problem retrieving folder',
},
problemRetrievingFolderDescription: {
id: 'course.material.folders.ErrorRetrievingFolderPage.problemRetrievingFolderDescription',
defaultMessage:
"Either it no longer exists, you don't have the permission to access it, or something unexpected happened when we were trying to retrieve it.",
},
goToTheWorkbin: {
phungmanhcuong marked this conversation as resolved.
Show resolved Hide resolved
id: 'course.material.folders.ErrorRetrievingFolderPage.goToTheWorkbin',
defaultMessage: 'Go to the Workbin',
phungmanhcuong marked this conversation as resolved.
Show resolved Hide resolved
},
});

const Illustration = (): JSX.Element => (
<div className="relative">
<FolderOutlined className="text-[6rem]" color="disabled" />
<Cancel
className="absolute bottom-0 -right-2 text-[4rem] bg-white rounded-full"
color="error"
/>
</div>
);

const useWorkbinURL = (courseId: string | undefined): string => {
const [workbinURL, setWorkbinURL] = useState(`/courses/${courseId}`);

useEffect(() => {
if (courseId) {
loadDefaultMaterialId().then((defaultMaterialId) => {
setWorkbinURL(
`/courses/${courseId}/materials/folders/${defaultMaterialId}`,
);
});
}
}, [courseId]);

return workbinURL;
};

const ErrorRetrievingFolderPage = (): JSX.Element => {
const { t } = useTranslation();
const params = useParams();
const workbinURL = useWorkbinURL(params.courseId);

return (
<BaseRetrieveMaterialPage
description={t(translations.problemRetrievingFolderDescription)}
illustration={<Illustration />}
title={t(translations.problemRetrievingFolder)}
>
<Link className="mt-10" to={workbinURL}>
{t(translations.goToTheWorkbin)}
</Link>
</BaseRetrieveMaterialPage>
);
};

export default ErrorRetrievingFolderPage;
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { FC, ReactElement, useEffect, useState } from 'react';
import { defineMessages } from 'react-intl';
import { useParams } from 'react-router-dom';
import { useLoaderData, useParams } from 'react-router-dom';

import EditButton from 'lib/components/core/buttons/EditButton';
import Page from 'lib/components/core/layouts/Page';
Expand All @@ -10,12 +10,13 @@ import { getCourseId } from 'lib/helpers/url-helpers';
import { useAppDispatch, useAppSelector } from 'lib/hooks/store';
import useTranslation from 'lib/hooks/useTranslation';

import { FolderData } from '../../../../../../types/course/material/folders';
import DownloadFolderButton from '../../components/buttons/DownloadFolderButton';
import NewSubfolderButton from '../../components/buttons/NewSubfolderButton';
import UploadFilesButton from '../../components/buttons/UploadFilesButton';
import MaterialUpload from '../../components/misc/MaterialUpload';
import WorkbinTable from '../../components/tables/WorkbinTable';
import { loadFolder } from '../../operations';
import { dispatchFolderData } from '../../operations';
import {
getCurrFolderInfo,
getFolderMaterials,
Expand Down Expand Up @@ -48,13 +49,16 @@ const FolderShow: FC = () => {
const materials = useAppSelector(getFolderMaterials);
const currFolderInfo = useAppSelector(getCurrFolderInfo);
const permissions = useAppSelector(getFolderPermissions);
const loaderData = useLoaderData() as FolderData;

const [isLoading, setIsLoading] = useState(true);
useEffect(() => {
if (folderId) {
dispatch(loadFolder(+folderId)).finally(() => setIsLoading(false));
if (loaderData) {
dispatch(dispatchFolderData(loaderData)).finally(() =>
setIsLoading(false),
);
}
}, [dispatch, folderId]);
}, [dispatch, loaderData]);

if (isLoading) {
return <LoadingIndicator />;
Expand Down
4 changes: 4 additions & 0 deletions client/app/routers/AuthenticatedApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import LessonPlanShow from 'bundles/course/lesson-plan/pages/LessonPlanShow';
import LevelsIndex from 'bundles/course/level/pages/LevelsIndex';
import DownloadingFilePage from 'bundles/course/material/files/DownloadingFilePage';
import ErrorRetrievingFilePage from 'bundles/course/material/files/ErrorRetrievingFilePage';
import ErrorRetrievingFolderPage from 'bundles/course/material/folders/pages/ErrorRetrievingFolderPage';
import FolderShow from 'bundles/course/material/folders/pages/FolderShow';
import TimelineDesigner from 'bundles/course/reference-timelines/TimelineDesigner';
import ResponseEdit from 'bundles/course/survey/pages/ResponseEdit';
Expand Down Expand Up @@ -127,6 +128,7 @@ import {
forumTopicHandle,
} from 'course/forum/handles';
import { leaderboardHandle } from 'course/leaderboard/handles';
import folderLoader from 'course/material/folderLoader';
import { folderHandle } from 'course/material/folders/handles';
import materialLoader from 'course/material/materialLoader';
import { videoWatchHistoryHandle } from 'course/statistics/handles';
Expand Down Expand Up @@ -221,7 +223,9 @@ const authenticatedRouter: Translated<RouteObject[]> = (t) =>
children: [
{
index: true,
loader: folderLoader,
element: <FolderShow />,
errorElement: <ErrorRetrievingFolderPage />,
},
{
path: 'files/:materialId',
Expand Down
Loading