Skip to content

feat: add campaign assignments report UI and API integration#4748

Open
renukaj07 wants to merge 3 commits into
devfrom
renuka-dev1
Open

feat: add campaign assignments report UI and API integration#4748
renukaj07 wants to merge 3 commits into
devfrom
renuka-dev1

Conversation

@renukaj07

Copy link
Copy Markdown
Contributor
  • Added the Campaign Reports Assignments subtab UI in ops console with desktop and mobile rendering, summary widgets, tooltip support, and subject-wise assignment table.

  • Wired the assignments report into the Campaign Reports flow by passing campaign context and overview totalStudents into the new assignments report component.

  • Added the assignments report API contract in ServiceApi, ApiHandler, and SqliteApi so the feature can fetch structured summary and subject-level report data.

  • Updated SupabaseApi to call the dedicated assignments report RPC and return the normalized assignments report payload to the frontend.

  • Added and updated English translation keys required for the new assignments report labels and tooltip copy.

  • Added focused tests for CampaignAssignmentsReport and updated report-level tests to cover the Assignments subtab rendering and API call flow.

image image

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@renukaj07
renukaj07 requested a review from vinaypanduga July 20, 2026 11:26

@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 new 'Assignments' report subtab to the campaigns overview, allowing users to view assignment summary widgets and subject-wise completion rows. It adds the CampaignAssignmentsReport component, updates the API layer to fetch report data via a new Supabase RPC, and integrates the subtab into the parent views. The review feedback focuses on enhancing robustness and code quality: it suggests using optional chaining and nullish coalescing operators to prevent potential runtime crashes from null or undefined API payloads, and recommends refactoring the hook's state management to use a single state with derived values to avoid desynchronization and redundant properties.

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 +1947 to +1960
const payload =
data as Database['public']['Functions']['get_campaign_assignments_report']['Returns'];

return {
summary: {
totalAssignments: payload.summary?.totalAssignments ?? 0,
assignedStudents:
payload.summary?.assignedStudents ?? params.totalStudents ?? 0,
activeStudents: payload.summary?.activeStudents ?? 0,
averageAssignmentsCompletion:
payload.summary?.averageAssignmentsCompletion ?? 0,
},
rows: payload.rows ?? [],
};

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 the RPC call returns null or fails, data (and thus payload) will be null. Accessing payload.summary directly will throw a TypeError and crash the application. We should use optional chaining (payload?.summary and payload?.rows) to safely handle this scenario.

Suggested change
const payload =
data as Database['public']['Functions']['get_campaign_assignments_report']['Returns'];
return {
summary: {
totalAssignments: payload.summary?.totalAssignments ?? 0,
assignedStudents:
payload.summary?.assignedStudents ?? params.totalStudents ?? 0,
activeStudents: payload.summary?.activeStudents ?? 0,
averageAssignmentsCompletion:
payload.summary?.averageAssignmentsCompletion ?? 0,
},
rows: payload.rows ?? [],
};
const payload =
data as Database['public']['Functions']['get_campaign_assignments_report']['Returns'];
return {
summary: {
totalAssignments: payload?.summary?.totalAssignments ?? 0,
assignedStudents:
payload?.summary?.assignedStudents ?? params.totalStudents ?? 0,
activeStudents: payload?.summary?.activeStudents ?? 0,
averageAssignmentsCompletion:
payload?.summary?.averageAssignmentsCompletion ?? 0,
},
rows: payload?.rows ?? [],
};

Comment on lines +22 to +67
export const buildCampaignAssignmentsSummaryCards = ({
activeStudents,
assignedStudents,
averageAssignmentsCompletion,
totalAssignments,
}: {
activeStudents: number;
assignedStudents: number;
averageAssignmentsCompletion: number;
totalAssignments: number;
}): CampaignAssignmentsSummaryCard[] => [
{
key: 'totalAssignments',
label: t('Total Assignments'),
value: String(totalAssignments),
info: t(
'Total number of assignments assigned through the campaign while creating the campaign.',
),
},
{
key: 'assignedStudents',
label: t('Assigned Students'),
value: assignedStudents.toLocaleString('en-IN'),
info: t(
'Total number of students who received at least one assignment through the campaign.',
),
},
{
key: 'activeStudents',
label: t('Active Students'),
value: activeStudents.toLocaleString('en-IN'),
info: t(
'Total number of students who completed at least one assignment during the campaign period.',
),
},
{
key: 'averageAssignmentsCompletion',
label: t('Average Assignments Completion'),
value: `${averageAssignmentsCompletion.toLocaleString('en-IN', {
maximumFractionDigits: 1,
})}%`,
info: t(
'Average number of assignments completed per active student during the selected period.',
),
},
];

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

