380 statistics for the statistic cards #404
Conversation
…g into 380-statistics-for-the-statistic-cards
WalkthroughThis pull request introduces several enhancements related to the statistics tracking feature for guide logs within the application. It includes a new migration for adding an index to the Changes
Possibly Related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (7)
frontend/src/services/statisticsService.js (1)
6-6: That endpoint lookin' kinda sus with that trailing slash! 👀Consider removing the trailing slash from the API endpoint for consistency.
- const response = await apiClient.get(`/statistics/`); + const response = await apiClient.get('/statistics');backend/migrations/20241219181037-add-index-guidelog.js (2)
1-2: Yo, that 'use strict' is as redundant as putting ketchup on poutine!Since we're working with ES modules, they're automatically in strict mode. Let's keep it clean, eh?
-"use strict"; -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
6-11: Those example comments are just taking up space like a moose in a phone booth!The example comments don't add value to our migration. Let's remove them to keep our code as clean as a fresh sheet of ice.
async up(queryInterface, Sequelize) { - /** - * Add altering commands here. - * - * Example: - * await queryInterface.createTable('users', { id: Sequelize.INTEGER }); - */ await queryInterface.addIndex("guide_logs", ["showingTime"], {Also applies to: 22-26
frontend/src/scenes/dashboard/HomePageComponents/StatisticCards/StatisticCards.jsx (1)
23-27: This JSX is cleaner than fresh snow, but we can make it even better!Let's extract the arrow logic into a separate component to keep things tidy.
+ const ChangeIndicator = ({ changeRate }) => { + if (changeRate === 0) return null; + return changeRate >= 0 ? <ArrowUpwardIcon /> : <ArrowDownwardIcon />; + }; return ( <div className={styles.statisticCard}> <div className={styles.metricName}>{metricName}</div> <div className={styles.metricValue}>{metricValue}</div> <div className={styles.changeRate}> <span style={{ color: getRateColor() }} className={styles.change}> - {changeRate !== 0 && - (changeRate >= 0 ? <ArrowUpwardIcon /> : <ArrowDownwardIcon />)} + <ChangeIndicator changeRate={changeRate} /> {getChangeRate()} </span> vs last month </div> </div> );backend/src/test/mocks/guidelog.mock.js (1)
4-11: The constructor's looking sharp, but it could use some validation, eh!Let's add some type checking to ensure we're getting the right kinds of values.
constructor(id, guideType) { + if (!Number.isInteger(id) || id < 1) { + throw new Error('ID must be a positive integer'); + } + if (!Object.values(GuideType).includes(guideType)) { + throw new Error('Invalid guide type'); + } this.guideLog = { guideType, guideId: id, userId: "1",backend/src/test/unit/controllers/statistics.test.js (1)
18-28: He's nervous, but on the surface the tests look calm and ready! 🎯The success case test needs more assertions on the response structure.
Add structure validation:
expect(status).to.be.equal(200); -expect(json).to.be.equal(guideLogList); +expect(json).to.be.an('array'); +json.forEach(stat => { + expect(stat).to.have.all.keys(['views', 'change', 'guideType']); + expect(stat.views).to.be.a('number'); + expect(stat.change).to.be.a('number'); + expect(stat.guideType).to.be.a('string'); +});frontend/src/scenes/dashboard/Dashboard.jsx (1)
10-27: Yo dawg, this switch case is straight fire! 🔥The
mapMetricNamefunction is clean and handles all guide types effectively. However, we could make it even more robust.Consider this enhancement to make it more maintainable:
+const GUIDE_TYPE_LABELS = { + popup: "Popups views", + hint: "Hints views", + banner: "Banners views", + link: "Links views", + tour: "Tours views", + checklist: "Checklists views", +}; const mapMetricName = (guideType) => { - switch (guideType) { - case "popup": - return "Popups views"; - case "hint": - return "Hints views"; - case "banner": - return "Banners views"; - case "link": - return "Links views"; - case "tour": - return "Tours views"; - case "checklist": - return "Checklists views"; - default: - return "Unknown"; - } + return GUIDE_TYPE_LABELS[guideType] || "Unknown"; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/migrations/20241219181037-add-index-guidelog.js(1 hunks)backend/src/controllers/statistics.controller.js(1 hunks)backend/src/models/GuideLog.js(1 hunks)backend/src/routes/statistics.routes.js(1 hunks)backend/src/server.js(2 hunks)backend/src/service/statistics.service.js(1 hunks)backend/src/test/mocks/guidelog.mock.js(2 hunks)backend/src/test/unit/controllers/statistics.test.js(1 hunks)backend/src/test/unit/services/statistics.test.js(1 hunks)frontend/src/scenes/dashboard/Dashboard.jsx(2 hunks)frontend/src/scenes/dashboard/HomePageComponents/StatisticCards/StatisticCards.jsx(1 hunks)frontend/src/services/statisticsService.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/models/GuideLog.js
🧰 Additional context used
🪛 Biome (1.9.4)
backend/migrations/20241219181037-add-index-guidelog.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (4)
backend/src/routes/statistics.routes.js (2)
1-4: Yo, these imports are straight fire! 🔥
The essential dependencies are properly imported, and the authentication middleware is included - that's crucial for keeping our stats secure, dawg!
5-7: Secure setup with authentication - respect! 💯
The router configuration with authentication middleware ensures all routes are protected. That's the way we roll!
backend/migrations/20241219181037-add-index-guidelog.js (1)
12-14: Beauty! The index looks solid as a hockey puck!
The index addition is well-structured and follows naming conventions. Just verify if this index will be used effectively by your queries.
✅ Verification successful
Yo, this index is gonna be droppin' hot performance beats!
The index on showingTime is absolutely crucial, eh! Found solid evidence it's used in:
- Date range filtering in
statistics.service.jsfor monthly view comparisons - Timestamp tracking in
guidelog.service.jsfor new entries - Already defined in the model with proper indexing configuration
The index will definitely improve query performance for these date-based operations, especially those monthly stats calculations. You're on fire with this one! 🔥
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the showingTime column is used in WHERE clauses or ORDER BY
rg -l "showingTime|showing_time.*WHERE|ORDER\s+BY" --type js
Length of output: 332
Script:
#!/bin/bash
# Let's check how showingTime is actually used in queries
rg "showingTime|showing_time" -B 2 -A 2 backend/src/service/guidelog.service.js
rg "showingTime|showing_time" -B 2 -A 2 backend/src/service/statistics.service.js
# Check the model definition to understand the column usage
rg "showingTime|showing_time" -B 2 -A 2 backend/src/models/GuideLog.js
Length of output: 996
backend/src/server.js (1)
26-26: Knees weak, but this route addition is strong! 💪
The statistics route integration follows the established pattern and is protected by the existing middleware stack.
Let's verify the route protection:
Also applies to: 61-61
✅ Verification successful
Mom's spaghetti is secure! The route is properly protected! 🍝
The statistics route is indeed locked down tight with the authenticateJWT middleware applied at the router level. Found the evidence right there in statistics.routes.js:
router.use(authenticateJWT)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the statistics route is protected by auth middleware
# Search for auth middleware in the statistics routes
rg -A 5 "router\.(get|post|put|delete).*statistics" ./backend/src/routes/
Length of output: 280
Script:
#!/bin/bash
# Let's check if there's any middleware setup in the main server.js file
rg -A 5 "app\.use.*statistics" ./backend/src/server.js
# Also check the route definition and imports in statistics routes
cat ./backend/src/routes/statistics.routes.js
# And let's see if there's a common middleware setup
rg "app\.use.*auth" ./backend/src/server.js
Length of output: 681
frontend/src/scenes/dashboard/HomePageComponents/StatisticCards/StatisticCards.jsx
Show resolved
Hide resolved
frontend/src/scenes/dashboard/HomePageComponents/StatisticCards/StatisticCards.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/controllers/statistics.controller.js(1 hunks)backend/src/service/statistics.service.js(1 hunks)backend/src/test/unit/services/statistics.test.js(1 hunks)frontend/src/scenes/dashboard/Dashboard.jsx(2 hunks)frontend/src/services/statisticsService.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/services/statisticsService.js
- frontend/src/scenes/dashboard/Dashboard.jsx
- backend/src/controllers/statistics.controller.js
🔇 Additional comments (2)
backend/src/service/statistics.service.js (2)
22-35: 🛠️ Refactor suggestion
Knees weak: Redundant type check in reducer! 💪
The guideType check in the reducer is redundant since we're already filtering by guideType in the query.
const { thisMonthViews, lastMonthViews } = logs.reduce(
(acc, log) => {
- if (log.guideType !== guideType) {
- return acc;
- }
if (log.showingTime >= thisMonth) {
acc.thisMonthViews += 1;
} else if (log.showingTime >= lastMonth) {
acc.lastMonthViews += 1;
}
return acc;
},
{ thisMonthViews: 0, lastMonthViews: 0 }
);Likely invalid or redundant comment.
13-21:
Mom's spaghetti moment: Missing pagination could make the server heavy! 🍜
The findAll without limits could cause memory issues with large datasets.
Add pagination:
const logs = await GuideLog.findAll({
where: {
guideType: Number(guideType),
userId,
showingTime: {
[db.Sequelize.Op.gte]: twoMonthsAgo,
},
},
+ limit: 1000,
+ order: [['showingTime', 'DESC']],
});Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/service/statistics.service.js (1)
14-22: 🛠️ Refactor suggestionKnees weak: Missing pagination and index, eh! 🍁
While the query includes proper filtering, consider these performance improvements:
- Add an index for the frequently queried columns
- Implement pagination to handle large datasets
Add an index and pagination:
+ // In your migration file + await queryInterface.addIndex('GuideLogs', ['userId', 'guideType', 'showingTime']); const logs = await GuideLog.findAll({ where: { guideType: Number(guideType), userId, showingTime: { [db.Sequelize.Op.gte]: twoMonthsAgo, }, }, + limit: 1000, });
🧹 Nitpick comments (1)
backend/src/service/statistics.service.js (1)
23-36: Mom's spaghetti: Redundant check detected! 🍁The
guideTypecheck in the reducer is redundant since the query already filters byguideType.Simplify the reducer:
const { thisMonthViews, lastMonthViews } = logs.reduce( (acc, log) => { - if (log.guideType !== guideType) { - return acc; - } if (log.showingTime >= thisMonth) { acc.thisMonthViews += 1; } else if (log.showingTime >= lastMonth) { acc.lastMonthViews += 1; } return acc; }, { thisMonthViews: 0, lastMonthViews: 0 } );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/service/statistics.service.js(1 hunks)
🔇 Additional comments (5)
backend/src/service/statistics.service.js (5)
1-5: LGTM! Clean imports and class structure, eh! 🍁
The imports and class declaration are well-organized and follow best practices.
8-11: Yo, these date calculations are looking fresh now! 🍁
The implementation properly uses month boundaries instead of fixed day offsets, addressing previous concerns about inconsistent results across months.
37-42: Arms are heavy: Percentage calculation needs more edge cases! 🍁
While the division by zero case is handled, consider making the calculation more explicit and handling the case when thisMonthViews is 0.
Improve edge case handling:
-const percentageDifference =
- lastMonthViews === 0
- ? 0
- : Math.round(
- ((thisMonthViews - lastMonthViews) / lastMonthViews) * 100
- );
+const percentageDifference = (() => {
+ if (lastMonthViews === 0) return thisMonthViews > 0 ? 100 : 0;
+ if (thisMonthViews === 0) return -100;
+ return Math.round(((thisMonthViews - lastMonthViews) / lastMonthViews) * 100);
+})();43-49: Clean result formatting, eh! 🍁
The result object is well-structured and maintains consistent casing.
51-54: Palms are sweaty: Improve error logging! 🍁
Replace console.log with a proper logging system for better error tracking in production.
Consider using a logging library:
-console.log(error);
+logger.error('Failed to generate statistics', { error, userId });
|
Looks good to me! Thanks Debora! |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/src/test/unit/controllers/statistics.test.js (2)
1-7: Yo, let's clean up these imports, they're making me nervous! 🍝Consider destructuring
beforeEachalong with the other test lifecycle hooks in line 1:-const { describe, it, afterEach } = require("mocha"); +const { describe, it, beforeEach, afterEach } = require("mocha");
17-28: The success test is straight fire, but we can make it hotter! 🔥The test covers the basics well, but let's add some spice:
- Verify service was called with correct user ID
- Add assertions for response structure
it("should return status 200 and the statistics to all guides", async () => { serviceMock.generateStatistics = sinon .stub(service, "generateStatistics") .resolves(guideLogList); await controller.getStatistics(req, res); + expect(serviceMock.generateStatistics.calledWith(req.user.id)).to.be.true; const status = res.status.firstCall.args[0]; const json = res.json.firstCall.args[0]; expect(status).to.be.equal(200); expect(json).to.be.equal(guideLogList); + expect(Array.isArray(json)).to.be.true; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/test/e2e/statistics.test.mjs(1 hunks)backend/src/test/unit/controllers/statistics.test.js(1 hunks)backend/src/test/unit/services/statistics.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/unit/services/statistics.test.js
🔇 Additional comments (1)
backend/src/test/unit/controllers/statistics.test.js (1)
29-41: Dropping bombs on error cases, but he keeps on forgetting! 💣
The error test case is too simple. We need to test specific error scenarios.
Describe your changes
Briefly describe the changes you made and their purpose.
Issue number
Mention the issue number(s) this PR addresses (e.g., #123).
Please ensure all items are checked off before requesting a review: