Skip to content

Fix: course creation - prevent comparison error for books with None for shelf_section - #1331

Merged
bnmnetp merged 2 commits into
RunestoneInteractive:mainfrom
ascholerChemeketa:no-category-for-book
Aug 2, 2026
Merged

Fix: course creation - prevent comparison error for books with None for shelf_section#1331
bnmnetp merged 2 commits into
RunestoneInteractive:mainfrom
ascholerChemeketa:no-category-for-book

Conversation

@ascholerChemeketa

Copy link
Copy Markdown
Contributor

I have a bunch of books in my dev library where shelf_section is None. That crashes the sort code when creating a new course. This fixes the course creation page to handle those.

Copilot AI review requested due to automatic review settings July 30, 2026 20:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent a crash on the course creation page when sorting book shelf_section values that may be None, by introducing a fallback section label.

Changes:

  • Default missing shelf_section values to "Misc" when building the sections list for the course creation page.
  • Apply the same sections behavior in both the GET handler and the error-rendering path of the POST handler.
Comments suppressed due to low confidence (1)

bases/rsptx/admin_server_api/routers/instructor.py:1450

  • Same issue in the error-handling path: sections uses the "Misc" fallback, but course_list retains shelf_section=None, so those books won’t render under any section in the template (it compares course.shelf_section == section). Normalize shelf_section in course_list before computing sections.
        course_list = [c.dict() for c in course_list if c.for_classes]
        sections = sorted({c["shelf_section"] or "Misc" for c in course_list})

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1322 to +1323
course_list = [c.dict() for c in course_list if c.for_classes]
sections = sorted({c["shelf_section"] for c in course_list})
sections = sorted({c["shelf_section"] or "Misc" for c in course_list})
@bnmnetp

bnmnetp commented Jul 31, 2026

Copy link
Copy Markdown
Member

Seems like providing a default for the shelf_section and/or making a pass through course_list to populate None values with Misc would be better so we don't leave books out of the library page.

Also we ought to file issues against those books that don't provide a shelf section so the author can remedy that. I must have "fixed" all those in production a while ago.

Copilot AI review requested due to automatic review settings August 2, 2026 17:33
@ascholerChemeketa

Copy link
Copy Markdown
Contributor Author

Added a default value to create_library_book that will make sure you can't add books with None for shelf_section.

Pretty sure the offending path was using addbookauthor to manually add books.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

bases/rsptx/admin_server_api/routers/instructor.py:1323

  • Only sections is normalized to replace None with "Misc"; course_list items still have shelf_section=None. In the template, books are rendered under a section only when course.shelf_section == section, so books with None will not show up under the "Misc" header (or any header), even though the crash is avoided.
    course_list = [c.dict() for c in course_list if c.for_classes]
    sections = sorted({c["shelf_section"] or "Misc" for c in course_list})

bases/rsptx/admin_server_api/routers/instructor.py:1450

  • Same issue as the GET handler: normalizing only the sections list prevents the TypeError but still leaves course_list entries with shelf_section=None, so those books won't render under the "Misc" section in create_course.html.
        course_list = [c.dict() for c in course_list if c.for_classes]
        sections = sorted({c["shelf_section"] or "Misc" for c in course_list})

components/rsptx/db/crud/library.py:69

  • book_vals = {"shelf_section": "Misc", **vals} does not actually prevent shelf_section=None because an explicit None in vals will override the default. This means new/updated callers that pass shelf_section: None can still write NULL and reintroduce the course creation sorting/display issue.
    book_vals = {"shelf_section": "Misc", **vals}
    new_book = Library(**book_vals, basecourse=bookid)

@ascholerChemeketa

Copy link
Copy Markdown
Contributor Author

Updated first commit to restrict Select All button to cards view.

@bnmnetp

bnmnetp commented Aug 2, 2026

Copy link
Copy Markdown
Member

I think your last comment belongs on the other PR. Otherwise this looks good.

@bnmnetp
bnmnetp merged commit 68ef0b9 into RunestoneInteractive:main Aug 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants