Make suggested Changes to exam level dashboard for exam board#776
Make suggested Changes to exam level dashboard for exam board#776Karthikeyantestpress wants to merge 7 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the exam level dashboard by introducing a new 'Appeared' metric for student test submissions, alongside robust location-based filtering for exam centers. It also adds a foundational Excel export feature for comprehensive data analysis and makes minor UI adjustments to integrate these new functionalities seamlessly. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new features to the exam dashboard, including filtering by location, an 'appeared' status, and a dummy Excel export. The changes are functional but there are several areas for improvement regarding code quality, maintainability, and security. Specifically, there's a security concern with using a CDN for a third-party script. There are also multiple instances of code duplication and verbose logic, particularly in the Alpine.js components and the new export script. Refactoring these areas to use Alpine's $store magic property, extracting repeated logic into helper functions, and sourcing data from a single source of truth will greatly improve the codebase.
| exportBtn.addEventListener('click', function () { | ||
| if (typeof XLSX === 'undefined') return; | ||
|
|
||
| // Hard-coded data for design phase (not from DOM/Alpine) | ||
| var pipeline = [ | ||
| { label: 'Scheduled', completed: 500 }, | ||
| { label: 'Registered', completed: 450 }, | ||
| { label: 'Biometrics', completed: 380 }, | ||
| { label: 'Photo', completed: 350 }, | ||
| { label: 'Present', completed: 320 }, | ||
| { label: 'Appeared', completed: 300 }, | ||
| { label: 'Absent', completed: 100 } | ||
| ]; | ||
|
|
||
| var rows = [ | ||
| { | ||
| name: 'Downtown Test Center', | ||
| state: 'Tamil Nadu', | ||
| city: 'Chennai', | ||
| venue: 'Chennai Central Hall', | ||
| address: '123 Main St, Chennai', | ||
| contactName: 'Raj Kumar', | ||
| contactPhone: '+91 98765 43210', | ||
| sessionCount: 4, | ||
| capacity: 900, | ||
| allocated: 832, | ||
| registered: 800, | ||
| biometrics: 780, | ||
| photos: 765, | ||
| present: 768, | ||
| submitted: 750, | ||
| absent: 64, | ||
| online: true, | ||
| clientNodes: '10/10', | ||
| battery: 85 | ||
| }, | ||
| { | ||
| name: 'North Campus Hub', | ||
| state: 'Karnataka', | ||
| city: 'Bangalore', | ||
| venue: 'Bangalore North Campus', | ||
| address: '45 College Rd, Bangalore', | ||
| contactName: 'Priya S.', | ||
| contactPhone: '+91 87654 32109', | ||
| sessionCount: 3, | ||
| capacity: 700, | ||
| allocated: 650, | ||
| registered: 620, | ||
| biometrics: 610, | ||
| photos: 600, | ||
| present: 598, | ||
| submitted: 590, | ||
| absent: 52, | ||
| online: true, | ||
| clientNodes: '10/10', | ||
| battery: 72 | ||
| }, | ||
| { | ||
| name: 'South Zone Centre', | ||
| state: 'Telangana', | ||
| city: 'Hyderabad', | ||
| venue: 'Hyderabad South Block', | ||
| address: '78 MG Rd, Hyderabad', | ||
| contactName: 'Anil M.', | ||
| contactPhone: '+91 76543 21098', | ||
| sessionCount: 2, | ||
| capacity: 500, | ||
| allocated: 450, | ||
| registered: 0, | ||
| biometrics: 0, | ||
| photos: 0, | ||
| present: 0, | ||
| submitted: 0, | ||
| absent: 0, | ||
| online: false, | ||
| clientNodes: '0/10', | ||
| battery: 5 | ||
| } | ||
| ]; | ||
|
|
||
| var now = new Date(); | ||
| var tsLabel = now.getFullYear() + '-' + String(now.getMonth() + 1).padStart(2, '0') + '-' + String(now.getDate()).padStart(2, '0') + ' ' + String(now.getHours()).padStart(2, '0') + ':' + String(now.getMinutes()).padStart(2, '0'); | ||
| var tsFile = now.getFullYear() + '-' + String(now.getMonth() + 1).padStart(2, '0') + '-' + String(now.getDate()).padStart(2, '0') + '-' + String(now.getHours()).padStart(2, '0') + '-' + String(now.getMinutes()).padStart(2, '0'); | ||
| var totalCandidates = 600; | ||
| var centersHeaders = ['Center Name', 'State', 'City', 'Venue', 'Address', 'Contact Name', 'Contact Phone', 'Sessions', 'Capacity', 'Scheduled', 'Registered', 'Biometrics', 'Photos', 'Present', 'Appeared', 'Absent', 'Health', 'Client Nodes', 'Battery']; | ||
| var summaryAoa = [ | ||
| ['Exam', 'ACCP Professional Certification Exam'], | ||
| ['Date & Time', 'Feb 12, 2026 · 9:00 AM to 12:00 PM'], | ||
| ['Code', 'EX-2026-001'], | ||
| ['Exported at', tsLabel], | ||
| [], | ||
| ['Overall stats', 'Value'] | ||
| ]; | ||
| pipeline.forEach(function (item) { | ||
| var completed = item.completed != null ? item.completed : 0; | ||
| var percent = totalCandidates ? Math.round((completed / totalCandidates) * 100) : 0; | ||
| var valueLabel = percent + '% (' + completed + '/' + totalCandidates + ')'; | ||
| summaryAoa.push([item.label, valueLabel]); | ||
| }); | ||
| summaryAoa.push([]); | ||
| summaryAoa.push(['Exam center details']); | ||
| summaryAoa.push(centersHeaders); | ||
| rows.forEach(function (row) { | ||
| summaryAoa.push([ | ||
| row.name || '', | ||
| row.state || '', | ||
| row.city || '', | ||
| row.venue || '', | ||
| row.address || '', | ||
| row.contactName || '', | ||
| row.contactPhone || '', | ||
| row.sessionCount != null ? row.sessionCount : '', | ||
| row.capacity != null ? row.capacity : '', | ||
| row.allocated != null ? row.allocated : '', | ||
| row.registered != null ? row.registered : '', | ||
| row.biometrics != null ? row.biometrics : '', | ||
| row.photos != null ? row.photos : '', | ||
| row.present != null ? row.present : '', | ||
| row.submitted != null ? row.submitted : '', | ||
| row.absent != null ? row.absent : '', | ||
| row.online === true ? 'Online' : (row.online === false ? 'Offline' : ''), | ||
| row.clientNodes || '', | ||
| row.battery != null ? row.battery + '%' : '' | ||
| ]); | ||
| }); | ||
| var summarySheet = XLSX.utils.aoa_to_sheet(summaryAoa); | ||
| var centersAoa = [ | ||
| ['Exam center details'], | ||
| ['Exported at', tsLabel], | ||
| [], | ||
| centersHeaders | ||
| ]; | ||
| rows.forEach(function (row) { | ||
| centersAoa.push([ | ||
| row.name || '', | ||
| row.state || '', | ||
| row.city || '', | ||
| row.venue || '', | ||
| row.address || '', | ||
| row.contactName || '', | ||
| row.contactPhone || '', | ||
| row.sessionCount != null ? row.sessionCount : '', | ||
| row.capacity != null ? row.capacity : '', | ||
| row.allocated != null ? row.allocated : '', | ||
| row.registered != null ? row.registered : '', | ||
| row.biometrics != null ? row.biometrics : '', | ||
| row.photos != null ? row.photos : '', | ||
| row.present != null ? row.present : '', | ||
| row.submitted != null ? row.submitted : '', | ||
| row.absent != null ? row.absent : '', | ||
| row.online === true ? 'Online' : (row.online === false ? 'Offline' : ''), | ||
| row.clientNodes || '', | ||
| row.battery != null ? row.battery + '%' : '' | ||
| ]); | ||
| }); | ||
| var centersSheet = XLSX.utils.aoa_to_sheet(centersAoa); | ||
| var wb = XLSX.utils.book_new(); | ||
| XLSX.utils.book_append_sheet(wb, summarySheet, 'Summary'); | ||
| XLSX.utils.book_append_sheet(wb, centersSheet, 'Centers'); | ||
| XLSX.writeFile(wb, 'exam-dashboard-export-' + tsFile + '.xlsx'); | ||
| }); |
There was a problem hiding this comment.
The Excel export functionality has a few issues:
- Hardcoded Data: It uses hardcoded
pipelineandrowsdata instead of sourcing it from theexamExportAlpine store. This means the exported file will not reflect any filtering or sorting applied by the user. - Code Duplication: The logic to map a
rowobject to an array for the Excel sheet is duplicated for both the 'Summary' and 'Centers' sheets. This violates the DRY (Don't Repeat Yourself) principle.
This entire block should be refactored to use the data from Alpine.store('examExport') and to extract the duplicated row processing logic into a helper function.
| {% block body_class %}overflow-y-scroll min-h-screen bg-gray-100 dark:bg-neutral-900{% endblock body_class %} | ||
|
|
||
| {% block content %} | ||
| <script src="https://cdn.sheetjs.com/xlsx-0.20.0/package/dist/xlsx.full.min.js"></script> |
There was a problem hiding this comment.
Loading external scripts from a CDN can introduce security vulnerabilities (if the CDN is compromised) and reliability issues (if the CDN is unavailable). It is recommended to manage third-party libraries like SheetJS (xlsx) as part of your project's dependencies using a package manager (like npm or yarn) and bundle them with your application's assets.
| const loc = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examLocationFilter') | ||
| if (loc) { | ||
| if (loc.state && loc.state !== 'All') list = list.filter(r => r.state === loc.state) | ||
| if (loc.city && loc.city !== 'All') list = list.filter(r => r.city === loc.city) | ||
| } |
There was a problem hiding this comment.
The filteredRows getter uses a verbose check typeof Alpine !== 'undefined' && Alpine.store && ... to access the Alpine store. This can be simplified by using the $store magic property within the component, which improves readability and conciseness.
| const loc = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examLocationFilter') | |
| if (loc) { | |
| if (loc.state && loc.state !== 'All') list = list.filter(r => r.state === loc.state) | |
| if (loc.city && loc.city !== 'All') list = list.filter(r => r.city === loc.city) | |
| } | |
| const loc = this.$store.examLocationFilter; | |
| if (loc) { | |
| if (loc.state && loc.state !== 'All') list = list.filter(r => r.state === loc.state) | |
| if (loc.city && loc.city !== 'All') list = list.filter(r => r.city === loc.city) | |
| } |
| }) | ||
| } | ||
| }" | ||
| x-init="$watch('sortedRows', function(v) { var s = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examExport'); if (s) s.rows = v || []; }); $nextTick(function() { var s = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examExport'); if (s) s.rows = $data.sortedRows || []; })" |
There was a problem hiding this comment.
The x-init attribute contains a long and hard-to-read line of JavaScript. To improve readability and maintainability, this logic should be extracted into a method within the x-data object.
For example, you could add a method to x-data:
updateExportStore() {
const store = Alpine.store('examExport');
if (store) {
store.rows = this.sortedRows || [];
}
}And then simplify x-init:
x-init="$watch('sortedRows', () => updateExportStore()); $nextTick(() => updateExportStore())"| get locationLabel() { | ||
| var s = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examLocationFilter'); | ||
| if (!s || !s.state || s.state === 'All') return 'Filter by location'; | ||
| return (s.city && s.city !== 'All') ? (s.state + ' – ' + s.city) : s.state; | ||
| }, | ||
| applyLocationFilter() { | ||
| var store = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examLocationFilter'); | ||
| var stateEl = document.getElementById('exam-location-state'); | ||
| var cityEl = document.getElementById('exam-location-city'); | ||
| if (store && stateEl && cityEl) { | ||
| store.state = stateEl.value || ''; | ||
| store.city = cityEl.value || ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
The component repeatedly uses verbose checks like typeof Alpine !== 'undefined' && Alpine.store && ... to access the Alpine store. Within an Alpine component, you can access stores more cleanly using the $store magic property. This will make the code more concise and easier to read.
| get locationLabel() { | |
| var s = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examLocationFilter'); | |
| if (!s || !s.state || s.state === 'All') return 'Filter by location'; | |
| return (s.city && s.city !== 'All') ? (s.state + ' – ' + s.city) : s.state; | |
| }, | |
| applyLocationFilter() { | |
| var store = typeof Alpine !== 'undefined' && Alpine.store && Alpine.store('examLocationFilter'); | |
| var stateEl = document.getElementById('exam-location-state'); | |
| var cityEl = document.getElementById('exam-location-city'); | |
| if (store && stateEl && cityEl) { | |
| store.state = stateEl.value || ''; | |
| store.city = cityEl.value || ''; | |
| } | |
| } | |
| get locationLabel() { | |
| const s = this.$store.examLocationFilter; | |
| if (!s || !s.state || s.state === 'All') return 'Filter by location'; | |
| return (s.city && s.city !== 'All') ? `${s.state} – ${s.city}` : s.state; | |
| }, | |
| applyLocationFilter() { | |
| const store = this.$store.examLocationFilter; | |
| const stateEl = document.getElementById('exam-location-state'); | |
| const cityEl = document.getElementById('exam-location-city'); | |
| if (store && stateEl && cityEl) { | |
| store.state = stateEl.value || ''; | |
| store.city = cityEl.value || ''; | |
| } | |
| } |
| document.addEventListener('alpine:init', function () { | ||
| if (typeof Alpine !== 'undefined' && !Alpine.store('examLocationFilter')) { | ||
| Alpine.store('examLocationFilter', { state: '', city: '' }); | ||
| } | ||
| if (typeof Alpine !== 'undefined' && !Alpine.store('examExport')) { | ||
| Alpine.store('examExport', { rows: [], pipeline: [] }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The typeof Alpine !== 'undefined' check is redundant inside an alpine:init event listener, as the Alpine object is guaranteed to be available in this context. You can simplify the code for better readability.
document.addEventListener('alpine:init', function () {
if (!Alpine.store('examLocationFilter')) {
Alpine.store('examLocationFilter', { state: '', city: '' });
}
if (!Alpine.store('examExport')) {
Alpine.store('examExport', { rows: [], pipeline: [] });
}
});
|
1 participant
Add appeared data in overall stats and exam center (appeared is for students actually submitted the test from present students)
Add filter by location (should filter by state and city)
Add dummy excel export (sample data)
Distinguishing Overall stats panel and exam center table panel with distinct colors