Skip to content

create: Merge from master to dev#4744

Open
narayanangourav-sutara wants to merge 54 commits into
devfrom
dev-ng-9
Open

create: Merge from master to dev#4744
narayanangourav-sutara wants to merge 54 commits into
devfrom
dev-ng-9

Conversation

@narayanangourav-sutara

Copy link
Copy Markdown
Contributor

No description provided.

Chimple Learning and others added 30 commits July 9, 2026 07:00
Translations update from Hosted Weblate
Fix: Campaign listing mobile browser zoom and assignment filtering
Root cause: partners is in text[], but the query applied scalar ilike, causing error
fix: Program Tab Filter Is Not Working
Root cause: the percentage fields are calculated values, not program_metrics database columns.
fix: Program Listing Filters Are Not Working
Fix: Restrict program creation access for program managers
Return uniqueSubjects as part of the campaign assignments response and consume it in the UI. CampaignAssignmentTab no longer calls getCampaignSubjectsByCampaignId; it uses response.uniqueSubjects and clears subjects on error. ServiceApi types updated to include uniqueSubjects. SupabaseApi parses unique_subjects from the RPC result, maps them, and adjusts total calculation. Database types updated for unique_subjects. Tests updated to reflect the new payload shape and behavior.
fix: disable sorting for school performance column for ops users in school list
fix: Message Editing Remains Enabled After Campaign Cancellation.
Include uniqueSubjects in campaign assignments
Fetch available grades from classes linked to the selected schools instead of showing all program grades. This ensures users only see grade options that are actually available for their school selection.

Changes include:
- Added getCampaignGradesForSchools API method to fetch grades by school IDs
- Implemented lazy loading of grades when schools are selected
- Filter selected grades to only include available options after schools change
- Add close-on-click-away behavior to grade dropdown for better UX
- Update tests to verify grade filtering and selection behavior
fix: Load grades dynamically based on selected schools
narayanangourav-sutara and others added 24 commits July 16, 2026 15:17
Update CampaignSetupActions disableNext logic to handle activeStep === 4. Previously other steps defaulted to false, which allowed advancing during submission on step 4. Now disableNext uses isSubmitting for step 4 to prevent duplicate submits. (File: src/ops-console/components/campaignSetup/CampaignSetupActions.tsx)
fix: Add Subjects to School button is missing for OPS user in Teacher mode
Disable Next while submitting on step 4
refactor: improve refresh token persistence and implement automatic session restoration logic in SupabaseAuth
fix: fetching classes and class users separately, then aggregating students client-side
CHIM-1187 Preserve school profile selection flow
Added API to get the schools related to USer
fix: Optimize campaign reach using batched school metrics queries and campaign reach queries
fix: Campaign Message Tab Displays Incorrect Data for "Alternate Days" and "Alternate Weeks (≥2)" Schedules
fix: Subject Dropdown Is Not Displaying All Subjects in campaign
fix: Once loaded, only campaign Grades and Subjects are shown.

@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 several enhancements and fixes to the campaign management system, including dynamic grade filtering based on selected schools, optimized audience summary retrieval via a database RPC, and support for campaign frequency. It also restricts program creation to authorized roles, adds web session refresh capabilities using stored tokens, and refines routing permissions for field coordinators. Feedback on these changes highlights a potential crash in SubjectSelection.tsx due to missing optional chaining on roles, a SQL injection risk in SqliteApi.ts from direct string interpolation, and a performance concern in SupabaseApi.ts where database-level pagination is bypassed for percentage filtering. Additionally, throwing a generic error in SupabaseAuth.ts discards crucial error properties, and the custom click-away logic in CampaignMultiSelect.tsx is redundant and can be simplified.

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 +494 to +498
if (response?.error) {
throw new Error(
'Web session refresh failed: ' + response.error.message,
);
}

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.

high

Wrapping the original response.error in a generic Error object with a custom prefix message discards the original error type and its specific properties (such as status or code). Since this.isInvalidRefreshTokenError(error) likely inspects these properties to identify stale or invalid refresh tokens, throwing a plain Error will prevent it from correctly identifying the error. Throwing response.error directly preserves the full error context.

        if (response?.error) {
          throw response.error;
        }

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.

high

If roles is undefined or null (e.g., before the authentication state is fully loaded), calling roles.includes(role) will throw a TypeError and potentially crash the page. Using optional chaining (roles?.includes) or a fallback array ensures robust and defensive execution.

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

