Log message web page #1390
Log message web page #1390
Conversation
Initial commit towards making a database to store logs.
Almost finished LogMsg database model
forgot to include these in push
added header file
… refresh button, new picking log msg UI
…now being sent as array for fetching, add logLimit for fetching, validation
There was a problem hiding this comment.
Thanks to @nmqng for addressing another issue. Overall, this works well and the UI is very nice. I've made some comment to consider. Also, there is now a merge conflict so let me know if you want me to deal with it (or you can). Let me know if anything is not clear.
| @@ -58,7 +59,10 @@ const router = createBrowserRouter([ | |||
| { path: 'csvMeters', element: <MetersCSVUploadComponent /> }, | |||
| { path: 'maps', element: <MapsDetailContainer /> }, | |||
| { path: 'units', element: <UnitsDetailComponent /> }, | |||
| { path: 'users', element: <UsersDetailComponent /> } | |||
| { path: 'conversions', element: <ConversionsDetailComponent /> }, | |||
There was a problem hiding this comment.
I think the conversions route was accidentally added twice.
| // initialize log message array to hold log messages | ||
| const initialLogs: any[] = []; | ||
| // log types for filtering | ||
| const logTypes = ['ERROR', 'INFO', 'WARN', 'SILENT']; |
There was a problem hiding this comment.
Normally OED uses an enum in a common place for items like this. I think it may be a good idea. Having said that, I notice src/server/log.js does not. Also, both server files (including src/server/sql/logmsg/create_log_types_enum.sql) have DEBUG as a type but this does not.
|
|
||
| // Handle checkbox change for log type in the table | ||
| const handleTableCheckboxChange = (logType: string) => { | ||
| if (selectedTableLogTypes.includes(logType)) { // Remove log type if already selected |
There was a problem hiding this comment.
OED convention is that comments be on a separate line. Normally it is above the line of code but with if-type statements it often goes inside the braces below that initial line.
| async function handleShowLogTable() { | ||
| // Number of logs being fetched must be between 1 and 1000 | ||
| if (!logLimit || logLimit < 1 || logLimit > 1000) { | ||
| showWarnNotification(translate('log.limit.required')); |
There was a problem hiding this comment.
This works fine but delays the msg until update is hit. In most forms with input OED uses invalid/FormFeedback to give immediate feedback. I'm not certain it will align correctly with the horizontal layout but it would be nice to use if possible.
| @@ -516,7 +516,18 @@ const LocaleTranslationData = { | |||
| "week": "Week", | |||
| "yes": "yes", | |||
| "yesterday": "Yesterday", | |||
| "you.cannot.create.a.cyclic.group": "You cannot create a cyclic group" | |||
| "you.cannot.create.a.cyclic.group": "You cannot create a cyclic group", | |||
| "log.messages": "Log Messages", | |||
There was a problem hiding this comment.
These should be alphabetically placed within each language. To keep together, the log specific ones might be best to start with "log.". On the other hand, "select.all", for example, is probably best under select alphabetically.
| // Log messages date range state | ||
| const [logDateRange, setLogDateRange] = React.useState<TimeInterval>(TimeInterval.unbounded()); | ||
| // Number of log messages to display | ||
| const [logLimit, setLogLimit] = React.useState(0); |
There was a problem hiding this comment.
Do you think it is best to force the user to set the number (as here since 0) or put in some modest value (say 50?) and then the user can change? Not sure but wondering which is better.
|
|
||
| // Open modal with the full log message | ||
| const handleLogMessageClick = (logMessage: string) => { | ||
| setModalLogMessage(logMessage); |
There was a problem hiding this comment.
Would it be easy to add the type and date/time to this so it is completely clear which msg popped up? It would be really nice if the popped up msg had a colored background but not necessary.
| }; | ||
|
|
||
| // React effect to keep track of the "Select All" checkbox state | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
Since the update and table ones are done separately above should this be two separate useEffects? Not a big deal but I don't think both updates can happen.
| // Handle sorting of logs by date | ||
| const handleDateSort = () => { | ||
| const newDateSortOrder = dateSortOrder === 'asc' ? 'desc' : 'asc'; | ||
| const sortedLogs = [...logs].sort((a, b) => { |
There was a problem hiding this comment.
Two thoughts. First, the use of Date vs moment. Second, while sort is used in some places so is orderBy (from Lodash). I think it may be able to deal with the order via parameter and also sort moment objects correctly.
| }; | ||
|
|
||
| // Filter logs based on selected log types and date range | ||
| const filteredLogs = logs.filter(log => { |
There was a problem hiding this comment.
I'm finding the dual use of the date picker for the update and display to be a little confusing. For type and number to display the update area does not impact the logs shown until update is hit. For the date picker it does. I think that difference may confuse users. If you agree then I'm okay with removing this filtering (since can update the logs quickly) or adding it to the log table area (maybe in the Log Time column header?) as a separate date picker.
Hi Steve, I think I can try to fix the merge conflict myself and I'll let you know if I need any help. I will have a look at your comments for this issue after I finish the compareLine one, which is about half completed. |
Description
Continue ianLewis8's work on Draft Pull Request #1327.
The purpose of this pull request is to create the web page that allows admin to look at the log file on a web page.
Fixes #712
Type of change
Checklist