Backend: Upcoming Batches #1830
Conversation
- Updated BatchConfiguration to calculate filled and available seats based on unique student count. - Added source field to StudentBatchUpload to track data origin. - Introduced first_name and last_name properties in StudentBatchUpload for easier access. - Enhanced user account creation process with better error handling and logging. - Implemented signals for automatic student profile creation and status change logging. - Updated email templates and SMTP settings for password emails. - Added migrations for new fields and constraints in models. - Improved withdrawal status handling to deactivate user accounts.
… and update field names for consistency
- Introduced CourseAuditLog model to track changes made to courses, including user, action, old and new values, version bump type, and reason for changes. - Updated course creation and update views to log changes in the CourseAuditLog. - Implemented intelligent versioning logic to determine the type of version bump (MAJOR, MINOR, PATCH) based on changes made to course fields. - Added endpoint to retrieve audit logs for specific courses with pagination support. - Included utility functions for calculating Levenshtein distance to identify potential typo corrections. - Added test endpoint for demonstrating intelligent versioning functionality. - Updated URLs to include new audit log endpoint and test endpoint for versioning.
…management logic - Updated email configuration in common.py for clarity. - Modified production.py to streamline email password retrieval. - Adjusted BatchSerializer to include all fields instead of specific ones. - Enhanced curriculum form handling to support multiple field names for semesters and credits. - Improved student management views to automate status updates and batch syncing. - Added robust filled seats calculation in batch management. - Introduced sync_batches_to_configuration function for batch data consistency.
… and in Generate course List :- Added List Type Feature
…ogic for better clarity in student reports
… generation queries faster execution
…update database name and remove unused batch upload view
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive student management system for programme curriculum, adding advanced batch operations with create, edit, delete functionality and student management capabilities including manual entry and Excel import with validation. The system includes student status tracking (Reported/Not Reported/Withdrawal), bulk operations for mass status updates, and real-time synchronization without page reloads.
Key changes include:
- New student management models for handling batch uploads and password email management
- Enhanced course management with intelligent versioning and audit logging
- Password email generation and distribution system with customizable templates
- Database optimization with new indexes for improved query performance
Reviewed Changes
Copilot reviewed 34 out of 39 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| views_password_email.py | New password email management with secure generation and SMTP delivery |
| views.py | Updated main views with JsonResponse import and removed duplicate instructor functions |
| signals.py | Django signals for automated student profile creation and status logging |
| models_student_management.py | Core student batch upload models with user account integration |
| models_password_email.py | Email logging and template management models |
| models.py | Enhanced course models with audit logging and version control |
| migrations/ | Database migrations for new models and indexes |
| forms.py | Updated form validation with better error handling for percentage fields |
| api/views.py | Enhanced API with intelligent course versioning, student management endpoints, and delete functionality |
| api/urls.py | New URL patterns for student management and password email features |
| api/serializers.py | Updated serializers for batch management |
| globals/api/views.py | Added course deletion proxy endpoint |
| examination/api/views.py | Enhanced PDF generation with better formatting and student result support |
| academic_procedures/api/views.py | Improved student verification with batch upload integration |
| academic_information/views.py | Updated student list generation with batch information |
| academic_information/api/views.py | Optimized Excel generation with caching and database query improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if not hasattr(settings, 'EMAIL_HOST_USER') or not settings.EMAIL_HOST_USER: | ||
| return False, "EMAIL_HOST_USER setting is not configured" |
There was a problem hiding this comment.
Duplicate validation check for EMAIL_HOST_USER on consecutive lines. The second check on line 108-109 is identical to lines 105-106 and should be removed or changed to check a different email setting like EMAIL_HOST_PASSWORD.
| if not hasattr(settings, 'EMAIL_HOST_USER') or not settings.EMAIL_HOST_USER: | |
| return False, "EMAIL_HOST_USER setting is not configured" | |
| if not hasattr(settings, 'EMAIL_HOST_PASSWORD') or not settings.EMAIL_HOST_PASSWORD: | |
| return False, "EMAIL_HOST_PASSWORD setting is not configured" |
| print(f"Delete request for instructor ID: {instructor_id}") | ||
| print(f"User: {request.user}") | ||
|
|
||
| # For API endpoints, we rely on token authentication | ||
| # The frontend handles role-based access control | ||
| print("Processing delete request") |
There was a problem hiding this comment.
Debug print statements should be removed from production code or replaced with proper logging using the logging module.
| def levenshtein_distance(s1, s2): | ||
| """Calculate Levenshtein distance between two strings""" | ||
| if len(s1) < len(s2): | ||
| return levenshtein_distance(s2, s1) |
There was a problem hiding this comment.
The recursive call in levenshtein_distance could cause stack overflow for very large strings. Consider using an iterative approach or adding a maximum string length check.
| return levenshtein_distance(s2, s1) | |
| s1, s2 = s2, s1 |
| return False | ||
|
|
||
| @staticmethod | ||
| def generate_secure_password(length=8): |
There was a problem hiding this comment.
Password length of 8 characters may be considered weak by modern security standards. Consider increasing the default minimum length to 12 characters for better security.
| def generate_secure_password(length=8): | |
| def generate_secure_password(length=12): |
| <p>Your FUSION account has been created successfully. Please find your login credentials below:</p> | ||
| <div style="background-color: #e3f2fd; padding: 20px; border-left: 4px solid #2196f3; margin: 20px 0; border-radius: 4px;"> | ||
| <p style="margin: 8px 0;"><strong>URL:</strong> <a href="http://fusion.iiitdmj.ac.in" style="color: #1976d2;">http://fusion.iiitdmj.ac.in</a></p> |
There was a problem hiding this comment.
Hardcoded HTTP URL should be changed to HTTPS for secure communication, especially when dealing with authentication credentials.
| <p style="margin: 8px 0;"><strong>URL:</strong> <a href="http://fusion.iiitdmj.ac.in" style="color: #1976d2;">http://fusion.iiitdmj.ac.in</a></p> | |
| <p style="margin: 8px 0;"><strong>URL:</strong> <a href="https://fusion.iiitdmj.ac.in" style="color: #1976d2;">https://fusion.iiitdmj.ac.in</a></p> |
| print(f"DEBUG CheckResultView - Found academic_year: {academic_year}") | ||
| else: | ||
| print("DEBUG CheckResultView - No grades found") |
There was a problem hiding this comment.
Debug print statements should be removed from production code or replaced with proper logging using the logging module.
| start_time = time.time() | ||
|
|
||
| # Debug: Check request data | ||
| print(f"DEBUG: Request data: {request.data}") |
There was a problem hiding this comment.
Debug print statements should be removed from production code or replaced with proper logging using the logging module.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 35 out of 40 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
FusionIIIT/applications/programme_curriculum/views_password_email.py:1
- Direct insertion of
settings.EMAIL_HOST_USERinto HTML without escaping could lead to XSS vulnerabilities. Use Django's template escaping orhtml.escape()to sanitize the email address before inserting it into HTML.
# Password Email Management Views for Student Management
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| unique_students = StudentBatchUpload.objects.filter( | ||
| discipline_q, | ||
| year=self.year | ||
| ).values_list('roll_number', flat=True).distinct() |
There was a problem hiding this comment.
The calculate_seats() method performs a database query with complex Q objects and distinct operations on every call. Consider adding database indexes on the branch and year fields, or implement caching for frequently accessed batch seat calculations.
| next_year = (year + 1) % 100 | ||
| self.academic_year = f"{year}-{next_year:02d}" |
There was a problem hiding this comment.
The modulo operation (year + 1) % 100 will cause incorrect academic year formatting for years after 2099. For example, year 2100 would result in '2100-01' instead of '2100-01'. Consider using str(year + 1)[-2:] instead to get the last two digits properly.
| next_year = (year + 1) % 100 | |
| self.academic_year = f"{year}-{next_year:02d}" | |
| next_year = str(year + 1)[-2:] | |
| self.academic_year = f"{year}-{next_year}" |
| date_of_birth=student_upload.date_of_birth or timezone.now().date(), | ||
| user_status='PRESENT', | ||
| address=student_upload.address or '', | ||
| phone_no=int(student_upload.phone_number) if student_upload.phone_number and student_upload.phone_number.isdigit() else 9999999999, |
There was a problem hiding this comment.
Using a hardcoded fallback phone number '9999999999' could create data quality issues. Consider using None/null for missing phone numbers or a more obvious placeholder like '0000000000' to clearly indicate missing data.
| phone_no=int(student_upload.phone_number) if student_upload.phone_number and student_upload.phone_number.isdigit() else 9999999999, | |
| phone_no=int(student_upload.phone_number) if student_upload.phone_number and student_upload.phone_number.isdigit() else None, |
| new_version = manual_version | ||
| bump_type = 'MAJOR' # Assume major if manually overridden | ||
| reason = f"Admin override: {reason}" |
There was a problem hiding this comment.
Setting bump_type = 'MAJOR' for all admin overrides may not be appropriate. The admin might be making a minor correction that should be classified as PATCH or MINOR. Consider validating the manual version against the automatic bump type determination or allowing admin to specify the bump type.
| new_version = manual_version | |
| bump_type = 'MAJOR' # Assume major if manually overridden | |
| reason = f"Admin override: {reason}" | |
| manual_bump_type = data.get('manual_bump_type', None) | |
| if manual_bump_type in ['PATCH', 'MINOR', 'MAJOR']: | |
| bump_type = manual_bump_type | |
| expected_version = calculate_new_version(old_version, bump_type) | |
| # Validate manual version is at least as high as expected for bump type | |
| if float(manual_version) < float(expected_version): | |
| return Response( | |
| {'error': f'Manual version ({manual_version}) is less than expected for {bump_type} bump ({expected_version})'}, | |
| status=status.HTTP_400_BAD_REQUEST | |
| ) | |
| reason = f"Admin override ({bump_type}): {reason}" | |
| else: | |
| # No bump type specified, validate against determined bump type | |
| expected_version = calculate_new_version(old_version, bump_type) | |
| if float(manual_version) < float(expected_version): | |
| return Response( | |
| {'error': f'Manual version ({manual_version}) is less than expected for {bump_type} bump ({expected_version})'}, | |
| status=status.HTTP_400_BAD_REQUEST | |
| ) | |
| reason = f"Admin override: {reason}" | |
| new_version = manual_version |
| """Calculate the new version number based on bump type - all bumps increment by 0.1""" | ||
| if bump_type == 'NONE': | ||
| return current_version | ||
|
|
||
| # Parse current version (e.g., "1.2" -> 1.2) | ||
| current_float = float(current_version) | ||
|
|
||
| # All version bumps increment by 0.1 | ||
| if bump_type in ['MAJOR', 'MINOR', 'PATCH']: | ||
| new_version = current_float + 0.1 | ||
| # Format to 1 decimal place | ||
| return f"{new_version:.1f}" | ||
|
|
||
| return current_version | ||
|
|
||
|
|
There was a problem hiding this comment.
All version bump types (MAJOR, MINOR, PATCH) increment by the same amount (0.1), which defeats the purpose of having different bump types. Consider implementing semantic versioning where MAJOR changes the major number, MINOR changes the minor number, and PATCH changes the patch number.
| """Calculate the new version number based on bump type - all bumps increment by 0.1""" | |
| if bump_type == 'NONE': | |
| return current_version | |
| # Parse current version (e.g., "1.2" -> 1.2) | |
| current_float = float(current_version) | |
| # All version bumps increment by 0.1 | |
| if bump_type in ['MAJOR', 'MINOR', 'PATCH']: | |
| new_version = current_float + 0.1 | |
| # Format to 1 decimal place | |
| return f"{new_version:.1f}" | |
| return current_version | |
| """ | |
| Calculate the new version number based on bump type using semantic versioning. | |
| MAJOR: X.Y.Z -> (X+1).0.0 | |
| MINOR: X.Y.Z -> X.(Y+1).0 | |
| PATCH: X.Y.Z -> X.Y.(Z+1) | |
| """ | |
| if bump_type == 'NONE': | |
| return current_version | |
| # Parse current version (e.g., "1.2.3" or "1.2") | |
| parts = current_version.split('.') | |
| # Ensure we have three parts: major, minor, patch | |
| while len(parts) < 3: | |
| parts.append('0') | |
| try: | |
| major, minor, patch = [int(p) for p in parts[:3]] | |
| except ValueError: | |
| # Fallback: if parsing fails, start from 1.0.0 | |
| major, minor, patch = 1, 0, 0 | |
| if bump_type == 'MAJOR': | |
| major += 1 | |
| minor = 0 | |
| patch = 0 | |
| elif bump_type == 'MINOR': | |
| minor += 1 | |
| patch = 0 | |
| elif bump_type == 'PATCH': | |
| patch += 1 | |
| else: | |
| # Unknown bump type, return current version | |
| return current_version | |
| return f"{major}.{minor}.{patch}" |
| logo_path = os.path.join(settings.MEDIA_ROOT, 'logo2.jpg') | ||
| if os.path.exists(logo_path): | ||
| # Make logo smaller to match website appearance | ||
| logo = Image(logo_path, width=0.8*inch, height=0.8*inch) |
There was a problem hiding this comment.
The logo loading code uses a hardcoded path 'logo2.jpg' without validation. If the logo file doesn't exist or is corrupted, this could cause the PDF generation to fail silently. Add proper error handling and logging for missing or invalid logo files.
|
Tested by Copilot, urgently merging for deployment. |
2 participants
Complete new tab:
Advanced batch operations: Create, edit, delete, and manage student batches
Student management: Add students manually or via Excel import with validation
Status tracking: Student reporting status (Reported/Not Reported/Withdrawal)
Bulk operations: Mass status updates, bulk deletion with conflict resolution
Real-time sync: Background data synchronization without page reloads