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

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

Merged
merged 7 commits into from
Nov 12, 2024

Conversation

cysjonathan
Copy link
Contributor

resolves issue #7437 by showing a 404 screen when visiting non-existent folders instead of an empty materials table
supersedes PR #7612

  • opens up new /materials/folders route which returns course's root folder data
  • when accessing a non-existent folder, 404 screen will appear (similar to non-existent file)
  • other minor changes --- add rspec for material folder show, drop trailing slash in folder links in FE

@cysjonathan cysjonathan changed the title Jon/materials missing folder error page feat(materials): add 404 folder not found page Oct 31, 2024
@cysjonathan cysjonathan force-pushed the jon/materials-missing-folder-error-page branch from 4b02e06 to 7811db9 Compare October 31, 2024 20:18
Copy link
Contributor

@purfectliterature purfectliterature left a comment

Choose a reason for hiding this comment

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

Very neat PR, except for,

  • the controller calling other controller action, and
  • anti-patterned getFolderTitle definition.

The rest are nitpicks and some questions.

With the new /materials/folders route, technically we can already replace the path for Workbin on the sidebar. This should reduce about 1-2 SQL calls since there's no need to resolve current_course.root_folder in main_sidebar_items in Course::MaterialsComponent. Just need to ensure that activePath (hence the active highlight on the sidebar) still works even after this change. But this isn't urgent, and the behaviour this PR brings is already good as it is; it can be another good first issue for others.

app/controllers/course/material/folders_controller.rb Outdated Show resolved Hide resolved
app/controllers/course/material/folders_controller.rb Outdated Show resolved Hide resolved
app/controllers/course/material/folders_controller.rb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
client/app/bundles/course/material/folders/handles.ts Outdated Show resolved Hide resolved
client/app/bundles/course/material/folders/handles.ts Outdated Show resolved Hide resolved
client/app/bundles/course/material/folders/handles.ts Outdated Show resolved Hide resolved
client/app/bundles/course/material/folders/handles.ts Outdated Show resolved Hide resolved
client/app/bundles/course/material/folders/handles.ts Outdated Show resolved Hide resolved
@cysjonathan cysjonathan force-pushed the jon/materials-missing-folder-error-page branch 4 times, most recently from da670f3 to 8344117 Compare November 4, 2024 17:49
@cysjonathan cysjonathan force-pushed the jon/materials-missing-folder-error-page branch 2 times, most recently from 3bef4f2 to 35e25a4 Compare November 6, 2024 05:19
@cysjonathan cysjonathan force-pushed the jon/materials-missing-folder-error-page branch from 35e25a4 to 66493a6 Compare November 6, 2024 08:03
- clean up folders_controller methods
- add rspec tests for show and index
- clean up rspec tests
- remove material root folder query in sidebar
point `/courses/<course-id>/materials/folders` to course root folder
@cysjonathan cysjonathan force-pushed the jon/materials-missing-folder-error-page branch from 66493a6 to f308fb8 Compare November 11, 2024 04:09
Copy link
Contributor

@bivanalhar bivanalhar left a comment

Choose a reason for hiding this comment

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

@cysjonathan the code now has been made much cleaner, just one more comment to be addressed, to make the redirection to Root Folder much clearer and easier to understand

@cysjonathan cysjonathan force-pushed the jon/materials-missing-folder-error-page branch from f308fb8 to 8331fdc Compare November 11, 2024 09:07
@cysjonathan cysjonathan dismissed purfectliterature’s stale review November 12, 2024 03:59

resolved conversations

@cysjonathan cysjonathan merged commit 2c5354b into master Nov 12, 2024
12 of 13 checks passed
@cysjonathan cysjonathan deleted the jon/materials-missing-folder-error-page branch November 12, 2024 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants