-
Notifications
You must be signed in to change notification settings - Fork 78
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
@folder = Course::Material::Folder.where(course_id: current_course.id, parent_id: nil).order(:created_at).first | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use existing |
||
if @folder | ||
render json: @folder, status: :ok | ||
else | ||
render json: { error: 'No folders available' }, status: :not_found | ||
end | ||
end | ||
|
||
def show | ||
authorize!(:read_owner, @material.folder) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = ( | ||
|
@@ -15,8 +17,12 @@ const getShouldDownloadFromContentDisposition = ( | |
}; | ||
|
||
export default class MaterialsAPI extends BaseCourseAPI { | ||
get #materialPrefix(): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, you shouldn't be introducing folder logic into 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> { | ||
|
@@ -25,6 +31,10 @@ export default class MaterialsAPI extends BaseCourseAPI { | |
); | ||
} | ||
|
||
fetchDefault(): APIResponse<FolderMiniEntity> { | ||
return this.client.get(`${this.#materialPrefix}/load_default`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can also use |
||
} | ||
|
||
/** | ||
* 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. | ||
|
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
|
||
} | ||
|
||
const workbinUrl = `${courseUrl}/materials/folders/${data.breadcrumbs[0].id}`; | ||
|
||
|
@@ -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}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant check. Your code will never throw this error unless In any case, |
||
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), | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { Operation } from 'store'; | ||
import { | ||
FolderData, | ||
FolderFormData, | ||
MaterialFormData, | ||
MaterialUploadFormData, | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there really a need to modify this? |
||
} | ||
|
||
export function createFolder( | ||
|
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; |
There was a problem hiding this comment.
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 inCourse::Material::FoldersController
instead.You will need to
skip_load_and_authorize_resource :folder
for the method you create inCourse::Material::FoldersController
The perk of doing that is that you can simply set the
@folder
and then fallback to the defaultshow
Folder method, which will do its own authorization checks.This will also align json responses (all folder responses return the same json schema)