Skip to content

feat(admin): add Courses Without BR page (#142) - #184

Open
mohitmohan2005-wq wants to merge 2 commits into
Coding-Club-IITG:devfrom
mohitmohan2005-wq:fix/courses-without-br
Open

feat(admin): add Courses Without BR page (#142)#184
mohitmohan2005-wq wants to merge 2 commits into
Coding-Club-IITG:devfrom
mohitmohan2005-wq:fix/courses-without-br

Conversation

@mohitmohan2005-wq

Copy link
Copy Markdown

Closes #142
Adds an admin page listing courses that have no branch representative assigned, backed by the getCoursesWithoutBR endpoint.

  • fetchCoursesWithoutBR in admin/src/apis/br.js
  • CoursesWithoutBR page + coursesWithoutBRTable component
  • Wire up /admin/courses-without-br route and sidebar nav item

Adds an admin page listing courses that have no branch representative
assigned, backed by the getCoursesWithoutBR endpoint.

- fetchCoursesWithoutBR in admin/src/apis/br.js
- CoursesWithoutBR page + coursesWithoutBRTable component
- Wire up /admin/courses-without-br route and sidebar nav item

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@DreamBot706 DreamBot706 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.

Review Summary:
The implementation of the new page and routing works, but there are security and performance issues that need fixing before this can be merged. We also need to stick to the established naming conventions. Address the inline comments below.

File Comments:

  • File: admin/src/apis/br.js

    • Comment: Security: You are using a hardcoded token (Bearer admin-coursehub-cc23-golang) in the Authorization header. Fetch this dynamically from environment variables or secure storage (like localStorage), similar to how authentication is handled in other API functions. Never hardcode tokens.
  • File: admin/src/components/coursesWithoutBRTable.jsx

    • Comment: Convention: Rename this file to PascalCase (CoursesWithoutBRTable.jsx) to match the naming convention of all other React components in this project.
    • Comment: UX: If the courses array is empty, the table renders headers but no rows. Implement a proper empty state (e.g., a "No courses found" message) when courses.length === 0.
  • File: server/modules/br/br.controller.js (Function: getCoursesWithoutBR)

    • Comment: Performance: This function fetches all users, all BR records, and all courses into application memory before filtering them in JavaScript. This will not scale. Refactor this to use MongoDB aggregation ($lookup and $match) so the filtering is done at the database level.

- Drop hardcoded admin Bearer token from br.js; rely on cookie auth
  (credentials: "include") like the rest of the admin app — the token
  wasn't a valid JWT and /api/br/* has no auth middleware anyway.
- Rename coursesWithoutBRTable.jsx to CoursesWithoutBRTable.jsx (PascalCase)
  and update its import.
- Add a "No courses found" empty state to CoursesWithoutBRTable.
- Refactor getCoursesWithoutBR to filter via a MongoDB aggregation
  pipeline ($lookup + $match) instead of loading all BRs, users, and
  courses into memory.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mohitmohan2005-wq

Copy link
Copy Markdown
Author

Thanks for the review! Addressed all four:

  1. Removed the hardcoded Bearer token from all functions in br.js —
    turned out it wasn't a valid JWT and /api/br/* has no auth
    middleware enforcing it anyway, so relying on the existing cookie
    auth (credentials: "include"), like the rest of the admin app,
    is correct and sufficient.
  2. Renamed to CoursesWithoutBRTable.jsx and updated the import
  3. Added a "No courses found" empty state
  4. Refactored getCoursesWithoutBR to use MongoDB aggregation
    ($lookup + $match) instead of loading everything into memory

Tested locally against a real server + in-memory MongoDB, including
edge cases (case-insensitive email matching, whitespace/case-
normalized course codes, a BR with no registered user, empty result).

Ready for another look whenever you get a chance!

@DreamBot706 DreamBot706 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.

LGTM!

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.

Courses Without BR (3)

2 participants