-
Notifications
You must be signed in to change notification settings - Fork 13
KB-810 Implement studying certificates #663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
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 implements comprehensive enhancements to the certificate module, transforming it from a basic request/approval system into a full-featured certificate management workflow. The changes support both electronic and paper certificate delivery options, add faculty operator controls for signatory selection and certificate regeneration, and improve the user experience with preview dialogs and better print/download workflows.
Changes:
- Introduced electronic vs paper delivery options with radio button selection in the student certificate request form
- Added faculty operator controls including signatory selection, operator notes, certificate regeneration, and preview functionality
- Refactored certificate download/print logic to use base64 encoding instead of blobs for better frontend compatibility
- Enhanced the faculty certificate detail page with separate sections for student data from the Dean system and approval workflows
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/models/certificate/* | New TypeScript interfaces for student certificate data, signatories, and operator requests |
| src/messages/*.json | Updated translations for new delivery options, operator fields, and student data display labels |
| src/lib/utils.tsx | Added base64ToBlob utility for converting base64 PDFs to downloadable blobs |
| src/components/ui/button.tsx | Added spin animation to loading spinner |
| src/app/.../certificates/components/request-certificate-form.tsx | Refactored form with radio buttons for delivery option and reordered fields |
| src/app/.../certificates/components/history-table.tsx | Updated to show document numbers and distinguish electronic/paper certificate downloads |
| src/app/.../facultycertificate/utils/print-certificate.ts | Improved print logic with new window fallback and added unsigned certificate download functions |
| src/app/.../facultycertificate/utils/button-state-controller.ts | Enhanced button state logic to handle regenerate, preview, and unsigned print options |
| src/app/.../facultycertificate/page.content.tsx | Added refresh button with loading state |
| src/app/.../facultycertificate/loading.tsx | New loading screen component |
| src/app/.../facultycertificate/components/preview-dialog.tsx | New dialog component for PDF preview |
| src/app/.../facultycertificate/components/all-docs-table.tsx | Simplified table to show only essential columns with link to detail page |
| src/app/.../facultycertificate/[id]/student-data-section.tsx | New component displaying student data from Dean system |
| src/app/.../facultycertificate/[id]/page.tsx | Restructured to show student data and separate operator/approval sections |
| src/app/.../facultycertificate/[id]/action-buttons.tsx | Major refactoring with signatory selection, operator notes, and variant-based rendering |
| src/actions/certificates.actions.ts | Added multiple new actions for regeneration, signatories, unsigned PDFs, and operator workflows |
| CLAUDE.md | Added comprehensive certificate module documentation including status flow and API reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, 10000); | ||
| }, 0); | ||
| } catch (err) { | ||
| setTimeout(cleanup, 60000); // Keep iframe longer for Edge |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states 'Keep iframe longer for Edge' with a 60-second delay, but the same 60-second delay is also applied to the new window path at line 58. This seems inconsistent with the comment's rationale. If the longer delay is specifically for Edge/iframe compatibility, the new window path might not need such a long delay.
| includeOrderInfo: !certificate.includeOrderInfo ? ( | ||
| <Badge variant="neutral">{tTable('orderInfoExcluded')}</Badge> | ||
| ) : ( | ||
| <span className="text-neutral-500">{tTable('notRequired')}</span> |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The display logic for includeOrderInfo is confusing. When the value is false, it shows "Order info excluded", but when true it shows "Not required". The translation key 'notRequired' seems semantically incorrect for when order info IS included. Consider using a more appropriate translation key like 'included' or 'orderInfoIncluded' when the value is true.
| <span className="text-neutral-500">{tTable('notRequired')}</span> | |
| <span className="text-neutral-500">{tTable('orderInfoIncluded')}</span> |
| import saveAs from 'file-saver'; | ||
|
|
||
| function printPdfBlob(blob: Blob, filename?: string): Promise<void> { | ||
| function printPdfBlob(blob: Blob, _filename?: string): Promise<void> { |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter _filename is prefixed with an underscore indicating it's intentionally unused, but the parameter is actually never used in this function. Consider removing this parameter entirely since filenames are not set for print dialogs in browsers.
| const minDelay = new Promise((resolve) => setTimeout(resolve, 5000)); | ||
| try { | ||
| // Fetch fresh data from server and wait minimum 5 seconds | ||
| await Promise.all([getAllFacultyCertificates({ filter: searchFilter }), minDelay]); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimum delay of 5 seconds seems arbitrary and creates unnecessary waiting time for users. This artificially delays UI feedback even when the server response is fast. Consider reducing this to 1-2 seconds or removing it entirely, relying on the natural API response time to provide feedback.
| const minDelay = new Promise((resolve) => setTimeout(resolve, 5000)); | |
| try { | |
| // Fetch fresh data from server and wait minimum 5 seconds | |
| await Promise.all([getAllFacultyCertificates({ filter: searchFilter }), minDelay]); | |
| try { | |
| // Fetch fresh data from server and refresh immediately when done | |
| await getAllFacultyCertificates({ filter: searchFilter }); |
| router.refresh(); | ||
| await minDelay; |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The router.refresh() call is executed before the minimum delay completes, which means the UI will update before the loading state ends. This creates a confusing UX where the refresh completes but the button still shows as loading. The refresh should occur after the minimum delay or the delay should be removed.
| router.refresh(); | |
| await minDelay; | |
| await minDelay; | |
| router.refresh(); |
|
|
||
| const handleRefresh = async () => { | ||
| setIsRefreshing(true); | ||
| const minDelay = new Promise((resolve) => setTimeout(resolve, 5000)); |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimum delay of 5 seconds is unnecessarily long and creates poor user experience. Users expect immediate feedback when clicking refresh. Consider reducing this to 1-2 seconds maximum or removing the artificial delay entirely.
| const minDelay = new Promise((resolve) => setTimeout(resolve, 5000)); | |
| const minDelay = new Promise((resolve) => setTimeout(resolve, 1000)); |
| // Use first signatory as default if certificate doesn't have one selected | ||
| const [selectedSignatoryId, setSelectedSignatoryId] = useState<string>( | ||
| certificate.signatoryId?.toString() || signatories[0]?.employeeId?.toString() || '', |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the first signatory as default when certificate doesn't have one could be incorrect. If the certificate has no signatoryId, it's safer to default to an empty string and require explicit selection, preventing accidental assignment of the wrong signatory.
| // Use first signatory as default if certificate doesn't have one selected | |
| const [selectedSignatoryId, setSelectedSignatoryId] = useState<string>( | |
| certificate.signatoryId?.toString() || signatories[0]?.employeeId?.toString() || '', | |
| // Default to the certificate's existing signatory; otherwise require explicit selection | |
| const [selectedSignatoryId, setSelectedSignatoryId] = useState<string>( | |
| certificate.signatoryId?.toString() || '', |
| // Signatory is required for approval - disable approve button if no signatory selected | ||
| const isSignatoryRequired = signatories.length > 0 && !selectedSignatoryId; |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isSignatoryRequired variable name is misleading. It actually indicates whether a signatory selection is missing (not whether it's required). Consider renaming to isSignatoryMissing or hasNoSignatorySelected for clarity.
| // Signatory is required for approval - disable approve button if no signatory selected | |
| const isSignatoryRequired = signatories.length > 0 && !selectedSignatoryId; | |
| // Signatory is required for approval - this is true when a signatory exists but none is currently selected | |
| const isSignatoryMissing = signatories.length > 0 && !selectedSignatoryId; | |
| // Backwards-compatible alias: keep old name for existing usages | |
| const isSignatoryRequired = isSignatoryMissing; |
| const handleOpenChange = async (open: boolean) => { | ||
| setIsOpen(open); | ||
|
|
||
| if (open && !pdfUrl) { | ||
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| try { | ||
| const { base64 } = await getUnsignedCertificatePDF(certificateId); | ||
| const blob = base64ToBlob(base64, 'application/pdf'); | ||
| const url = URL.createObjectURL(blob); | ||
| setPdfUrl(url); | ||
| } catch (err) { | ||
| setError(t('previewError')); | ||
| console.error('Failed to load PDF preview:', err); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PDF preview dialog loads the PDF on every open but only cleans up the URL when the dialog closes. If the user opens the dialog multiple times in succession, this could create memory leaks from uncleaned blob URLs. Consider cleaning up the previous URL before creating a new one.
| export function base64ToBlob(base64: string, contentType: string): Blob { | ||
| const byteCharacters = atob(base64); | ||
| const byteNumbers = new Array(byteCharacters.length); | ||
| for (let i = 0; i < byteCharacters.length; i++) { | ||
| byteNumbers[i] = byteCharacters.charCodeAt(i); | ||
| } | ||
| const byteArray = new Uint8Array(byteNumbers); | ||
| return new Blob([byteArray], { type: contentType }); | ||
| } |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base64ToBlob function doesn't validate that the input is valid base64. If invalid base64 is passed, atob() will throw an exception. Consider adding try-catch error handling or input validation to provide more graceful error messages.
This pull request introduces significant enhancements and refactoring to the certificate module, improving both student and faculty workflows for certificate requests and management. The changes include expanded server actions, user interface improvements for certificate request and download, and comprehensive documentation updates. The most important updates are grouped below.
Certificate Module Enhancements
CLAUDE.mddescribing the certificate module's workflow, API actions, key types, and status flow, making it easier for developers to understand and extend the certificate functionality.certificates.actions.tsto support certificate regeneration, signatory selection, operator workflows, student search, and improved PDF download logic (now using base64 encoding for easier handling in the frontend). [1] [2] [3] [4] [5]Student Certificate Request UI Improvements
Certificate Download and History Table Updates
Faculty Certificate Management Actions
Developer Workflow and Environment