Skip to content

fix: Add Subjects to School button is missing for OPS user in Teacher mode#4734

Merged
vinaypanduga merged 1 commit into
mainfrom
main-ng-9
Jul 16, 2026
Merged

fix: Add Subjects to School button is missing for OPS user in Teacher mode#4734
vinaypanduga merged 1 commit into
mainfrom
main-ng-9

Conversation

@narayanangourav-sutara

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a list of roles authorized to modify subjects (SUBJECT_MODIFICATION_ROLES) and updates the permission check in SubjectSelection.tsx to allow users with these roles to perform modifications. The review feedback points out a potential runtime error where roles might be undefined or null, and suggests using optional chaining (roles?.includes(role)) to prevent a TypeError.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +128 to +130
const hasSubjectModificationRole = SUBJECT_MODIFICATION_ROLES.some((role) =>
roles.includes(role),
);

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.

medium

Using roles.includes(role) directly can throw a TypeError if roles is undefined or null. Since roles is retrieved from the Redux store and might not be immediately available (as indicated by the defensive check const userRoles = roles || [] on line 99), we should use optional chaining roles?.includes(role) to prevent potential runtime crashes.

Suggested change
const hasSubjectModificationRole = SUBJECT_MODIFICATION_ROLES.some((role) =>
roles.includes(role),
);
const hasSubjectModificationRole = SUBJECT_MODIFICATION_ROLES.some((role) =>
roles?.includes(role),
);

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ac39df6919

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +131 to +133
setCanModify(
schoolMatches &&
(normalizedRole === RoleType.PRINCIPAL || hasSubjectModificationRole),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow ops roles to bypass school matching

When an ops user opens a school's subjects from the school list, DetailList.handleSubjectIconClick navigates with only { schoolId } and does not update Util.getCurrentSchool(). If the user previously had another school selected, currentSchool.id becomes the target school after the fetch while selectedSchoolId remains stale, so this schoolMatches && ... gate still hides the Add button even though hasSubjectModificationRole is true. I inspected the ManageSchools/DetailList flow; this keeps the button missing for ops users browsing a different school than the one currently stored.

Useful? React with 👍 / 👎.

@vinaypanduga
vinaypanduga merged commit aa72604 into main Jul 16, 2026
1 check 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.

2 participants