Comment on lines +4501 to +4504
LEFT JOIN ${TABLES.Assignment_user} au
ON a.id = au.assignment_id
AND au.user_id = "${studentId}"
AND au.is_deleted = 0

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.

security-high high

Directly interpolating studentId into the SQL query string poses a SQL injection risk. Even though this query runs on client-side SQLite, it is highly recommended to use parameterized queries with placeholders (e.g., ?) to ensure security, robust escaping, and query plan caching.

Comment on lines +45 to +57
const rootRef = useRef<HTMLDivElement | null>(null);
const handleClickAway = (event: MouseEvent | TouchEvent) => {
const target = event.target as Node | null;
if (target && rootRef.current?.contains(target)) {
return;
}

setOpen(false);
};

return (
<Autocomplete
multiple
disableCloseOnSelect
disablePortal
openOnFocus
options={options}
value={value}
loading={loading}
getOptionLabel={getOptionLabel}
isOptionEqualToValue={isOptionEqualToValue}
slotProps={{
popper: {
placement: 'bottom-start',
modifiers: [
{
name: 'flip',
enabled: false,
<ClickAwayListener onClickAway={handleClickAway}>
<div ref={rootRef} className="campaign-multi-select-root">

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

The ClickAwayListener component from @mui/material already handles detecting clicks outside of its child element. Therefore, maintaining a custom rootRef and manually checking rootRef.current?.contains(target) inside handleClickAway is redundant. We can simplify this by removing the ref and the custom handler entirely, and passing an inline arrow function directly to onClickAway.

  return (
    <ClickAwayListener onClickAway={() => setOpen(false)}>
      <div className="campaign-multi-select-root">

Comment on lines +12866 to +12868
const pagedQuery = requiresCalculatedPercentageFiltering
? orderedQuery
: orderedQuery.range(from, to);

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

When requiresCalculatedPercentageFiltering is active, database-level pagination is bypassed, and the entire dataset is fetched into memory to perform filtering and slicing. As the number of programs grows, this can lead to severe performance degradation, high memory usage, and increased network bandwidth. Consider calculating these percentage fields on the database side (e.g., via a database view or generated columns) so that PostgREST can filter them directly, preserving database-level pagination.

@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: 6d4a1d0851

ℹ️ 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 +178 to +183
const availableGrades = useMemo(() => {
const relevantGradeIds = new Set(
subjects.flatMap((subject) => subject.gradeIds),
);

return grades.filter((grade) => relevantGradeIds.has(grade.id));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Return grade IDs with the subject metadata

Against the checked-in get_campaign_assignments contract in src/services/database.ts (lines 7964-7967), each unique_subjects entry contains only subject_id and subject_name; SupabaseApi consequently defaults every missing grade_ids field to []. This makes relevantGradeIds empty here and removes every option from the Grade filter for real RPC responses, even though assignments have grades. The RPC contract or the client derivation needs to supply the subject-to-grade mapping before relying on it.

Useful? React with 👍 / 👎.

Comment on lines +12866 to +12869
const pagedQuery = requiresCalculatedPercentageFiltering
? orderedQuery
: orderedQuery.range(from, to);
const { data, error, count } = await pagedQuery;

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 Page through all rows before percentage filtering

When any calculated-percentage filter is active, this branch removes .range() and assumes the response contains the complete program_metrics relation before filtering and slicing it in memory. PostgREST still applies its configured maximum response-row limit to an unbounded select, so once the table exceeds that limit, matching programs beyond the returned prefix disappear and both total and later pages are incorrect. Fetch all batches explicitly or move the calculated filtering into an RPC/database expression.

Useful? React with 👍 / 👎.

const shouldUseDatabasePagination = Boolean(
nativeSortColumn || shouldUseRelationSort,
);
const shouldUseDatabasePagination = Boolean(nativeSortColumn);

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 Preserve pagination when sorting campaign relations

For manager and programName sorts, nativeSortColumn is undefined, so this change disables database pagination and later sorts only the unbounded query response in memory. If the campaign count exceeds the PostgREST response-row limit, campaigns beyond that limit are omitted, totalCount reports only the truncated response length, and users cannot reach them. Relation sorting needs a database/RPC ordering strategy or explicit batched retrieval before client-side sorting.

Useful? React with 👍 / 👎.

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.

10 participants