fix: raj pravas arrival time shown in 12 hr format with backward comp…#110
fix: raj pravas arrival time shown in 12 hr format with backward comp…#110
Conversation
Summary of ChangesHello @vendz, 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 primarily focuses on enhancing the display of arrival times within the admin travel booking interface. The 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. 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 primarily focuses on formatting improvements and updating the formatDateTime function to correctly format arrival times into a 12-hour format, enhancing readability and backward compatibility. However, a security audit identified several high-severity Cross-Site Scripting (XSS) vulnerabilities. These issues stem from the use of innerHTML to render unsanitized data from API endpoints in both the summary and main bookings tables, and an insecure pattern of injecting variables into inline onclick attributes. It is strongly recommended to refactor the table rendering logic to use safer DOM manipulation methods such as textContent and addEventListener. Additionally, for improved code quality and maintainability, please ensure variables are properly declared to avoid global namespace pollution and consider renaming the formatDateTime function to better reflect its updated behavior.
| } | ||
|
|
||
| const row = document.createElement('tr'); | ||
| row.innerHTML = `<td>${s.destination}</td><td>${displayStatus}</td><td>${s.count}</td>`; |
There was a problem hiding this comment.
The application renders data from the API response directly into the HTML using innerHTML without sanitization. This can lead to Cross-Site Scripting (XSS) if the API returns malicious content in fields like s.destination or displayStatus. It is safer to use textContent with row.insertCell() to populate the table cells.
row.insertCell().textContent = s.destination;
row.insertCell().textContent = displayStatus;
row.insertCell().textContent = s.count;| : statusLabelMap[b.status] || b.status | ||
| }</td> | ||
| <td> | ||
| <a href="#" onclick="openUpdateModal('${b.bookingid}')">Update Booking Status</a> |
There was a problem hiding this comment.
The bookingid is inserted into an onclick attribute using a template literal within an innerHTML assignment. If b.bookingid contains a single quote, it can break out of the attribute and execute arbitrary JavaScript. This is a form of Cross-Site Scripting (XSS). It is highly recommended to avoid inline event handlers and instead use addEventListener on elements created via document.createElement.
| statusDropdown = document.getElementById('status'); | ||
| issueCreditsField = document.getElementById('issueCreditsField'); | ||
| issueCreditsDropdown = document.getElementById('issueCredits'); |
There was a problem hiding this comment.
These variables are being declared without const, let, or var, which makes them global variables attached to the window object. This is generally considered bad practice as it can lead to naming conflicts and unexpected behavior. They should be explicitly declared using const since they are not reassigned.
const statusDropdown = document.getElementById('status');
const issueCreditsField = document.getElementById('issueCreditsField');
const issueCreditsDropdown = document.getElementById('issueCredits');| @@ -461,15 +524,28 @@ function restoreFilters() { | |||
| function formatDateTime(dateInput) { | |||
There was a problem hiding this comment.
The function name formatDateTime is now misleading as it only formats and returns the time, not the date. For better maintainability, consider renaming it to something like formatTime or formatTime12Hour and updating its call site on line 272.
There was a problem hiding this comment.
Pull request overview
This pull request aims to convert arrival times in the Raj Pravas (travel) booking system from 24-hour format to 12-hour format (with AM/PM) while maintaining backward compatibility. The changes include extensive code formatting updates (converting from double quotes to single quotes, improving indentation and line breaks) and a significant modification to the formatDateTime function.
Changes:
- Modified
formatDateTimefunction to convert times to 12-hour format with AM/PM - Applied consistent code formatting throughout the file (single quotes, improved indentation)
- Maintained all existing functionality for booking status management and filtering
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isNaN(dateObj)) return ''; | ||
|
|
||
| return `${day}-${month}-${year} ${hours}:${minutes}`; | ||
| return to12HourTime(dateObj.getHours(), dateObj.getMinutes()); |
There was a problem hiding this comment.
The new formatDateTime function only returns the time in 12-hour format, but it no longer returns the date component (day, month, year). The old implementation returned the full date and time in the format "DD-MM-YYYY HH:MM". This is a breaking change that will result in loss of date information in the arrival time column, making it impossible to distinguish between arrival times on different days. The function should include the date portion as well to maintain backward compatibility and display complete information.
| return to12HourTime(dateObj.getHours(), dateObj.getMinutes()); | |
| const day = String(dateObj.getDate()).padStart(2, '0'); | |
| const month = String(dateObj.getMonth() + 1).padStart(2, '0'); | |
| const year = dateObj.getFullYear(); | |
| const hours = String(dateObj.getHours()).padStart(2, '0'); | |
| const minutes = String(dateObj.getMinutes()).padStart(2, '0'); | |
| // Preserve full date information in "DD-MM-YYYY HH:MM" format | |
| return `${day}-${month}-${year} ${hours}:${minutes}`; |
| return `${hour}:${minute} ${ampm}`; | ||
| }; | ||
|
|
||
| const timeOnlyPattern = /^(\d{2}):(\d{2})(?::(\d{2}))?$/; |
There was a problem hiding this comment.
The regex pattern for time-only detection does not require leading zeros for single-digit hours (it matches two digits exactly with \d{2}). This means times like "9:30" or "8:45" won't match the pattern, even though they might be valid time inputs. Consider making the hour pattern more flexible to match one or two digits: /^(\d{1,2}):(\d{2})(?::(\d{2}))?$/
| const timeOnlyPattern = /^(\d{2}):(\d{2})(?::(\d{2}))?$/; | |
| const timeOnlyPattern = /^(\d{1,2}):(\d{2})(?::(\d{2}))?$/; |
| const mumbaiPoints = new Set([ | ||
| 'dadar', | ||
| 'dadar (swami narayan temple)', | ||
| 'dadar (swaminarayan temple)', | ||
| 'amar mahal', | ||
| 'airoli', | ||
| 'borivali', | ||
| 'vile parle (sahara star)', | ||
| 'airport terminal 1', | ||
| 'airport terminal 2', | ||
| 'railway station (bandra terminus)', | ||
| 'railway station (kurla terminus)', | ||
| 'railway station (ltt - kurla)', | ||
| 'railway station (csmt)', | ||
| 'railway station (mumbai central)', | ||
| 'mullund', | ||
| 'mulund', | ||
| 'airport t1', | ||
| 'airport t2', | ||
| 'other', | ||
| 'other (enter location in comments)', | ||
| 'railway station (ltt - kurla)', | ||
| 'vile parle (sahara star hotel)', | ||
| 'full car booking', | ||
| 'dadar (pritam hotel)', | ||
| 'borivali (indraprasth shopping centre)', | ||
| 'dadar (pritam da dhaba)', | ||
| 'mulund (sarvoday nagar)' | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Unused variable mumbaiPoints.
| const mumbaiPoints = new Set([ | |
| 'dadar', | |
| 'dadar (swami narayan temple)', | |
| 'dadar (swaminarayan temple)', | |
| 'amar mahal', | |
| 'airoli', | |
| 'borivali', | |
| 'vile parle (sahara star)', | |
| 'airport terminal 1', | |
| 'airport terminal 2', | |
| 'railway station (bandra terminus)', | |
| 'railway station (kurla terminus)', | |
| 'railway station (ltt - kurla)', | |
| 'railway station (csmt)', | |
| 'railway station (mumbai central)', | |
| 'mullund', | |
| 'mulund', | |
| 'airport t1', | |
| 'airport t2', | |
| 'other', | |
| 'other (enter location in comments)', | |
| 'railway station (ltt - kurla)', | |
| 'vile parle (sahara star hotel)', | |
| 'full car booking', | |
| 'dadar (pritam hotel)', | |
| 'borivali (indraprasth shopping centre)', | |
| 'dadar (pritam da dhaba)', | |
| 'mulund (sarvoday nagar)' | |
| ]); |
2 participants
…atiblity