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

phungmanhcuong · 2024-10-23T10:43:54Z

This pull request fixes the issue where a default 404 page was shown when a folder was missing in the course materials section. Now, a custom error page is displayed instead. This resolves Issue #7437, which was raised to improve user experience by providing a more informative error message when materials are not found.

cysjonathan

Please squash both commits. Your 2nd commit changes only contain:

text formatting changes (should have been squashed in first commit)
Change of icon from InsertDriveFileOutlined to FolderOutlined (should have done this in the first commit in the first place)

client/app/bundles/course/material/folders/handles.ts

client/app/bundles/course/material/folders/pages/BaseRetrievePage.tsx

client/app/bundles/course/material/folders/pages/ErrorRetrievingFolderPage.tsx

client/app/bundles/course/material/folders/pages/FolderShow/index.tsx

client/app/bundles/course/material/folders/pages/ErrorRetrievingFolderPage.tsx

client/app/bundles/course/material/folderLoader.ts

client/app/bundles/course/material/files/ErrorRetrievingFilePage.tsx

client/app/bundles/course/material/BaseRetrieveMaterialPage.tsx

client/app/bundles/course/material/folderLoader.ts

cysjonathan · 2024-10-25T11:40:06Z

client/app/bundles/course/material/folders/pages/ErrorRetrievingFolderPage.tsx

+const useWorkbinURL = (courseId: string | undefined): string => {
+  const [workbinURL, setWorkbinURL] = useState(`/courses/${courseId}`);
+
+  useEffect(() => {
+    if (courseId) {
+      loadDefaultMaterial(+courseId).then((materialsItem) => {
+        if (materialsItem?.path) {
+          setWorkbinURL(materialsItem.path);
+        }
+      });
+    }
+  }, [courseId]);
+
+  return workbinURL;
+};


You should be fetching the folderId in the loader,
then consume the data here using useLoaderData().

The loaderData might not be available on the ErrorPage.

cysjonathan · 2024-10-25T12:01:32Z

client/app/bundles/course/material/folders/handles.ts

+const extractMaterial = (
+  sidebar: { label: string; key: string; path: string }[],
+): { label: string; key: string; path: string } | undefined =>
+  sidebar.find((item) => item.key === 'materials');
+
+export const loadDefaultMaterial = async (
+  courseId: number,
+): Promise<{
+  label: string;
+  key: string;
+  path: string;
+} | null> => {
+  const {
+    data: { sidebar },
+  } = await CourseAPI.courses.fetchLayout(courseId);
+  return sidebar ? extractMaterial(sidebar) || null : null;
+};


This logic for pulling information from sidebar makes the code convoluted.

Just open a new /courses/<course-id>/materials route and then you can statically reference that route.
The route can then redirect the user to the root folder.
This will make the code clean.

To be confirmed your idea, this is whats you want ?

Backend needs to open a route as well

Have added new backend api and updated the fronted for using it.

config/routes.rb

…ing in course materials

cysjonathan · 2024-10-31T08:11:15Z

app/controllers/course/material/materials_controller.rb

+  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


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

cysjonathan · 2024-10-31T08:39:20Z

app/controllers/course/material/materials_controller.rb

-  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


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)

cysjonathan · 2024-10-31T08:42:21Z

client/app/api/course/Materials.ts

@@ -25,6 +31,10 @@ export default class MaterialsAPI extends BaseCourseAPI {
    );
  }

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


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

cysjonathan · 2024-10-31T08:53:55Z

client/app/bundles/course/material/folders/handles.ts

  const folderId = getIdFromUnknown(match.params?.folderId);
+  if (!courseId) throw new Error(`Invalid course id: ${courseId}`);


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.

cysjonathan · 2024-10-31T09:01:41Z

client/app/api/course/Materials.ts

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

 export default class MaterialsAPI extends BaseCourseAPI {
+  get #materialPrefix(): string {


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.

cysjonathan · 2024-10-31T09:05:37Z

client/app/bundles/course/material/folders/handles.ts

+    const defaultMaterialId = await loadDefaultMaterialId();
+    ({ data } = await CourseAPI.folders.fetch(defaultMaterialId));


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).