To prevent potential runtime crashes if null or undefined values are passed from the API at runtime, we should use nullish coalescing operators (?? 0) before calling .toLocaleString() or String() on these numeric fields.

export const buildCampaignAssignmentsSummaryCards = ({
  activeStudents,
  assignedStudents,
  averageAssignmentsCompletion,
  totalAssignments,
}: {
  activeStudents: number;
  assignedStudents: number;
  averageAssignmentsCompletion: number;
  totalAssignments: number;
}): CampaignAssignmentsSummaryCard[] => [
  {
    key: 'totalAssignments',
    label: t('Total Assignments'),
    value: String(totalAssignments ?? 0),
    info: t(
      'Total number of assignments assigned through the campaign while creating the campaign.',
    ),
  },
  {
    key: 'assignedStudents',
    label: t('Assigned Students'),
    value: (assignedStudents ?? 0).toLocaleString('en-IN'),
    info: t(
      'Total number of students who received at least one assignment through the campaign.',
    ),
  },
  {
    key: 'activeStudents',
    label: t('Active Students'),
    value: (activeStudents ?? 0).toLocaleString('en-IN'),
    info: t(
      'Total number of students who completed at least one assignment during the campaign period.',
    ),
  },
  {
    key: 'averageAssignmentsCompletion',
    label: t('Average Assignments Completion'),
    value: (averageAssignmentsCompletion ?? 0).toLocaleString('en-IN', {
      maximumFractionDigits: 1,
    }) + '%',
    info: t(
      'Average number of assignments completed per active student during the selected period.',
    ),
  },
];

Comment on lines +69 to +82
export const mapAssignmentReportRows = (
rows: Array<{
subjectId: string;
subjectName: string;
lessonsAssigned: number;
completionPercent: number;
}>,
): CampaignAssignmentsTableRow[] =>
rows.map((row) => ({
id: row.subjectId,
subject: row.subjectName,
lessonsAssigned: row.lessonsAssigned,
completionPercent: `${Math.round(row.completionPercent)}%`,
}));

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

Add defensive fallbacks (?? 0) for lessonsAssigned and completionPercent to prevent potential runtime errors or NaN% values if the database returns null or undefined values.

Suggested change
export const mapAssignmentReportRows = (
rows: Array<{
subjectId: string;
subjectName: string;
lessonsAssigned: number;
completionPercent: number;
}>,
): CampaignAssignmentsTableRow[] =>
rows.map((row) => ({
id: row.subjectId,
subject: row.subjectName,
lessonsAssigned: row.lessonsAssigned,
completionPercent: `${Math.round(row.completionPercent)}%`,
}));
export const mapAssignmentReportRows = (
rows: Array<{
subjectId: string;
subjectName: string;
lessonsAssigned: number;
completionPercent: number;
}>,
): CampaignAssignmentsTableRow[] =>
rows.map((row) => ({
id: row.subjectId,
subject: row.subjectName,
lessonsAssigned: row.lessonsAssigned ?? 0,
completionPercent: Math.round(row.completionPercent ?? 0) + '%',
}));

Comment on lines +84 to +160
export const useCampaignAssignmentsReportState = (
campaignId?: string,
totalStudents?: number | null,
) => {
const [loading, setLoading] = useState(false);
const [rows, setRows] = useState<CampaignAssignmentsTableRow[]>([]);
const [summaryCards, setSummaryCards] = useState<
CampaignAssignmentsSummaryCard[]
>([]);

useEffect(() => {
let active = true;

const loadReport = async () => {
if (!campaignId) {
setRows([]);
setSummaryCards(
buildCampaignAssignmentsSummaryCards({
totalAssignments: 0,
assignedStudents: 0,
activeStudents: 0,
averageAssignmentsCompletion: 0,
}),
);
return;
}

setLoading(true);
try {
const response =
await ServiceConfig.getI().apiHandler.getCampaignAssignmentsReport(
campaignId,
{ totalStudents: totalStudents ?? 0 },
);
if (!active) return;
setRows(mapAssignmentReportRows(response.rows));
setSummaryCards(
buildCampaignAssignmentsSummaryCards({
totalAssignments: response.summary.totalAssignments,
assignedStudents: response.summary.assignedStudents,
activeStudents: response.summary.activeStudents,
averageAssignmentsCompletion:
response.summary.averageAssignmentsCompletion,
}),
);
} catch (error) {
if (!active) return;
logger.error('Error loading campaign assignments report:', error);
setRows([]);
setSummaryCards(
buildCampaignAssignmentsSummaryCards({
totalAssignments: 0,
assignedStudents: totalStudents ?? 0,
activeStudents: 0,
averageAssignmentsCompletion: 0,
}),
);
} finally {
if (active) setLoading(false);
}
};

void loadReport();
return () => {
active = false;
};
}, [campaignId, totalStudents]);

const mobileRows = useMemo(() => rows, [rows]);

return {
loading,
mobileRows,
rows,
summaryCards,
};
};

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

Instead of maintaining separate desynchronized states for rows and summaryCards (which causes multiple re-renders and a layout flash of empty cards on first render), we can store the raw reportData in a single state and derive rows and summaryCards using useMemo. This also allows us to eliminate the redundant mobileRows property.

export const useCampaignAssignmentsReportState = (
  campaignId?: string,
  totalStudents?: number | null,
) => {
  const [loading, setLoading] = useState(false);
  const [reportData, setReportData] = useState<CampaignAssignmentsReportResponse | null>(null);

  useEffect(() => {
    if (!campaignId) {
      setReportData(null);
      return;
    }

    let active = true;
    setLoading(true);

    const loadReport = async () => {
      try {
        const response =
          await ServiceConfig.getI().apiHandler.getCampaignAssignmentsReport(
            campaignId,
            { totalStudents: totalStudents ?? 0 },
          );
        if (!active) return;
        setReportData(response);
      } catch (error) {
        if (!active) return;
        logger.error('Error loading campaign assignments report:', error);
        setReportData(null);
      } finally {
        if (active) setLoading(false);
      }
    };

    void loadReport();
    return () => {
      active = false;
    };
  }, [campaignId, totalStudents]);

  const summaryCards = useMemo(() => {
    if (!reportData) {
      return buildCampaignAssignmentsSummaryCards({
        totalAssignments: 0,
        assignedStudents: totalStudents ?? 0,
        activeStudents: 0,
        averageAssignmentsCompletion: 0,
      });
    }
    return buildCampaignAssignmentsSummaryCards({
      totalAssignments: reportData.summary.totalAssignments,
      assignedStudents: reportData.summary.assignedStudents,
      activeStudents: reportData.summary.activeStudents,
      averageAssignmentsCompletion: reportData.summary.averageAssignmentsCompletion,
    });
  }, [reportData, totalStudents]);

  const rows = useMemo(() => {
    if (!reportData) return [];
    return mapAssignmentReportRows(reportData.rows);
  }, [reportData]);

  return {
    loading,
    rows,
    summaryCards,
  };
};

Comment on lines +98 to +100
{isMobile ? (
<Box mt={1.5} display="grid" gap={1}>
{report.mobileRows.map((row) => (

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

Update the mobile view to map over report.rows directly, since the redundant mobileRows property has been removed.

Suggested change
{isMobile ? (
<Box mt={1.5} display="grid" gap={1}>
{report.mobileRows.map((row) => (
{isMobile ? (
<Box mt={1.5} display="grid" gap={1}>
{report.rows.map((row) => (

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.

1 participant