-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.
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
fix: engagement dashboard message and hourly activity charts #35019
base: develop
Are you sure you want to change the base?
Conversation
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: feebcfa The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #35019 +/- ##
========================================
Coverage 59.17% 59.18%
========================================
Files 2822 2822
Lines 68118 68116 -2
Branches 15145 15144 -1
========================================
Hits 40312 40312
+ Misses 24975 24973 -2
Partials 2831 2831
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
tickSize: 0, | ||
// TODO: Get it from theme | ||
tickPadding: 4, | ||
tickRotation: 0, | ||
format: (date): string => moment(date).format('dddd'), | ||
format: (date): string => moment(date).format('dddd').substring(0, 3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format: (date): string => moment(date).format('dddd').substring(0, 3), | |
format: (date): string => moment(date).format('ddd'), |
I think this has the same effect you're looking for.
if (utc) { | ||
response.hours = response.hours.map((hours) => { | ||
const newDay = moment().hour(hours.hour).subtract(displacement, 'days').toDate(); | ||
return { | ||
hour: moment.utc(newDay, 'HH').hour(), | ||
users: hours.users, | ||
}; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this a little strange, since the data should be coming correctly from the BE.
During investigation, I think I figured out what is happening: We're sending the correct dates to the backend. When set to local timezone, we send the start
parameter as midnight of the day in the current timezone, converted to an ISO string (ISO strings afaik are UTC). For example, GMT -3 end of day is '2025-01-24T02:59:59.999Z'
, and UTC end of day is '2025-01-23T23:59:59.999Z'
, meaning GMT -3 midnight is 3 hours after UTC midnight. So the parameter is correct.
The issue is that in the DB, the dates for loginAt
are saved in UTC, and then we extract the hour (which is in UTC time), and sort it. When we show the data, we are not taking that into account, so the value for 12 am for example is actually the value for 12 am UTC, instead of GMT-3. The graph is interpreting the data as as GMT, when it's actually UTC time.
What we need to do is get those hours and convert them to GMT. I think we can do that in the format
property of the ResponsiveBar
component in the ContentForHours
component (or maybe here if that doesn't work).
.changeset/witty-buttons-greet.md
Outdated
--- | ||
"@rocket.chat/meteor": patch | ||
--- | ||
Fixes issue with some charts on engagement dashboard not using utc and missing data visualizers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to update the changeset when the other requested changes are complete
I've also noticed that we're displacing the days in the graph by 1 day. For example today (Thu, 23rd Jan), we are requesting data starting from 23 Jan 23:59, instead of 23 Jan 00:00 |
Proposed changes (including videos or screenshots)
Fixes issue with hourly chat activity chart not accounting for utc, also restores some missing data visualizers on the messages chart
About tests, this is a similar situation as this one #34113 (comment)
Issue(s)
SUP-676