fetch invalid folder payload (throws error)

~~fetch valid root folderId~~

fetch valid root folder payload

cysjonathan · 2024-10-31T09:08:30Z

client/app/bundles/course/material/folders/operations.ts

+export function dispatchFolderData(
+  data: FolderData,
+): Operation<SaveFolderAction> {
+  return async (dispatch) => {
+    return dispatch(
+      actions.saveFolder(
+        data.currFolderInfo,
+        data.subfolders,
+        data.materials,
+        data.advanceStartAt,
+        data.permissions,
+      ),
+    );
+  };


Is there really a need to modify this?

cysjonathan · 2024-11-14T08:03:09Z

superseded by #7626

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch from 357b033 to 0ae652d Compare October 23, 2024 11:00

phungmanhcuong changed the title ~~Fix: Display custom error page when file is missing in course materials~~ Fix: Display custom error page when folder is missing in course materials Oct 23, 2024

cysjonathan requested a review from bivanalhar October 23, 2024 12:37

cysjonathan requested changes Oct 24, 2024

View reviewed changes

bivanalhar requested changes Oct 24, 2024

View reviewed changes

client/app/bundles/course/material/folders/pages/FolderShow/index.tsx Outdated Show resolved Hide resolved

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch from 0ae652d to 2df7c03 Compare October 24, 2024 02:57

bivanalhar requested changes Oct 24, 2024

View reviewed changes

client/app/bundles/course/material/folders/pages/ErrorRetrievingFolderPage.tsx Show resolved Hide resolved

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch 5 times, most recently from c57bf44 to c939249 Compare October 25, 2024 00:49

phungmanhcuong requested review from cysjonathan and bivanalhar October 25, 2024 01:57

phungmanhcuong self-assigned this Oct 25, 2024

phungmanhcuong requested a review from syoopie October 25, 2024 01:57

syoopie reviewed Oct 25, 2024

View reviewed changes

client/app/bundles/course/material/folders/pages/ErrorRetrievingFolderPage.tsx Show resolved Hide resolved

client/app/bundles/course/material/folderLoader.ts Show resolved Hide resolved

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch from c939249 to e5d1958 Compare October 25, 2024 03:28

cysjonathan requested changes Oct 25, 2024

View reviewed changes

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch 3 times, most recently from d1b5abf to 0f54b22 Compare October 30, 2024 08:14

syoopie reviewed Oct 30, 2024

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch 2 times, most recently from 23414e8 to 8f234c3 Compare October 30, 2024 09:15

fix(materials): display custom error page when file or folder is miss…

2c84924

…ing in course materials

phungmanhcuong force-pushed the mcphung/materials-missing-folder-error-page branch from 8f234c3 to 2c84924 Compare October 30, 2024 09:30

cysjonathan requested changes Oct 31, 2024

View reviewed changes

cysjonathan mentioned this pull request Oct 31, 2024

feat(materials): add 404 folder not found page #7626

Merged

3 tasks

cysjonathan closed this Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

phungmanhcuong commented Oct 23, 2024 •

edited

Loading

cysjonathan left a comment

cysjonathan Oct 25, 2024

phungmanhcuong Oct 30, 2024

cysjonathan Oct 25, 2024

phungmanhcuong Oct 28, 2024 •

edited

Loading

cysjonathan Oct 28, 2024

phungmanhcuong Oct 30, 2024

cysjonathan Oct 31, 2024

cysjonathan Oct 31, 2024

cysjonathan Oct 31, 2024

cysjonathan Oct 31, 2024

cysjonathan Oct 31, 2024

cysjonathan Oct 31, 2024

cysjonathan Oct 31, 2024

cysjonathan commented Nov 14, 2024

		const folderId = getIdFromUnknown(match.params?.folderId);
		if (!courseId) throw new Error(`Invalid course id: ${courseId}`);

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

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

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

Conversation

phungmanhcuong commented Oct 23, 2024 • edited Loading

cysjonathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phungmanhcuong Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cysjonathan commented Nov 14, 2024

phungmanhcuong commented Oct 23, 2024 •

edited

Loading

phungmanhcuong Oct 28, 2024 •

edited

Loading