Skip to content
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.

Sign up for GitHub

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

feat/infra monitor details request #1177

Merged
merged 5 commits into from
Nov 25, 2024
Merged

feat/infra monitor details request #1177

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec1563b
Add parameter for date ranges to return all checks
ajhollid Nov 22, 2024
69281eb
update network request with correct params
ajhollid Nov 22, 2024
6363fa7
add back 50 checks to display for sanity
ajhollid Nov 25, 2024
f90208b
Add dependencies to useEffect
ajhollid Nov 25, 2024
6dab90f
fix logger missing debug level, replace console.error with logger.error
ajhollid Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Client/src/Pages/Infrastructure/Details/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ const InfrastructureDetails = () => {
];
const [monitor, setMonitor] = useState(null);
const { authToken } = useSelector((state) => state.auth);
const [dateRange, setDateRange] = useState("day");
const [dateRange, setDateRange] = useState("all");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Yo dawg, your useEffect's dependency array is emptier than mom's spaghetti!

The dateRange state is initialized but changes to it won't trigger a data refetch because it's not included in the useEffect's dependency array.

Add dateRange to the dependency array:

-	}, []);
+	}, [dateRange, authToken, monitorId]);

Committable suggestion skipped: line range outside the PR's diff.

const { statusColor, determineState } = useUtils();
// These calculations are needed because ResponsiveContainer
// doesn't take padding of parent/siblings into account
Expand All @@ -189,11 +189,11 @@ const InfrastructureDetails = () => {
const response = await networkService.getStatsByMonitorId({
authToken: authToken,
monitorId: monitorId,
sortOrder: null,
sortOrder: "asc",
limit: null,
dateRange: dateRange,
numToDisplay: 50,
normalize: true,
numToDisplay: null,
normalize: false,
});
setMonitor(response.data.data);
} catch (error) {
Expand Down
3 changes: 2 additions & 1 deletion Server/db/mongo/modules/monitorModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,15 @@ const getIncidents = (checks) => {

/**
* Get date range parameters
* @param {string} dateRange - 'day' | 'week' | 'month'
* @param {string} dateRange - 'day' | 'week' | 'month' | 'all'
* @returns {Object} Start and end dates
*/
const getDateRange = (dateRange) => {
const startDates = {
day: new Date(new Date().setDate(new Date().getDate() - 1)),
week: new Date(new Date().setDate(new Date().getDate() - 7)),
month: new Date(new Date().setMonth(new Date().getMonth() - 1)),
all: new Date(0),
};
return {
start: startDates[dateRange],
Expand Down
2 changes: 1 addition & 1 deletion Server/validation/joi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ const getMonitorStatsByIdQueryValidation = joi.object({
status: joi.string(),
limit: joi.number(),
sortOrder: joi.string().valid("asc", "desc"),
dateRange: joi.string().valid("day", "week", "month"),
dateRange: joi.string().valid("day", "week", "month", "all"),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Yo, we got a schema mismatch that's heavier than mom's spaghetti! 🍝

The verification confirms inconsistency in dateRange validation across different endpoints:

  • getMonitorStatsById: Accepts "day", "week", "month", "all"
  • getChecks and getTeamChecks: Only accept "day", "week", "month"

These endpoints are actively used in their respective controllers and the validation mismatch could cause confusion. Either:

  1. Update all schemas to include "all" for consistency
  2. Or document why getMonitorStatsById is special and needs the extra option

• Server/validation/joi.js: Update getChecksQueryValidation and getTeamChecksQueryValidation schemas
• Server/controllers/checkController.js: Both getChecks and getTeamChecks endpoints will be affected

🔗 Analysis chain

Yo dawg, the change looks good but we got some consistency issues! 🍝

The addition of "all" as a valid dateRange option aligns with the PR objectives. However, other validation schemas that use dateRange haven't been updated to include this option.

Let's check for potential impacts:

Consider updating these schemas for consistency:

  • getChecksQueryValidation
  • getTeamChecksQueryValidation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all validation schemas using dateRange to ensure consistency

# Search for validation schemas with dateRange
echo "Searching for validation schemas with dateRange..."
rg -B 2 -A 2 'dateRange:\s*joi\.string\(\)\.valid\('

# Search for usages of these validation schemas
echo "Searching for API endpoints using these schemas..."
rg -l 'getMonitorStatsByIdQueryValidation|getChecksQueryValidation|getTeamChecksQueryValidation'

Length of output: 1758

Script:

#!/bin/bash
# Let's check how these validation schemas are actually used in the controllers
echo "Checking monitor controller usage..."
rg -B 3 -A 3 'getMonitorStatsByIdQueryValidation' Server/controllers/monitorController.js

echo "\nChecking check controller usage..."
rg -B 3 -A 3 'getChecksQueryValidation|getTeamChecksQueryValidation' Server/controllers/checkController.js

Length of output: 1689

numToDisplay: joi.number(),
normalize: joi.boolean(),
});
Expand Down